1318030 - Possible uninitialised value uses relating to security/pkix/test/gtest/pkixcert_extension_tests.cpp

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

I have only the thinnest of evidence that this actually has anything
to do with the pkixcert_extension gtests, but am filing anyway.

Running gtests on valgrind produces the complaints in the attachment.
These seem to be to do with printing something, and, following the
stack traces a bit, to do with printing bytes in an object.  It might
be that the object in question contains alignment holes that are
uninitialised, which is why V complains.

The stack traces are all gtest internals, super-clever templated code
that I don't claim to understand.  The only connection to any particular
test is the fact that pkixcert_extension appears as a type template
parameter:

  0x16CC81E7: 
  testing::internal::ParameterizedTestCaseInfo<pkixcert_extension>::RegisterTests()

As a result I wound up looking at security/pkix/test/gtest/pkixcert_extension_tests.cpp
and wondering whether either of

  TEST_P(pkixcert_extension, ExtensionHandledProperly)  or
  TEST_F(pkixcert_extension, DuplicateSubjectAltName)

might create a temporary object, with alignment holes, which gets
printed out as part of the test.

Comment 1

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

My guess: the gtest framework is trying to print values of type ExtensionTestcase to generate the names of tests parameterized over values of that type.  The gtest value printer defaults to hexdumping the bytes in the object if there isn't a specialization for the type in question.  On x86_64 Linux, ExtensionTestcase is a basic_string (size 8, alignment 8) followed by an enum (size 4, alignment 4), so the last 4 bytes are alignment padding, which is uninitialized.

Also, basic_string is a refcounted pointer, so dumping the bytes of the pointer itself is probably not very useful (although not a valgrind error).

I'd suggest specializing the gtest value printer for ExtensionTestcase; see https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#teaching-google-test-how-to-print-your-values

Comment 2

[Julian Seward [:jseward]]

I believe you're right.  On investigation, though, the problem is
more widespread than it appears at first -- there are reports
for quite a number of the pkix gtests.  Only one is actually
presented because of valgrind's heuristic of considering errors
to be duplicates if the top few (4 ?) stack frames match, which
in this case they all do.

I made a bunch of dummy gtest printer methods, enough to keep V
quiet.  They just print a "fixme" string, but this doesn't seem to
have any negative effect on the test outcomes.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1318030-1.cset

This is the patch.  I'm not sure whether it's really suitable
for landing.  dkeeler, can you advise?

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Sure, let's do something like this (but not exactly this - if there's something useful to print, we can, but otherwise we can just print some generic identifier).

Comment 5

[Julian Seward [:jseward]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> if there's something useful to print, we can, but otherwise we can
> just print some generic identifier).

Can you be a bit more specific?  I don't know how to modify the patch
with that.  What's the definition of "something useful"?  And for the
cases where it isn't, what kind of generic identifier did you have in
mind?

Comment 6

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

Something I was wrong about earlier: the printed values aren't used in the test names, but they do appear in the optional report file; e.g., from NSS:

  <testcase name="HasNoUtf8/5" value_param="6-byte object &lt;00-D8 FF-FE 00-00&gt;" status="run" time="0" classname="BadUtf16TestCases/BadUtf16Test" />

So it's useful if you can look at the printed version and see what's being tested, but not essential (at least if the source is an array so you can count off the Nth element).

That said, all of the structs here are products of ByteString, pkix::Result, and a few primitive types (bool, some integers), so it's not too hard to preserve all the information.  There's already a PrintTo for pkix::Result that tries to generate meaningful strings, so you'd just need a helper routine to hexdump the ByteStrings (or use gtest's printer for those, which prints them as quoted ASCII with hex escapes), and then the rest is boilerplate.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Julian Seward [:jseward] from comment #5)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> > if there's something useful to print, we can, but otherwise we can
> > just print some generic identifier).
> 
> Can you be a bit more specific?  I don't know how to modify the patch
> with that.  What's the definition of "something useful"?  And for the
> cases where it isn't, what kind of generic identifier did you have in
> mind?

Sorry - I meant pretty much what Jed said. e.g. for EKUTestcase, we can hex-print the ekuSEQUENCE, write something that handles KeyPurposeId (there's only 6 of them), and use the PrintTo on the Results. I suppose that's a non-trivial amount of work, so one option would be to just land this patch with "TODO (bug #######)" instead of the "FIXME"s and file a follow-up.

Comment 8

[Julian Seward [:jseward]]

Attachment: bug1318030-2.cset

Revised per the less-work suggestion in comment 7.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8812157 [details] [diff] [review]
bug1318030-2.cset

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

Thanks!
r=me with s/bug 1318030 comment 7/bug 1318770/g

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a614fa2f4d78
Possible uninitialised value uses relating to security/pkix/test/gtest/pkixcert_extension_tests.cpp.  r=[e-mail].

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a614fa2f4d78