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

["Xiaoyu lu" <xiaoyux.lu@intel.com>]

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?

Thanks,
Xiaoyu

-----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;
>>
> 
> 
> 
>