Issue 279190043: Add simple tests for a few libssl internal interfaces.
|
Created:
8 years, 4 months ago by jld Modified:
7 years, 10 months ago Reviewers:
ekr-rietveld Visibility:
Public. |
DescriptionAdd simple tests for a few libssl internal interfaces.
Patch Set 1 #
Total comments: 31
Patch Set 2 : Add simple tests for a few libssl internal interfaces. #Patch Set 3 : Add simple tests for a few libssl internal interfaces. [v3] #
Total comments: 33
Patch Set 4 : Add simple tests for a few libssl internal interfaces. [v4] #
MessagesTotal messages: 5
https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:15: #undef explicit In other code, we dealt with this by having C accessors. Not sure how excited I am about this version. Would be better, I think to fix the explicit/implicit inside sslimpl.h https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:25: #include <string.h> Standard C/C++ includes should come first. See the Google style guide https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:34: #endif // GTEST_HAS_DEATH_TEST When would/wouldn't we have DEATH tests? Would prefer to avoid a conditional here. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:46: } TearDown is automatically called as part of the fixture, so you shouldn'tbe calling it here. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:47: Please follow the conventional ordering here: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:50: EXPECT_TRUE(fd_); ASSERT_NE(nullptr) https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:60: ss_ = nullptr; Why are you resetting these? In any case, the ss_ assignment should not be in the conditional since there is no local logic that enforces that. Also, you can just do this in the dtor. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:69: InternalKeyPairTest() : keys_() { } keys_(nullptr). https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:71: TearDown(); Don't call TearDown here. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:82: /* len */ 0)); C++-style comments here and below please. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:83: EXPECT_TRUE(ecParams); You need an assert here because you dereference ecParams in a minute. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:94: } Why is this braced? I don't see any RAII stuff here. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:105: keys_ = nullptr; Why reset this to null? For that matter why not just do this in the dtor? https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:136: } All this machinery seems really complicated. I would just implement C wrapper functions. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:177: #endif See above comments about conditionals here.
Sign in to reply to this message.
https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:15: #undef explicit On 2015/12/31 17:54:50, ekr-webrtc wrote: > In other code, we dealt with this by having C accessors. > > Not sure how excited I am about this version. Would be better, I think to fix > the explicit/implicit inside sslimpl.h So I looked into this a little more, and those declarations are unused; I can just delete them instead. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:25: #include <string.h> On 2015/12/31 17:54:51, ekr-webrtc wrote: > Standard C/C++ includes should come first. See the Google style guide > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:34: #endif // GTEST_HAS_DEATH_TEST On 2015/12/31 17:54:51, ekr-webrtc wrote: > When would/wouldn't we have DEATH tests? Would prefer to avoid a conditional > here. Support for death tests varies by OS; e.g., the version of gtest currently in the NSS tree doesn't support them on FreeBSD (although newer upstream does). But there is an ASSERT_DEATH_IF_SUPPORTED macro (which emits a warning at runtime if it's not supported, and compiles but doesn't run the code in question), so I can use that. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:46: } On 2015/12/31 17:54:50, ekr-webrtc wrote: > TearDown is automatically called as part of the fixture, so you shouldn'tbe > calling it here. Do I need to care about releasing resources in the case of an exception during the test? If not, then I agree. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:47: On 2015/12/31 17:54:51, ekr-webrtc wrote: > Please follow the conventional ordering here: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:50: EXPECT_TRUE(fd_); On 2015/12/31 17:54:50, ekr-webrtc wrote: > ASSERT_NE(nullptr) Done. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:60: ss_ = nullptr; On 2015/12/31 17:54:51, ekr-webrtc wrote: > Why are you resetting these? In any case, the ss_ assignment should not be in > the conditional since there is no local logic that enforces that. > > Also, you can just do this in the dtor. It's been a while since I wrote this, but I think the reason I did that is that I thought it might be meaningful for any side-effects of closing the fd to occur in TearDown vs. the dtor? If there's no reason why that would matter then I can just use the dtor and delete TearDown (and the comment above about exceptions doesn't apply). As for ss_, not that it matters anymore: if I remember how all of this works, the PR_Close calls into ssl_*Close and makes that sslSocket* a dangling pointer, so clearing it was to reflect that. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:69: InternalKeyPairTest() : keys_() { } On 2015/12/31 17:54:51, ekr-webrtc wrote: > keys_(nullptr). Done. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:71: TearDown(); On 2015/12/31 17:54:51, ekr-webrtc wrote: > Don't call TearDown here. As above. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:82: /* len */ 0)); On 2015/12/31 17:54:51, ekr-webrtc wrote: > C++-style comments here and below please. Done. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:83: EXPECT_TRUE(ecParams); On 2015/12/31 17:54:51, ekr-webrtc wrote: > You need an assert here because you dereference ecParams in a minute. Done. Also some of the other EXPECTs that have that property. (I think that started because some of them were originally in ctors/dtors, where ASSERT_* can't be used. But that's not the case here.) https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:94: } On 2015/12/31 17:54:50, ekr-webrtc wrote: > Why is this braced? I don't see any RAII stuff here. Limiting the scope of tmpPubKey to reflect that it shouldn't be used after pubKey takes ownership. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:105: keys_ = nullptr; On 2015/12/31 17:54:51, ekr-webrtc wrote: > Why reset this to null? For that matter why not just do this in the dtor? (As above.) https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:136: } On 2015/12/31 17:54:50, ekr-webrtc wrote: > All this machinery seems really complicated. I would just implement C wrapper > functions. I'm not sure what you're suggesting or in what way it would help. The code above is not very complicated — most of the work went into figuring out the PR_CreateThread parameters. https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:177: #endif On 2015/12/31 17:54:51, ekr-webrtc wrote: > See above comments about conditionals here. Acknowledged.
Sign in to reply to this message.
Jed, please provide a revised patch here as well as on Bugzilla.
Sign in to reply to this message.
new https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_... external_tests/ssl_gtest/ssl_internal_unittest.cc:46: } On 2016/01/13 01:15:54, jld wrote: > On 2015/12/31 17:54:50, ekr-webrtc wrote: > > TearDown is automatically called as part of the fixture, so you shouldn'tbe > > calling it here. > > Do I need to care about releasing resources in the case of an exception during > the test? If not, then I agree. I wouldn't. https://codereview.appspot.com/279190043/diff/40001/external_tests/common/sco... File external_tests/common/scoped_ptrs.h (right): https://codereview.appspot.com/279190043/diff/40001/external_tests/common/sco... external_tests/common/scoped_ptrs.h:11: #include "pk11pub.h" This is the only change to this file, which seems suspicious. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:12: #include <memory> // for unique_ptr Are you sure this is going to compile on all our builders? https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:23: } Are you sure extern "C" is needed here? See: https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/seccomon... https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:28: // FIXME, bug 1243238: move this into a common location. Convention here is either TODO(<email>) https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:34: #ifdef XP_UNIX Where does XP_UNIX come from? https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:36: #endif This seems like an odd indirection. Why don't you just put the conditional in the place it's used? Even if you don't do that, you can just conditionalize these declarations inline since C++ will autogenerate the ctor and dtor. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:41: _saved_limit.rlim_cur = _saved_limit.rlim_max = RLIM_INFINITY; It seems odd to set this before you call getrlimit(), which presumably overwrites it. Does this actually work and if so, why? https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:63: #endif // DEBUG If you just conditionalize the SuppressCoreDump on XP_UNIX && DEBUG, then you can remove this #ifdef. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:71: } Instead why don't you add ScopedFd https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:93: } ScopedKeyPair https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:100: ScopedSECKEYPublicKey pubKey; This isn't C, define right before use. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:117: } Why are you wrapping this in {}? Just seems confusing. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:137: RunOnThreads(size_t n, const F& func) Please give n a sensible name. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:156: Please defint hs right before use. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:176: // Run this twice -- on non-debug builds, an excess unlock is ignored. Why twice? This comment is not clear. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:186: for (int i = 0; i < 2; ++i) { Nor this one. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:215: ASSERT_EQ(1 + numThreads * iterations, size_t(keys_->refCount)); please don't cast to size_t this way.
Sign in to reply to this message.
https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:12: #include <memory> // for unique_ptr On 2016/04/25 01:46:42, ekr-webrtc wrote: > Are you sure this is going to compile on all our builders? There's already a lot of #include <memory> (and external_tests/common/scoped_ptrs.h should have but doesn't) in the test suite, and use of unique_ptr. Or do you mean having a comment on the same line as a #include? That *should* work in C++, if I understand the parsing/preprocessing spec correctly. And it builds locally on my Windows VM (as well as Mac OS X and Linux). https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:23: } On 2016/04/25 01:46:42, ekr-webrtc wrote: > Are you sure extern "C" is needed here? See: > https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/seccomon... It's not needed; thanks. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:28: // FIXME, bug 1243238: move this into a common location. On 2016/04/25 01:46:42, ekr-webrtc wrote: > Convention here is either TODO(<email>) Done. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:34: #ifdef XP_UNIX On 2016/04/25 01:46:42, ekr-webrtc wrote: > Where does XP_UNIX come from? The first non-empty/comment line of coreconf/UNIX.mk. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:36: #endif On 2016/04/25 01:46:42, ekr-webrtc wrote: > This seems like an odd indirection. Why don't you just put the conditional in > the place it's used? > > Even if you don't do that, you can just conditionalize these declarations inline > since C++ will autogenerate the ctor and dtor. Acknowledged. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:41: _saved_limit.rlim_cur = _saved_limit.rlim_max = RLIM_INFINITY; On 2016/04/25 01:46:42, ekr-webrtc wrote: > It seems odd to set this before you call getrlimit(), which presumably > overwrites it. Does this actually work and if so, why? To handle the error case, but that should be done explicitly for clarity. Fixed. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:63: #endif // DEBUG On 2016/04/25 01:46:42, ekr-webrtc wrote: > If you just conditionalize the SuppressCoreDump on XP_UNIX && DEBUG, then you > can remove this #ifdef. I wasn't sure if SuppressCoreDump would be useful more generally, but I suppose that can be dealt with when/if there's a use for it. I've redone this with 3 versions of DEBUG_ASSERT_DEATH: debug/unix, debug/non-unix, and non-debug. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:71: } On 2016/04/25 01:46:42, ekr-webrtc wrote: > Instead why don't you add ScopedFd Done. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:93: } On 2016/04/25 01:46:42, ekr-webrtc wrote: > ScopedKeyPair Problem: ssl3KeyPair and ssl3_FreeKeyPair are declared in sslimpl.h, but scoped_ptrs.h is used in tests that aren't set up to include libssl's internal headers. But I can just set up a special case for it in ssl_internal_unittest.cc. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:100: ScopedSECKEYPublicKey pubKey; On 2016/04/25 01:46:42, ekr-webrtc wrote: > This isn't C, define right before use. Done. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:117: } On 2016/04/25 01:46:42, ekr-webrtc wrote: > Why are you wrapping this in {}? Just seems confusing. It enforces that tmpPubKey isn't used after ownership is trasferred to pubKey. Not a big deal; I can take it out. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:137: RunOnThreads(size_t n, const F& func) On 2016/04/25 01:46:42, ekr-webrtc wrote: > Please give n a sensible name. Done. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:156: On 2016/04/25 01:46:42, ekr-webrtc wrote: > Please defint hs right before use. Done. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:176: // Run this twice -- on non-debug builds, an excess unlock is ignored. On 2016/04/25 01:46:42, ekr-webrtc wrote: > Why twice? This comment is not clear. Expanded the comment. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:186: for (int i = 0; i < 2; ++i) { On 2016/04/25 01:46:42, ekr-webrtc wrote: > Nor this one. And this one. https://codereview.appspot.com/279190043/diff/40001/external_tests/ssl_gtest/... external_tests/ssl_gtest/ssl_internal_unittest.cc:215: ASSERT_EQ(1 + numThreads * iterations, size_t(keys_->refCount)); On 2016/04/25 01:46:42, ekr-webrtc wrote: > please don't cast to size_t this way. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
