Closed Bug 1222665 Opened 6 years ago Closed 6 years ago

support for gtests for libssl internal interfaces

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(3 files, 9 obsolete files)

5.34 KB, patch
kmckinley
: feedback+
Details | Diff | Splinter Review
1.50 KB, patch
wtc
: review+
Details | Diff | Splinter Review
666 bytes, patch
Details | Diff | Splinter Review
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.
Attachment #8684513 - Flags: feedback?(kmckinley)
Kate, do you know of a good gtest to really test this, or a more substantial test that Jed could write?
Flags: needinfo?(kmckinley)
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.
Attachment #8684513 - Flags: feedback?(kmckinley) → feedback+
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?
Flags: needinfo?(kmckinley)
(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.)
Flags: needinfo?(kmckinley)
Attached patch Patch: some less-trivial tests. (obsolete) — Splinter Review
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.)
Flags: needinfo?(kmckinley)
Attachment #8691558 - Flags: review?(kmckinley)
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?
Flags: needinfo?(jld)
Comment on attachment 8691558 [details] [diff] [review]
Patch: some less-trivial tests.

lgtm
Flags: needinfo?(jld)
Attachment #8691558 - Flags: review?(kmckinley) → review+
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.
It's been suggested that Kai might be a better reviewer….
Flags: needinfo?(kaie)
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/
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 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
Attachment #8694491 - Flags: review?(wtc) → review-
(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.
Attachment #8694491 - Attachment is obsolete: true
Current work in progress.  Not quite done yet (there's a comment or two that's basically a FIXME).
Attachment #8691558 - Attachment is obsolete: true
See Also: → 1243208
Flags: needinfo?(kaie)
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.
Attachment #8713275 - Flags: review?(wtc)
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.
Attachment #8713319 - Attachment description: Part 2: Add simple tests for a few libssl internal interfaces. [v2] → Part 2: Add simple tests for a few libssl internal interfaces. [v3]
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.
Attachment #8713275 - Flags: review?(wtc)
Jed, Wan-Teh, this static linkage patch is going to block TLS 1.3. Can we tr to bring it to a conclusion?
Flags: needinfo?(wtc)
Flags: needinfo?(jld)
Attachment #8708638 - Attachment is obsolete: true
Attachment #8713275 - Attachment is obsolete: true
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.)
Attachment #8708639 - Attachment is obsolete: true
Flags: needinfo?(jld)
Attachment #8713339 - Flags: review?(wtc)
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.
Flags: needinfo?(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 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" :-)
Attachment #8713339 - Flags: review?(wtc) → review+
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
Attachment #8714184 - Flags: review?(martin.thomson)
Attached patch bug1222665.patch (obsolete) — Splinter Review
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.
Attachment #8714195 - Flags: review?(ekr)
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.
Attachment #8714184 - Flags: review?(martin.thomson)
(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 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 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
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.
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.
I already have r+ from WTC. I am going to land the r+ed patch soon
Jed, I updated and landed your patch as: https://hg.mozilla.org/projects/nss/rev/d6ecb8117a43
Now that the fallout from the build changes seems to have settled, I've uploaded the updated tests to https://codereview.appspot.com/279190043
Flags: needinfo?(wtc)
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.
Attachment #8713319 - Attachment is obsolete: true
Attachment #8722317 - Flags: review?(ekr)
See Also: → 1256518
I'm moving the patch with the actual tests to a new bug, to clarify what's done and what's not.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: gtests for libssl internal interfaces → support for gtests for libssl internal interfaces
Attachment #8714195 - Attachment is obsolete: true
Attachment #8714195 - Flags: review?(ekr)
Attachment #8722317 - Attachment is obsolete: true
Attachment #8722317 - Flags: review?(ekr)
No longer blocks: 1243238
You need to log in before you can comment on or make changes to this bug.