From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web09.1965.1571675333183807145 for ; Mon, 21 Oct 2019 09:28:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Frw42anJ; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1571675332; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ng0NpcTZoCDibYnjfz5f6YoxFXHG/hOWT0Z05pnuU/w=; b=Frw42anJASrH7x017e2lHgRtEBWWfPsDG+zYGsqw6rtGGUJm4KYcl7fVRY4QO7Vlve+bPF FZ3b1MGDg8iRGGx03699FMu0AJUJBBI6Eb1AdvY9noJ78gBfzTJsJ9XrTLywQ0mW3dmRVP 66xY7rtmP8bVEMHxBT0sqENFbkOFEM8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-114-TMLCqWamPTSBv0Z0g-iw3Q-1; Mon, 21 Oct 2019 12:28:47 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7616780183E; Mon, 21 Oct 2019 16:28:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-225.ams2.redhat.com [10.36.116.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0D78319C58; Mon, 21 Oct 2019 16:28:44 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] CryptoPkg: Upgrade OpenSSL to 1.1.1d To: devel@edk2.groups.io, shenglei.zhang@intel.com Cc: Jian J Wang , Xiaoyu Lu , David Woodhouse References: <20191021080652.24852-1-shenglei.zhang@intel.com> From: "Laszlo Ersek" Message-ID: <27634217-cad7-9472-5122-3f62735c801d@redhat.com> Date: Mon, 21 Oct 2019 18:28:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191021080652.24852-1-shenglei.zhang@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: TMLCqWamPTSBv0Z0g-iw3Q-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/21/19 10:06, Zhang, Shenglei wrote: > Update openssl from 1.1.1b to 1.1.1d. > Something needs to be noticed is that, there is a bug existing in the > released 1_1_1d version(894da2fb7ed5d314ee5c2fc9fd2d9b8b74111596), > which causes build failure. So we switch the code base to a usable > version, which is 2 commits later than the stable tag. > Now we use the version c3656cc594daac8167721dde7220f0e59ae146fc. > This log is to fix the build failure. > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2226 > > Cc: Jian J Wang > Cc: Xiaoyu Lu > Signed-off-by: Shenglei Zhang > --- > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 57 ------------------- > .../Library/OpensslLib/OpensslLibCrypto.inf | 49 ---------------- > CryptoPkg/Library/OpensslLib/openssl | 2 +- > 3 files changed, 1 insertion(+), 107 deletions(-) When I try to apply this patch manually, on top of current master (91f98c908627), then "git am" fails. However, if I try to reproduce this patch myself (advancing the submodule to c3656cc594da, and then running "process_files.pl"), then the result ("git diff") matches the code changes in the patch -- not counting CRLF vs. LF, anyway. (It seems like the "git am" failure is due to mixed line-endings within the patch -- the submodule reference hunk uses LFs, not CRLFs. I can live with that.) Having to use openssl at c3656cc594da is unfortunate, but I think it's justified. Unfortunately, with this update, the following build command fails for me (it may fail for other OVMF builds as well, this was simply my first attempt): $ nice build \ -a IA32 \ -p OvmfPkg/OvmfPkgIa32.dsc \ -t GCC48 \ -b DEBUG \ -D SMM_REQUIRE \ -D SECURE_BOOT_ENABLE \ -D NETWORK_IP6_ENABLE \ -D NETWORK_TLS_ENABLE \ -D NETWORK_HTTP_BOOT_ENABLE \ -D E1000_ENABLE \ -n 4 \ --report-file=3D$HOME/tmp/build.ovmf.32.report \ --log=3D$HOME/tmp/build.ovmf.32.log \ --cmd-len=3D65536 \ --genfds-multi-thread The directly failing command is: "gcc" \ -o Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Universal/Variable/Runt= imeDxe/VariableSmm/DEBUG/VariableSmm.dll \ -nostdlib \ -Wl,-n,-q,--gc-sections \ -z common-page-size=3D0x20 \ -Wl,--entry,_ModuleEntryPoint \ -u _ModuleEntryPoint \ -Wl,-Map,Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Universal/Variabl= e/RuntimeDxe/VariableSmm/DEBUG/VariableSmm.map,--whole-archive \ -Wl,-m,elf_i386,--oformat=3Delf32-i386 \ -z common-page-size=3D0x1000 \ -Wl,--start-group,@Build/OvmfIa32/DEBUG_GCC48/IA32/MdeModulePkg/Univers= al/Variable/RuntimeDxe/VariableSmm/OUTPUT/static_library_files.lst,--end-gr= oup \ -g \ -fshort-wchar \ -fno-builtin \ -fno-strict-aliasing \ -Wall \ -Werror \ -Wno-array-bounds \ -ffunction-sections \ -fdata-sections \ -include AutoGen.h \ -fno-common \ -DSTRING_ARRAY_NAME=3DVariableSmmStrings \ -m32 \ -march=3Di586 \ -malign-double \ -fno-stack-protector \ -D EFI32 \ -fno-asynchronous-unwind-tables \ -Wno-address \ -Os \ -mno-mmx \ -mno-sse \ -D DISABLE_NEW_DEPRECATED_INTERFACES \ -Wl,--defsym=3DPECOFF_HEADER_SIZE=3D0x220 \ -Wl,--script=3DBaseTools/Scripts/GccBase.lds And the error message: > Build/OvmfIa32/DEBUG_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/O= UTPUT/OpensslLib.lib(dso_lib.obj): In function `DSO_new_method': > CryptoPkg/Library/OpensslLib/openssl/crypto/dso/dso_lib.c:25: undefined r= eference to `DSO_METHOD_openssl' > Build/OvmfIa32/DEBUG_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/O= UTPUT/OpensslLib.lib(dso_lib.obj): In function `DSO_pathbyaddr': > CryptoPkg/Library/OpensslLib/openssl/crypto/dso/dso_lib.c:314: undefined = reference to `DSO_METHOD_openssl' This is strange, because the missing function is provided by "crypto/dso/dso_openssl.c", which is listed in the INF files. Hmmm. I ran the following command too: $ build \ -p CryptoPkg/CryptoPkg.dsc \ -a IA32 \ -b NOOPT \ -t GCC48 \ -m CryptoPkg/Library/OpensslLib/OpensslLib.inf This compiles OK. The last commands are: > rm -f Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/Opens= slLib/OUTPUT/OpensslLib.lib > > "ar" cr \ > Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLi= b/OUTPUT/OpensslLib.lib \ > @Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslL= ib/OUTPUT/object_files.lst If I check the file Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/= OUTPUT/object_files.lst I definitely see Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslLib/= OUTPUT/openssl/crypto/dso/dso_openssl.obj there. However, if I run nm Build/CryptoPkg/NOOPT_GCC48/IA32/CryptoPkg/Library/OpensslLib/OpensslL= ib/OUTPUT/OpensslLib.lib then the DSO_METHOD_openssl() symbol is reported as undefined: > dso_lib.obj: > U DSO_METHOD_openssl In fact, if I run the "nm" command on "dso_openssl.obj" itself, the output is totally empty! (No symbols in the object file.) Ahh, I know what's up. See the source code in "crypto/dso/dso_openssl.c": > #ifdef DSO_NONE > > static DSO_METHOD dso_meth_null =3D { > "NULL shared library method" > }; > > DSO_METHOD *DSO_METHOD_openssl(void) > { > return &dso_meth_null; > } > #endif The #ifdef comes from OpenSSL commit 5fba3afad017 ("Rework DSO API conditions and configuration option", 2019-04-10), which is part of OpenSSL_1_1_1c. See the change (excerpt): $ git show 5fba3afad017 -- \ crypto/dso/dso_openssl.c \ crypto/include/internal/dso_conf.h.in > commit 5fba3afad01707f4a8856a35500de007a8a256ec > Author: Richard Levitte > Date: Mon Apr 1 06:40:33 2019 +0200 > > Rework DSO API conditions and configuration option > > 'no-dso' is meaningless, as it doesn't get any macro defined. > Therefore, we remove all checks of OPENSSL_NO_DSO. However, there ma= y > be some odd platforms with no DSO scheme. For those, we generate the > internal macro DSO_NONE aand use it. > > Reviewed-by: Paul Dale > (Merged from https://github.com/openssl/openssl/pull/8622) > > diff --git a/crypto/dso/dso_openssl.c b/crypto/dso/dso_openssl.c > index 6626331e9256..eeebd98087b4 100644 > --- a/crypto/dso/dso_openssl.c > +++ b/crypto/dso/dso_openssl.c > @@ -9,7 +9,7 @@ > > #include "dso_locl.h" > > -#if !defined(DSO_VMS) && !defined(DSO_DLCFN) && !defined(DSO_DL) && !def= ined(DSO_WIN32) && !defined(DSO_DLFCN) > +#ifdef DSO_NONE > > static DSO_METHOD dso_meth_null =3D { > "NULL shared library method" > diff --git a/crypto/include/internal/dso_conf.h.in b/crypto/include/inter= nal/dso_conf.h.in > index d6e9d1b1baae..17fae7d8023a 100644 > --- a/crypto/include/internal/dso_conf.h.in > +++ b/crypto/include/internal/dso_conf.h.in > @@ -10,7 +10,6 @@ > > #ifndef HEADER_DSO_CONF_H > # define HEADER_DSO_CONF_H > -{- output_off() if $disabled{dso} -} > {- # The DSO code currently always implements all functions so that no > # applications will have to worry about that from a compilation poin= t > # of view. However, the "method"s may return zero unless that platfo= rm > @@ -18,6 +17,9 @@ > # by a define "DSO_" ... we translate the "dso_scheme" config > # string entry into using the following logic; > my $scheme =3D uc $target{dso_scheme}; > + if (!$scheme) { > + $scheme =3D "NONE"; > + } > my @macros =3D ( "DSO_$scheme" ); > if ($scheme eq 'DLFCN') { > @macros =3D ( "DSO_DLFCN", "HAVE_DLFCN_H" ); > @@ -26,5 +28,4 @@ > } > join("\n", map { "# define $_" } @macros); -} > # define DSO_EXTENSION "{- $target{dso_extension} -}" > -{- output_on() if $disabled{dso} -} > #endif Sure enough, "build.info" invokes the generator on this template file too: > DEPEND[include/openssl/opensslconf.h]=3Dconfigdata.pm > GENERATE[include/openssl/opensslconf.h]=3Dinclude/openssl/opensslconf.h.i= n > DEPEND[crypto/include/internal/bn_conf.h]=3Dconfigdata.pm > GENERATE[crypto/include/internal/bn_conf.h]=3Dcrypto/include/internal/bn_= conf.h.in > DEPEND[crypto/include/internal/dso_conf.h]=3Dconfigdata.pm > GENERATE[crypto/include/internal/dso_conf.h]=3Dcrypto/include/internal/ds= o_conf.h.in Unfortunately, it seems like the DSO_NONE internal macro is *not* generated, in our case. ... Ah. Our "process_files.pl" script manually generates "opensslconf.h", from the configuration data. But, we have never done the same for "dso_conf.h". Thus far, we've gotten away with it, because the *absence* of all DSO_xxx flags happened to do the right thing for us. But now, the "right thing for us" actually depends on a new macro, DSO_NONE, and for getting that, we need to invoke the generator on "dso_conf.h.in" too. Otherwise, "crypto/dso/dso_openssl.c" gets pre-processed to an empty source file, as a result of openssl commit 5fba3afad017. Therefore, the bug is in "process_files.pl". Please invoke the generator on "dso_conf.h.in" too, similarly to "opensslconf.h.in". ... And now I'm left with the question: did you test this patch at all, with any real platform? Laszlo