Make EC test programs only depend on softoken libs

RESOLVED FIXED in 3.27

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ueno, Assigned: ueno)

Tracking

trunk
3.27

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8779644 [details] [diff] [review]
ectest-softoken.patch

The EC test programs (ecperf and ectest) are currently using functions
from NSS libs, while they substantially rely on the softoken libs
only.  The attached patch elliminates the extra dependencies and allows those
programs to build with NSS_BUILD_SOFTOKEN_ONLY=1.  That also makes
downstream packaging simpler, if they split the upstream tarball into
multiple stages, like in Fedora.

To test, I did the following:
- remove external_tests from the toplevel manifest.mn
- make nss_build_all NSS_BUILD_SOFTOKEN_ONLY=1 BUILD_OPT=1 USE_64=1
- cd tests
- NSS_CYCLES=standard NSS_TESTS=ec NSS_SSL_TESTS=' ' NSS_SSL_RUN=' ' USE_64=1 FREEBL_NO_DEPEND=1 BUILD_OPT=1 NSS_BUILD_SOFTOKEN_ONLY=1 NSS_ENABLE_ECC=1 ./all.sh
Attachment #8779644 - Flags: review?(franziskuskiefer)

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

2 years ago
Assignee: nobody → dueno
Comment on attachment 8779644 [details] [diff] [review]
ectest-softoken.patch

Review of attachment 8779644 [details] [diff] [review]:
-----------------------------------------------------------------

While I'm in general ok with this change it doesn't work as is.
You probably didn't notice that due to a faulty test script (which is my fault).

To fix it you have to initialise the PRNG for the ecperf tests. That can be achieved by calling RNG_RNGInit and RNG_SystemInfoForRNG.
In order to fix the test script and catch the assertion error that's thrown with your patch (due to the uninitialised prng) you could change ecperf.sh to |ECPERF_OUT=$(ecperf 2>& 1)| and then parse for an assertion failure in addition to failure |PARSED_ECPERF_OUT+=`echo $ECPERF_OUT | grep -i "Assertion failure"`|. (I'm not sure why I get a 0 exit code in the script when the assertion is thrown.)
Attachment #8779644 - Flags: review?(franziskuskiefer) → feedback+
(Assignee)

Comment 2

2 years ago
Created attachment 8780000 [details] [diff] [review]
Make EC test programs only depend on softoken libs

The EC test programs (ecperf and ectest) are currently using functions
from NSS libs, while they substantially rely on the softoken libs
only.  This patch elliminates the extra dependencies and allows those
programs to build with NSS_BUILD_SOFTOKEN_ONLY=1.  That also makes
downstream packaging simpler, if they split the upstream tarball into
multiple stages, like in Fedora.
(Assignee)

Comment 3

2 years ago
Thank you for the review.  I have confirmed that the modified test script fails if the PRNG is not properly initialized.  The v2 patch should fix the issue.
(Assignee)

Updated

2 years ago
Attachment #8779644 - Attachment is obsolete: true
Comment on attachment 8780000 [details] [diff] [review]
Make EC test programs only depend on softoken libs

Review of attachment 8780000 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those two things fixed.

A try run https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=479b09bf5ebedc4947fc5f0c8ca12444d8037158

::: cmd/ecperf/ecperf.c
@@ +774,4 @@
>  #endif
>  
>  cleanup:
> +    rv |= SECOID_Shutdown();

We should shutdown the the RNG as well (RNG_RNGShutdown).

::: tests/ec/ectest.sh
@@ +40,5 @@
>  }
>  
>  ectest_init
> +ECTEST_OUT=$(ectest 2>&1)
> +ECTEST_OUT=`echo $ECTEST_OUT | grep -i 'not okay\|Assertion failure'"`

stray "
Attachment #8780000 - Flags: review+
(Assignee)

Comment 5

2 years ago
Created attachment 8780138 [details] [diff] [review]
Make EC test programs only depend on softoken libs

The EC test programs (ecperf and ectest) are currently using functions
from NSS libs, while they substantially rely on the softoken libs
only.  This patch elliminates the extra dependencies and allows those
programs to build with NSS_BUILD_SOFTOKEN_ONLY=1.  That also makes
downstream packaging simpler, if they split the upstream tarball into
multiple stages, like in Fedora.
(Assignee)

Updated

2 years ago
Attachment #8780000 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Thank you for pointing those out.  I am attaching the fixed version for reference, which has been incorporated into Fedora builds now:
http://koji.fedoraproject.org/koji/buildinfo?buildID=790901
Looks like Daiki has made the changes that Franziskus asked for, and this is good to be checked in.
Franziskus, do you want to check and land, or should I?
https://hg.mozilla.org/projects/nss/rev/df61b15b2cca
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.27
You need to log in before you can comment on or make changes to this bug.