Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl

["Laszlo Ersek" <lersek@redhat.com>]

On 05/14/19 14:41, Lu, XiaoyuX wrote:
> Hi Laszlo, 
> 	I think process_files.pl is used to control which of OpenSSL source files we need which we don't need.
> 	If we have unwanted files, the effective way is exclude them directly in process_files.pl.
>     You can see process_files.pl
> 	
> 	> 129   ┆ ┆ ┆ ┆ ┆ next if $s =~ "crypto/bio/b_print.c";
> 
> 	Qing Long also use this way to exclude unwanted file.
> 
> 	If the file (example: rand_unix.c) is used by OpenSSL internal, We can't exclude it in process_files.pl, 
> 	Than we consider submitting patches for OpenSSL.
> 
> 	What do you think?

I agree to excluding "rand_unix.c" similarly to "b_print.c"; that is,
with "next if" in the "process_files.pl" script.

Thanks
Laszlo

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
> Sent: Monday, May 13, 2019 11:13 PM
> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude unnecessary files in process_files.pl
> 
> On 05/10/19 10:51, Xiaoyu lu wrote:
>> Hi, Laszlo:
>>
>> Thank you for your time.
>>
>> I try the method you mentioned.
>>
>>> (1) Therefore, the right thing to do here is to add "no-store" to the above list, in my opinion. Can you try that, please?
>>>
>>> And, this change should be a standalone patch, similarly to patch v2 1/6 in this series.
>>
>> (1)  OpenSSL configure script don't support no-store option.
>> It will lead to configure error.
>>
>> Unsupported options: no-store
>>
>>> (2a) Therefore, we should modify the "randfile.c" source file, with an upstream OpenSSL contribution, to hide the function definitions, when OPENSSL_SYS_UEFI is defined. In other words, continue with Qin Long's approach from commit fb4844bbc62f.
>>
>> I think this is the best way. But the openssl community takes time to accept the patch.
>> I just let OpenSSL work for UEFI. So UEFI can use the new algorithm in OpenSSL_1_1_1.
>> I am willing to continue to modify this later.
> 
> Please pick one of two:
> 
> - file a new TianoCore BZ about cleaning up this technical debt, and paste the BZ URL into the code, as a comment
> 
> - delay TianoCore BZ#1089 to the next edk2-stable release, and work with upstream OpenSSL to compile out parts of "randfile.c".
> 
> Thanks
> Laszlo
> 
>>
>>> (2b) Alternatively, I'm noticing that "rand" is just another module (similar to "store", see above). Assuming we really don't need RAND_* functions for anything in edk2: have we tried configuring OpenSSL, for the edk2 build, with the "no-rand" parameter?
>>
>> (2) I'm afraid not. Same as (1)
>>
>> ***** Unsupported options: no-rand
>>
>> Thanks,
>> Xiaoyu.
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Thursday, May 9, 2019 9:43 PM
>> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux.lu@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude 
>> unnecessary files in process_files.pl
>>
>> On 05/09/19 07:23, Xiaoyu lu wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1089
>>>
>>> When running process_files.py to configure OpenSSL, we can exclude 
>>> some unnecessary files. This can reduce porting time, compiling time 
>>> and library size.
>>
>> Indeed.
>>
>>> OpenSSL_1_1_1(1708e3e85b4a8) add a STORE module (crypto/store/*).
>>
>> This statement is incorrect (or, minimally, inexact). According to the following command:
>>
>> $ git log --oneline --reverse OpenSSL_1_1_1b -- crypto/store/ \
>>   | head -n 1
>>
>> the first OpenSSL commit that added files to crypto/store/ was:
>>
>>> commit a5db6fa5760f21d16d59e025e930c02456e00fef
>>> Author: Richard Levitte <levitte@openssl.org>
>>> Date:   Thu May 1 03:53:12 2003 +0000
>>>
>>>     Define a STORE type.  For documentation, read the entry in CHANGES,
>>>     crypto/store/README, crypto/store/store.h and crypto/store/str_locl.h.
>>
>> This commit goes back to 2003, and is part of releae OpenSSL_0_9_7d.
>>
>> Instead, let's check what the following command reports:
>>
>> $ git log --oneline --reverse \
>>     OpenSSL_1_1_0j..OpenSSL_1_1_1b -- crypto/store/ \
>>   | head -1
>>
>> It states that the first commit after OpenSSL_1_1_0j, but not after OpenSSL_1_1_1b, to modify the "crypto/store/" subdirectory, was commit 71a5516dcc8a ("Add the STORE module", 2017-06-29).
>>
>> If we investigate that commit:
>>
>> $ git show --stat 71a5516dcc8a
>>
>> we see that the commit modifies the Configure script:
>>
>>>  Configure                       |   2 +-
>>
>> So let's check that part of the diff in detail:
>>
>> $ git show 71a5516dcc8a -- Configure
>>
>> And we get:
>>
>>> diff --git a/Configure b/Configure
>>> index 2eacb2312e34..e302a58abb71 100755
>>> --- a/Configure
>>> +++ b/Configure
>>> @@ -310,7 +310,7 @@ $config{sdirs} = [
>>>      "bn", "ec", "rsa", "dsa", "dh", "dso", "engine",
>>>      "buffer", "bio", "stack", "lhash", "rand", "err",
>>>      "evp", "asn1", "pem", "x509", "x509v3", "conf", "txt_db", "pkcs7",
>>>      "pkcs12", "comp", "ocsp", "ui",
>>> -    "cms", "ts", "srp", "cmac", "ct", "async", "kdf"
>>> +    "cms", "ts", "srp", "cmac", "ct", "async", "kdf", "store"
>>>      ];
>>>  # test/ subdirectories to build
>>>  $config{tdirs} = [ "ossl_shim" ];
>>
>> We can see that the "store" module is added after modules such as "cms", "ts", "srp", and so on.
>>
>> Now, if you look at "CryptoPkg/Library/OpensslLib/process_files.pl", you find (with edk2 master being at commit 49693202ec9c):
>>
>>     49                  "./Configure",
>>     50                  "UEFI",
>>     51                  "no-afalgeng",
>>     52                  "no-asm",
>>     53                  "no-async",          <---- disables "async"
>>     54                  "no-autoalginit",
>>     55                  "no-autoerrinit",
>>     56                  "no-bf",
>>     57                  "no-blake2",
>>     58                  "no-camellia",
>>     59                  "no-capieng",
>>     60                  "no-cast",
>>     61                  "no-chacha",
>>     62                  "no-cms",            <---- disables "cms"
>>     63                  "no-ct",             <---- disables "ct"
>>     64                  "no-deprecated",
>>     65                  "no-dgram",
>>     66                  "no-dsa",
>>     67                  "no-dynamic-engine",
>>     68                  "no-ec",
>>     69                  "no-ec2m",
>>     70                  "no-engine",
>>     71                  "no-err",
>>     72                  "no-filenames",
>>     73                  "no-gost",
>>     74                  "no-hw",
>>     75                  "no-idea",
>>     76                  "no-mdc2",
>>     77                  "no-pic",
>>     78                  "no-ocb",
>>     79                  "no-poly1305",
>>     80                  "no-posix-io",
>>     81                  "no-rc2",
>>     82                  "no-rfc3779",
>>     83                  "no-rmd160",
>>     84                  "no-scrypt",
>>     85                  "no-seed",
>>     86                  "no-sock",
>>     87                  "no-srp",            <---- disables "srp"
>>     88                  "no-ssl",
>>     89                  "no-stdio",
>>     90                  "no-threads",
>>     91                  "no-ts",             <---- disables "ts"
>>     92                  "no-ui",
>>     93                  "no-whirlpool"
>>
>> (1) Therefore, the right thing to do here is to add "no-store" to the above list, in my opinion. Can you try that, please?
>>
>> And, this change should be a standalone patch, similarly to patch v2 1/6 in this series.
>>
>>> But UEFI don't use them. So exclude these files.
>>
>>> This file, crypto/rand/randfile.c, have been modified between
>>> OpenSSL_1_1_0j(74f2d9c1ec5f5) and OpenSSL_1_1_1b(50eaac9f33376672).
>>> It requires more crt runtime support. But UEFI don't use it.
>>> So exclude the file.
>>
>> I think I disagree with this approach.
>>
>> In OpenSSL commit fb4844bbc62f -- "Add UEFI flag for rand build", 
>> 2015-09-03, part of OpenSSL_1_1_0 --, Qin Long customized 
>> "crypto/rand/rand_unix.c". So that, when OPENSSL_SYS_UEFI was 
>> #defined, the real RAND_poll() function was replaced by a stub that 
>> would always report failure. (So this was a safe stub.)
>>
>> In OpenSSL commit 8389ec4b4950 -- "Add --with-rand-seed", 2017-07-22 --, the feature test itself has been reworked (see the previous patch in this series). However, it remains the case that "rand_unix.c" consumes and honors the OPENSSL_SYS_UEFI macro.
>>
>> So, let's check the "randfile.c" file. It defines three functions:
>> - RAND_load_file
>> - RAND_write_file
>> - RAND_file_name
>>
>> Nothing inside the OpenSSL library calls them (they exist purely for client code), and nothing in edk2 calls them either.
>>
>> (2a) Therefore, we should modify the "randfile.c" source file, with an upstream OpenSSL contribution, to hide the function definitions, when OPENSSL_SYS_UEFI is defined. In other words, continue with Qin Long's approach from commit fb4844bbc62f.
>>
>> (2b) Alternatively, I'm noticing that "rand" is just another module (similar to "store", see above). Assuming we really don't need RAND_* functions for anything in edk2: have we tried configuring OpenSSL, for the edk2 build, with the "no-rand" parameter?
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Ting Ye <ting.ye@intel.com>
>>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
>>> ---
>>>  CryptoPkg/Library/OpensslLib/process_files.pl | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl
>>> b/CryptoPkg/Library/OpensslLib/process_files.pl
>>> index 6c136cc..e277108 100755
>>> --- a/CryptoPkg/Library/OpensslLib/process_files.pl
>>> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl
>>> @@ -127,6 +127,12 @@ foreach my $product ((@{$unified_info{libraries}},
>>>          foreach my $s (@{$unified_info{sources}->{$o}}) {
>>>              next if ($unified_info{generate}->{$s});
>>>              next if $s =~ "crypto/bio/b_print.c";
>>> +
>>> +            # No need to add unused files in UEFI.
>>> +            # So it can reduce porting time, compile time, library size.
>>> +            next if $s =~ "crypto/rand/randfile.c";
>>> +            next if $s =~ "crypto/store/";
>>> +
>>>              if ($product =~ "libssl") {
>>>                  push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
>>>                  next;
>>>
>>
>>
>>
>>
> 
> 
> 
>