1335284 - Enable CHECK_FORK_PTHREAD on FreeBSD

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Conrad Meyer]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

Firefox on FreeBSD shows *heavy* use of getpid() syscall, thousands per second in some cases.  Profiling shows that it comes from libnss3.so`PK11_DigestFinal+0xf6.  A little digging for getpid in the NSS sources locates this in softtoken.h:

=================================8<=================================
#if !defined(CHECK_FORK_MIXED) && !defined(CHECK_FORK_PTHREAD) && \
    !defined(CHECK_FORK_GETPID)

/* Choose fork check method automatically unless specified
 * This section should be updated as more platforms get pthread fixes
 * to unregister fork handlers in dlclose.
 */

#ifdef SOLARIS

/* Solaris 8, s9 use PID checks, s10 uses pthread_atfork */

#define CHECK_FORK_MIXED

#elif defined(LINUX) || defined(__GLIBC__)

#define CHECK_FORK_PTHREAD

#else

/* Other Unix platforms use only PID checks. Even if pthread_atfork is
 * available, the behavior of dlclose isn't guaranteed by POSIX to
 * unregister the fork handler. */

#define CHECK_FORK_GETPID

#endif
=================================8<=================================

FreeBSD supports pthread_atfork() and unregisters unloaded callbacks at dlclose().  This support was added in FreeBSD r211706, which is present in all supported FreeBSD releases (10.x and 11.x).  Please change the softtoken.h like so:

=================================8<=================================
-#elif defined(LINUX) || defined(__GLIBC__)
+#elif defined(LINUX) || defined(__GLIBC__) || defined(__FreeBSD__)
 
 #define CHECK_FORK_PTHREAD
=================================8<=================================

So that the less costly CHECK_FORK_PTHREAD is used on FreeBSD as well as Glibc platforms.

Comment 1

[Landry Breuil (:gaston)]

Might be relevant for OpenBSD and NetBSD too ?

Comment 2

[Conrad Meyer]

(In reply to Landry Breuil (:gaston) from comment #1)
> Might be relevant for OpenBSD and NetBSD too ?

Depends.  Do OpenBSD/NetBSD have pthread_atfork(), and does it unregister callbacks on dlclose()?

Comment 3

[Martin Husemann]

On NetBSD unloading the dso does not automagically deregister handlers. I have to say this is one of the worst hacks I've ever seen - instead of creating a deregister function and calling that from a destructor.

We are discussing what to do about this, thanks for pinging.

Comment 4

[Martin Husemann]

Another (maybe stupid) question: would it be ok to ignore all the pthread_atfork() stuff if we only ever use posix_spawn() in firefox?

Comment 5

[Conrad Meyer]

(In reply to Martin Husemann from comment #4)
> Another (maybe stupid) question: would it be ok to ignore all the
> pthread_atfork() stuff if we only ever use posix_spawn() in firefox?

I think libnss is a library that can't make the assumption that the consumer will never use fork(), or you could remove all of this fork handling logic entirely.

Comment 6

[Jan Beich]

Attachment: v1

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=93848c474e3a

Comment 7

[Landry Breuil (:gaston)]

Comment on attachment 8841447 [details] [diff] [review]
v1

Only built-tested...

Comment 8

[Robert Relyea]

Comment on attachment 8841447 [details] [diff] [review]
v1

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

r+ rrelyea as long a I can get a freebsd and openssl user to run the NSS test suite on their platform to make sure nothing breaks.

bob

Comment 9

[Conrad Meyer]

(In reply to Robert Relyea from comment #8)
> r+ rrelyea as long a I can get a freebsd and openssl user to run the NSS
> test suite on their platform to make sure nothing breaks.

Hi Bob, please point me to documentation for running the test suite and I will be happy to do so (FreeBSD).

Comment 10

[Jan Beich]

See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Building#Unit_Testing
or  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Sources_Building_Testing#Running_the_NSS_test_suite
or  http://www-archive.mozilla.org/projects/security/pki/nss/testnss_32.html

Downstream way:

  $ cd /usr/ports/security/nss
  $ make clean all test
  [...]
  Tests summary:
  --------------
  Passed:             12372
  Failed:             0
  Failed with core:   0
  ASan failures:      0
  Unknown status:     16 (gtest disabled)
  TinderboxPrint:Unknown: 16

Upstream way (GYP):

  $ pkg install binutils # lld and nm (elftoolchain) aren't supported yet
  $ export COMPILER_PATH=${LOCALBASE=/usr/local}/bin PATH=$LOCALBASE/bin:$PATH 

  $ pkg install bash gmake mercurial ninja perl5 py27-gyp python
  $ hg clone https://hg.mozilla.org/projects/nspr
  $ hg clone https://hg.mozilla.org/projects/nss
  $ cd nss
  $ hg up -rc188b719789d # bug 1346602 workaround
  $ hg import 'https://bugzilla.mozilla.org/attachment.cgi?id=8841447'
  $ bash ./build.sh -c
  $ (cd tests; HOST=localhost DOMSUF=localdomain sh ./all.sh)
  [...]
  Tests summary:
  --------------
  Passed:             28096
  Failed:             626
  Failed with core:   0
  ASan failures:      0
  Unknown status:     0

Comment 11

[Jan Beich]

Landry, can you run tests on OpenBSD before and after applying the patch here?

Comment 12

[Jan Beich]

(In reply to Jan Beich from comment #10)
>   Failed:             626

*Sigh* I did test inside jail.

Comment 13

[Landry Breuil (:gaston)]

trying to follow what you use in comment 10, bash build.sh -c doesnt seem to build many things, because gyp is too old (cf 1311619#c25).

[15:21] c64:~/src/nss/ $bash ./build.sh -c                                                                                                                    
NSPR [1/3] configure ...
NSPR [2/3] make ...
NSPR [3/3] install ...
[15:24] c64:~/src/nss/ $./build.sh -v 
run_scanbuild gyp -f ninja --depth=/home/landry/src/nss --generator-output=. -Dnss_dist_dir=/home/landry/src/dist -Dnss_dist_obj_dir=/home/landry/src/dist/Debu
g -Dnspr_lib_dir=/home/landry/src/dist/Debug/lib -Dnspr_include_dir=/home/landry/src/dist/Debug/include/nspr /home/landry/src/nss/nss.gyp
gyp: target_conditions _type=="shared_library" must be length 2 or 3, not 6

Stupid gyp, i hate you.

in ../dist, i only have the nspr binaries/headers; nothing from nss.

Using gmake, build fails:

'
cc -o OpenBSD6.0_DBG.OBJ/quickder.o -c -g -fPIC -DPIC  -Wall -Wno-switch -pipe -DOPENBSD -Wall -DNSS_NO_GCC48 -DXP_UNIX -DDEBUG -UNDEBUG -DDEBUG_landry -pthread -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/OpenBSD6.0_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  quickder.c                   
In file included from quickder.c:10:                                          
secasn1.h:15:21: error: plarena.h: No such file or directory

Comment 14

[Landry Breuil (:gaston)]

Dammit. If i update gyp, it still fails on freebl3.poly1305-donna-x64-sse2-incremental-source: that stuff is still broken.

../../lib/freebl/poly1305-donna-x64-sse2-incremental-source.c:63: error: 'uint128_t' undeclared (first use in this function)

Looking again at what we did in bug #1311619, that's too much sh** to handle for me now.

Comment 15

[Landry Breuil (:gaston)]

And it finally builds with gyp from 20161117, and forcing clang:

$env CC=clang CXX=clang++ bash ./build.sh -v

will run the tests now.

Comment 16

[Conrad Meyer]

(In reply to Jan Beich from comment #10)
>   Tests summary:
>   --------------
>   Passed:             12372
>   Failed:             0
>   Failed with core:   0
>   ASan failures:      0
>   Unknown status:     16 (gtest disabled)
>   TinderboxPrint:Unknown: 16

Thanks Jan.

Bob, I'm assuming this is sufficient testing from the FreeBSD side of things -- just let us know if you want something more from us.  Thanks!

Comment 17

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #15)
> will run the tests now.

If you see too many false positives try taking advantage of do-test target in the OpenBSD port.

Comment 18

[Landry Breuil (:gaston)]

With and without the patch to enable pthread_atfork(), the test results are the same:

Tests summary:
--------------
Passed:             32138
Failed:             626
Failed with core:   0
ASan failures:      0
Unknown status:     0

Comment 19

[Jan Beich]

Robert, anything else to do? If not, please, land it in the repo.

(In reply to Landry Breuil (:gaston) from comment #18)
> Failed:             626

Are all of those due to certificate verification? If the failures disappear in downstream builds then GYP on BSDs need more work.

$ fgrep FAILED ../tests_results/security/localhost.1/output.log | random 626 | head -1
chains.sh: #26154: Extension2: Verifying certificate(s)  User2CA2.der with flags -d AllDB -pp      -o OID.1.0  -t CA1 - FAILED

Comment 20

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #19)
> Robert, anything else to do? If not, please, land it in the repo.
> 
> (In reply to Landry Breuil (:gaston) from comment #18)
> > Failed:             626
> 
> Are all of those due to certificate verification? If the failures disappear
> in downstream builds then GYP on BSDs need more work.
> 
> $ fgrep FAILED ../tests_results/security/localhost.1/output.log | random 626
> | head -1
> chains.sh: #26154: Extension2: Verifying certificate(s)  User2CA2.der with
> flags -d AllDB -pp      -o OID.1.0  -t CA1 - FAILED

[22:08] c64:~/src/nss/ $grep FAILED tests-patch.log |grep -v Verif 
fips.sh: #840: Mangle libsoftokn3.so.1.0 - FAILED
fips.sh: #10215: Mangle libsoftokn3.so.1.0 - FAILED
fips.sh: #18148: Mangle libsoftokn3.so.1.0 - FAILED
fips.sh: #24945: Mangle libsoftokn3.so.1.0 - FAILED

All the other failures are certs verification failures, and i suppose the 4 libsoftkn3 mangling failure are unsurprising...

Comment 21

[Franziskus Kiefer [:franziskus]]

(In reply to Jan Beich from comment #19)
> Robert, anything else to do? If not, please, land it in the repo.
> 
> (In reply to Landry Breuil (:gaston) from comment #18)
> > Failed:             626
> 
> Are all of those due to certificate verification? If the failures disappear
> in downstream builds then GYP on BSDs need more work.
> 
> $ fgrep FAILED ../tests_results/security/localhost.1/output.log | random 626
> | head -1
> chains.sh: #26154: Extension2: Verifying certificate(s)  User2CA2.der with
> flags -d AllDB -pp      -o OID.1.0  -t CA1 - FAILED

Gyp doesn't build libpkix by default [1]. Hence those tests won't work. (We currently don't plan to add libpkix to the gyp build unless someone needs it. It's deprecated.)

[1] https://searchfox.org/nss/rev/cc62cf3e09337924461518b6bf664a6df7a38837/coreconf/config.gypi#94

Comment 22

[Jan Beich]

Since needinfo was cancelled, I guess, this is ready to land.

Comment 23

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/e77d9a39893e4e19fb7bd6154048b407fe6fcbe2