1222665 - support for gtests for libssl internal interfaces

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Proof of concept: sslBuffer tests.

We'd like to have gtests that access NSS internal interfaces.  The test plans aren't finalized yet, but for now I have a proof of concept patch that adds some (probably overengineered) tests for the internal sslBuffer abstraction in libssl and applies static linking to ssl_gtest so that that can work.  I don't know if we'd want to commit those; if not, this bug can be used for more substantial tests later.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Kate, do you know of a good gtest to really test this, or a more substantial test that Jed could write?

Comment 2

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8684513 [details] [diff] [review]
Proof of concept: sslBuffer tests.

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

This is fine as a proof of concept.

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Here are some potential possibilities for other tests to try:

* Locking / mutex on sockets - what happens if you have a "double unlock"
* ssl3_GetKeyPairRef / ssl3_FreeKeyPair 	
* static SECStatus ssl3_GetNewRandom (SSL3Random * random) uses ssl_Time() - can you get the same random twice?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Kate McKinley [:kmckinley] from comment #3)
> * ssl3_GetKeyPairRef / ssl3_FreeKeyPair 	

What about them?  I can try to check that they don't lose updates when called on multiple threads.  I could also overflow the refcount, but that's not very useful as a test.

> * static SECStatus ssl3_GetNewRandom (SSL3Random * random) uses ssl_Time() -
> can you get the same random twice?

It looks like that call to ssl_Time() went away in NSS 3.15.4 (bug 937976), and now it just passes through the PKCS#11 module's RNG.  I have a test that checks two random values for inequality, but that's not very interesting.  (Statistical tests of randomness could be useful, if there aren't any already, but that wouldn't go here, I don't think.)

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: some less-trivial tests.

Tests here:
1. Two successive values from ssl3_GetNewRandom aren't equal.
2a. Double-unlock causes an assertion failure on debug builds.
2b. Double-unlock is ignored on non-debug builds (lock/unlock afterwards still works).
3. Key pair refcounts don't lose updates when used in parallel.  (At least for inc/inc and dec/dec conflicts.  Also, I haven't checked that this actually fails when used with non-atomic refcounts.)

Comment 6

[Eric Rescorla (:ekr)]

It would be great to land some test, any test, here, that accesses the internal interfaces. It doesn't need to test substantial functionality, just demonstrate that linking works. That way it won't be in my way when I want to land some of the internal HKDF tests. 

Jed/Kate: can we do that next week?

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8691558 [details] [diff] [review]
Patch: some less-trivial tests.

lgtm

Comment 8

[Eric Rescorla (:ekr)]

Kate, Jed,

This whole patch needs review from an NSS peer and the build change
needs review from someone who know that area. I suggest Wan-Teh.
So, can you please:

1. Split this into two patches, one that is just the build change
and one that is the new tests.
2. Put the build change up here (since it's trivial).
3. Put the other pach up on Rietveld.

Thanks.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 1: Just the build config change

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The rest of the patch, on Rietveld: https://codereview.appspot.com/279190043

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

It's been suggested that Kai might be a better reviewer….

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 8691558 [details] [diff] [review]
Patch: some less-trivial tests.

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

Comments up at:
https://codereview.appspot.com/279190043/

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Further annoyance, and I'm not sure why I didn't run into this sooner: the death test's assertion failure causes a core dump, which the test script detects and (not unreasonably) considers a failure.  This is easy enough to fix by temporarily setting the soft limit to 0, but I probably want to put the code for it in external_tests/common/.

Comment 14

[Wan-Teh Chang]

Comment on attachment 8694491 [details] [diff] [review]
Step 1: Just the build config change

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

Hi Jed:

I'm sorry I missed this code review request.

I think it's a bad idea for external_tests/ssl_gtest/Makefile
to include ../../cmd/platlibs.mk.

I suggest two small changes to the zlib change.

Also, I think it's a bad idea for
external_tests/ssl_gtest/Makefile to include
../../cmd/platlibs.mk.

::: external_tests/ssl_gtest/Makefile
@@ +24,5 @@
>  #######################################################################
>  # (4) Include "local" platform-dependent assignments (OPTIONAL).      #
>  #######################################################################
>  
>  include ../../cmd/platlibs.mk

Hmm... this makefile is outside nss/cmd but it is using
nss/cmd/platlibs.mk. That seems bad.

@@ +29,5 @@
>  
> +ifdef USE_SYSTEM_ZLIB
> +OS_LIBS += $(ZLIB_LIBS)
> +else
> +EXTRA_LIBS += $(ZLIB_LIBS)

Why does ssl_gtest need to link with zlib? I guess this
comes from libssl?  If so, this should match the code
in nss/lib/ssl/config.mk and be inside an ifdef NSS_ENABLE_ZLIB
block. And we should move the NSS_ENABLE_ZLIB = 1 statement
to some central makefile in nss/coreconf, such as nss/coreconf/config.mk
as follows:

diff --git a/coreconf/config.mk b/coreconf/config.mk
--- a/coreconf/config.mk
+++ b/coreconf/config.mk
@@ -149,16 +149,22 @@ endif
 ifdef NSS_DISABLE_ECC
 DEFINES += -DNSS_DISABLE_ECC
 endif
  
 ifdef NSS_ECC_MORE_THAN_SUITE_B
 DEFINES += -DNSS_ECC_MORE_THAN_SUITE_B
 endif
 
+# NSS_ENABLE_ZLIB controls the zlib compression feature in libssl.
+# Mozilla's mozilla/modules/zlib/src/zconf.h adds the MOZ_Z_ prefix to zlib
+# exported symbols, which causes problem when NSS is built as part of Mozilla.
+# So we add a NSS_ENABLE_ZLIB variable to allow Mozilla to turn this off.
+NSS_ENABLE_ZLIB = 1
+
 ifdef NSS_ALLOW_UNSUPPORTED_CRITICAL
 DEFINES += -DNSS_ALLOW_UNSUPPORTED_CRITICAL
 endif
 
 ifdef BUILD_LIBPKIX_TESTS
 DEFINES += -DBUILD_LIBPKIX_TESTS
 endif
 
diff --git a/lib/ssl/config.mk b/lib/ssl/config.mk
--- a/lib/ssl/config.mk
+++ b/lib/ssl/config.mk
@@ -67,20 +67,16 @@ EXTRA_SHARED_LIBS += \
 	$(NULL)
 
 ifeq ($(OS_ARCH), BeOS)
 EXTRA_SHARED_LIBS += -lbe
 endif
 
 endif
 
-# Mozilla's mozilla/modules/zlib/src/zconf.h adds the MOZ_Z_ prefix to zlib
-# exported symbols, which causes problem when NSS is built as part of Mozilla.
-# So we add a NSS_ENABLE_ZLIB variable to allow Mozilla to turn this off.
-NSS_ENABLE_ZLIB = 1
 ifdef NSS_ENABLE_ZLIB
 
 DEFINES += -DNSS_ENABLE_ZLIB
 
 # If a platform has a system zlib, set USE_SYSTEM_ZLIB to 1 and
 # ZLIB_LIBS to the linker command-line arguments for the system zlib
 # (for example, -lz) in the platform's config file in coreconf.
 ifdef USE_SYSTEM_ZLIB

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Wan-Teh Chang from comment #14)
> Also, I think it's a bad idea for
> external_tests/ssl_gtest/Makefile to include
> ../../cmd/platlibs.mk.

FYI, it no longer does that directly, because the include moved into external_tests/common/gtest.mk in bug 1221819.

> >  include ../../cmd/platlibs.mk
> 
> Hmm... this makefile is outside nss/cmd but it is using
> nss/cmd/platlibs.mk. That seems bad.

This is building an executable that uses NSS's libraries, so I'm not sure what else it should do, besides moving external_tests/ inside of cmd/ (which is complicated, because external_tests/google_test builds a library and external_tests/common might eventually do so as well).   But this isn't anything this patch is changing; it was like that before I saw this code.

> @@ +29,5 @@
> >  
> > +ifdef USE_SYSTEM_ZLIB
> > +OS_LIBS += $(ZLIB_LIBS)
> > +else
> > +EXTRA_LIBS += $(ZLIB_LIBS)
> 
> Why does ssl_gtest need to link with zlib? I guess this
> comes from libssl?

Yes, this is because libssl depends on zlib and there isn't a way to propagate that dependency when linking statically.  See also cmd/modutil/Makefile and cmd/signtool/Makefile, which do the same thing.

> If so, this should match the code
> in nss/lib/ssl/config.mk and be inside an ifdef NSS_ENABLE_ZLIB
> block. And we should move the NSS_ENABLE_ZLIB = 1 statement
> to some central makefile in nss/coreconf, such as nss/coreconf/config.mk
> as follows:
[...]

Thanks; that's a good idea.  But I wonder if it's worth moving this into coreconf/zlib.mk or something, given that there'd be 4 different copies of it after this patch.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 0: Remove unused items from libssl internal headers. [v2]

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 1: USE_STATIC_LIBS for ssl_gtest. [v2]

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 2: Add simple tests for a few libssl internal interfaces. [v2]

Current work in progress.  Not quite done yet (there's a comment or two that's basically a FIXME).

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 3: Clean up zlib makefile fragments.

This patch centralizes all the copies of the zlib-related makefile directives and makes them properly conditional.   I could revise history to move this before the other patches, but there doesn't seem to be any reason to do that.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 2: Add simple tests for a few libssl internal interfaces. [v3]

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8713319 [details] [diff] [review]
Part 2: Add simple tests for a few libssl internal interfaces. [v3]

The only change over the last version is a slight adjustment to one comment to note the followup bug.

Comment 22

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8713275 [details] [diff] [review]
Part 3: Clean up zlib makefile fragments.

On second thought, I'm going to swap this with part 1 after all; it's a little silly to add code in one patch and then immediately remove it.

Comment 23

[Eric Rescorla (:ekr)]

Jed, Wan-Teh, this static linkage patch is going to block TLS 1.3. Can we tr to bring it to a conclusion?

Comment 24

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 1: USE_STATIC_LIBS for ssl_gtest. [v3]

Depends on patch from bug 1243872.  (The existing makefile items that reach into ../../cmd are, as mentioned, not my change and out of scope for this bug.)

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Do I still need more review for Part 2?  Part 1 + bug 1243872 should be enough to unblock other things that want to add internal tests; I can split this bug more if that would help.

Comment 26

[Eric Rescorla (:ekr)]

Jed, yes, I think you need more review on part 2. I will start on that now.

Wan-Teh, are you happy with patch 1 and bug 1243872? These are blocking TLS 1.3 so can you either review or nominate someone else?

Comment 27

[Wan-Teh Chang]

Comment on attachment 8713339 [details] [diff] [review]
Part 1: USE_STATIC_LIBS for ssl_gtest. [v3]

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

r=wtc.

::: external_tests/ssl_gtest/Makefile
@@ +25,5 @@
>  # (4) Include "local" platform-dependent assignments (OPTIONAL).      #
>  #######################################################################
>  
>  include ../common/gtest.mk
> +include $(CORE_DEPTH)/coreconf/zlib.mk

It seems that this could also be in section (2). I am not sure
whether this should be considered "global" or "local" :-)

Comment 28

[Eric Rescorla (:ekr)]

Attachment: 0002-Bug-1222665-Statically-link-SSL-gtests.patch

Comment 29

[Eric Rescorla (:ekr)]

Comment on attachment 8714184 [details] [diff] [review]
0002-Bug-1222665-Statically-link-SSL-gtests.patch

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

MT, this lives on top of: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1243872&attachment=8714185

Comment 30

[Martin Thomson [:mt:]]

Attachment: bug1222665.patch

I think that it makes sense to make this more homogenous.  This makes the previously local NSS_ENABLE_ZLIB config apply more globally.  I couldn't find any way to disable this option by invoking make, so I switched the flag to an NSS_DISABLE_ZLIB one, which will mean that the existing gecko configuration will have to be tweaked some, though no more so than with ekr's version.

The two utilities that depend on zlib now don't get compiled if this is enabled.

One advantage of this is that ALL of the gtests are now statically linked.

Comment 31

[Martin Thomson [:mt:]]

Comment on attachment 8714184 [details] [diff] [review]
0002-Bug-1222665-Statically-link-SSL-gtests.patch

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

You should be able to add the USE_STATIC_LIBS line to .../external_tests/common/gtest.mk instead.

::: external_tests/ssl_gtest/manifest.mn
@@ +30,5 @@
>  
>  PROGRAM = ssl_gtest
>  EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX)
> +
> +USE_STATIC_LIBS = 1

Do you want to add this to ../common/gtest.mk instead?

I don't know if that works though, because it didn't work when I tried it.

`make USE_STATIC_LIBS=1` fails on my machine with undefined zlib symbols.

Comment 32

[Martin Thomson [:mt:]]

(In reply to Martin Thomson [:mt:] from comment #31)
> I don't know if that works though, because it didn't work when I tried it.
> 
> `make USE_STATIC_LIBS=1` fails on my machine with undefined zlib symbols.

Oops, ignore these, I only wanted to keep the first part of the comment.

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8714195 [details] [diff] [review]
bug1222665.patch

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

Hmm, this seems right.  ekr, this is intended to replace both of your patches.

Yes, it includes zlib outside of the places where it is needed (unless you set NSS_DISABLE_ZLIB).  That's OK because it isn't used when it isn't needed.

Comment 34

[Eric Rescorla (:ekr)]

Comment on attachment 8714195 [details] [diff] [review]
bug1222665.patch

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

This doesn't seem to match the guidance I got from WTC, which is that
the command line utilities always use zlib and so they shouldn't be
conditional but that there should be a conditional to wrap whether
SSL uses zlib

Comment 35

[Eric Rescorla (:ekr)]

D(In reply to Martin Thomson [:mt:] from comment #31)
> Comment on attachment 8714184 [details] [diff] [review]
> 0002-Bug-1222665-Statically-link-SSL-gtests.patch
> 
> Review of attachment 8714184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should be able to add the USE_STATIC_LIBS line to
> .../external_tests/common/gtest.mk instead.
> 
> ::: external_tests/ssl_gtest/manifest.mn
> @@ +30,5 @@
> >  
> >  PROGRAM = ssl_gtest
> >  EXTRA_LIBS = $(DIST)/lib/$(LIB_PREFIX)gtest.$(LIB_SUFFIX)
> > +
> > +USE_STATIC_LIBS = 1
> 
> Do you want to add this to ../common/gtest.mk instead?

No, because the non-SSL tests don't need zlib


> 
> I don't know if that works though, because it didn't work when I tried it.
> 
> `make USE_STATIC_LIBS=1` fails on my machine with undefined zlib symbols.


You need both patches.

Comment 36

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

If we just want to get ekr's TLS 1.3 stuff unblocked, we could resurrect attachment 8708639 [details] [diff] [review] — which isn't quite right, but it's the same thing that other test executables have been doing for years — and land it with the understanding that cleanups of the zlib stuff will follow.

Comment 37

[Eric Rescorla (:ekr)]

I already have r+ from WTC. I am going to land the r+ed patch soon

Comment 38

[Eric Rescorla (:ekr)]

Jed, I updated and landed your patch as: https://hg.mozilla.org/projects/nss/rev/d6ecb8117a43

Comment 39

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Now that the fallout from the build changes seems to have settled, I've uploaded the updated tests to https://codereview.appspot.com/279190043

Comment 40

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Part 2: Add simple tests for a few libssl internal interfaces. [v3.1]

Essentially the same as the last patch, but this should be the same base rev as the Rietveld upload mentioned in comment #39.  And, as mentioned in comment #26, this needs more review.

Comment 41

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'm moving the patch with the actual tests to a new bug, to clarify what's done and what's not.