Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(172)

Issue 279190043: Add simple tests for a few libssl internal interfaces.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 10 months ago by jld
Modified:
5 years, 4 months ago
Reviewers:
ekr-rietveld
Visibility:
Public.

Description

Add 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] #

Unified diffs Side-by-side diffs Delta from patch set Stats (+239 lines, -1 line) Patch
M external_tests/common/scoped_ptrs.h View 1 2 3 3 chunks +3 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/manifest.mn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A external_tests/ssl_gtest/ssl_internal_unittest.cc View 1 2 3 1 chunk +233 lines, -0 lines 0 comments Download
M lib/ssl/ssl3con.c View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M lib/ssl/sslimpl.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 5
ekr-rietveld
https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc#newcode15 external_tests/ssl_gtest/ssl_internal_unittest.cc:15: #undef explicit In other code, we dealt with this ...
5 years, 9 months ago (2015-12-31 17:54:51 UTC) #1
jld
https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc#newcode15 external_tests/ssl_gtest/ssl_internal_unittest.cc:15: #undef explicit On 2015/12/31 17:54:50, ekr-webrtc wrote: > In ...
5 years, 9 months ago (2016-01-13 01:15:54 UTC) #2
ekr-rietveld
Jed, please provide a revised patch here as well as on Bugzilla.
5 years, 8 months ago (2016-01-31 13:18:16 UTC) #3
ekr-rietveld
new https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc File external_tests/ssl_gtest/ssl_internal_unittest.cc (right): https://codereview.appspot.com/279190043/diff/1/external_tests/ssl_gtest/ssl_internal_unittest.cc#newcode46 external_tests/ssl_gtest/ssl_internal_unittest.cc:46: } On 2016/01/13 01:15:54, jld wrote: > On ...
5 years, 5 months ago (2016-04-25 01:46:42 UTC) #4
jld
5 years, 4 months ago (2016-06-08 00:56:31 UTC) #5
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b