1369806 - pkixocsp_VerifyEncodedOCSPResponse.cpp:828:41 [-Wzero-as-null-pointer-constant] zero as null pointer constant

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Daniel Holbert [:dholbert]]

Filing this bug for two warnings that I'm seeing in clang 5.0 Trunk (built from svn 304217):
{
warning: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:828:41 [-Wzero-as-null-pointer-constant] zero as null pointer constant

warning: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:925:1 [-Wzero-as-null-pointer-constant] zero as null pointer constant
}

These are about two calls like this:
  CreateEncodedBasicConstraints(true, 0, Critical::No)

The "0" there is supposed to be a pointer to a long, as shown in the function signature here:
> // BasicConstraints ::= SEQUENCE {
> //         cA                      BOOLEAN DEFAULT FALSE,
> //         pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
> ByteString
> CreateEncodedBasicConstraints(bool isCA,
>                               /*optional*/ long* pathLenConstraintValue,
>                               Critical critical)
> {
https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/security/pkix/test/lib/pkixtestutil.cpp#737-743

It's not clear to me whether the original intent was to pass null (skipping the optional argument), or if we're really trying to pass "0" as the long here (in which case we'd need to create a local variable with value 0 and pass that in).  Maybe those have the same effect anyway?  I don't know this API.

keeler, do you know what the right solution is here?  (Looks like you reviewed this test code back in bug 916629.)

Comment 1

[Daniel Holbert [:dholbert]]

Here are the calls in question:
https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp#828
https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp#881

In every other call to this function, we pass in "nullptr" for this arg, FWIW.  So that does seem to be a reasonable thing to do (though it still seems possible that the intent of the test *might* be to pass in a pointer-to-zero...)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: strawman fix: just use nullptr

Here's the straightforward fix (not changing behavior, just doing s/0/nullptr/ here).  Though as noted above, I'm not 100% sure whether this matches the intent of the test.

Comment 3

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

Comment on attachment 8873907 [details] [diff] [review]
strawman fix: just use nullptr

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

Looks like CreateEncodedBasicConstraints originally landed with that parameter as a long, not a pointer to a long: https://hg.mozilla.org/mozilla-central/rev/2a5624d51bd3#l2.723
It was changed to a pointer in https://hg.mozilla.org/mozilla-central/rev/a24472ea29d4#l4.72 but the callers weren't updated (whoops).
So I think the intention was for that parameter to be 0, in which case we should pass in a pointer to a long of value 0 instead.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Looks like CreateEncodedBasicConstraints originally landed with that
> parameter as a long, not a pointer to a long:

Aha, interesting!

> It was changed to a pointer in
> https://hg.mozilla.org/mozilla-central/rev/a24472ea29d4#l4.72 but the
> callers weren't updated (whoops).

Ah, but actually, the faulty calling code here was added after that change.

The long->long* refactoring was in a push on June 6 2014 -- and then this test code (that passes "0" into CreateEncodedBasicConstraints) was posted a month later (July 10 2014), in bug 916629 comment 39.  And it landed shortly afterwards in https://hg.mozilla.org/mozilla-central/rev/6dc520f8b95e#l1.356

So this test code was wrong when it was posted & when it was landed, AFAICT.

However, I'll bet you're basically right -- the test may have been from WIP local code that was written before the long->long* refactoring

Comment 5

[Daniel Holbert [:dholbert]]

Note: we get many suprious instances of this same build warning (-Wzero-as-null-pointer-constant) for gtest headers using "NULL" rather than nullptr.  So I filed bug 1369864 to suppress this build warning in the pkix gtest directory.  We should still fix this instance, though, since this particular instance seems legitimately broken at the moment.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1369806: Fix up pkix test to correctly pass zero to CreateEncodedBasicConstraints (which takes a pointer-to-long, rather than a long).

Review commit: https://reviewboard.mozilla.org/r/145448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145448/

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8873998 [details]
Bug 1369806: Fix up pkix test to correctly pass zero to CreateEncodedBasicConstraints (which takes a pointer-to-long, rather than a long).

https://reviewboard.mozilla.org/r/145448/#review149374

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:825
(Diff revision 1)
> +  static const long zero = 0;
>  
>    // sub-CA of root (root is the direct issuer of endEntity)
>    const ByteString subCAExtensions[] = {
> -    CreateEncodedBasicConstraints(true, 0, Critical::No),
> +    CreateEncodedBasicConstraints(true, &zero, Critical::No),

(Note: this layers on top of my patch in bug 1369871. Otherwise, this triggers a compile error due to the constness mismatch between the local static variable "zero" here & the type of the parameter.)

Comment 8

[Daniel Holbert [:dholbert]]

Here's a try run (linux only), to be sure this test still passes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d0c49e0c897

Comment 9

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

Comment on attachment 8873998 [details]
Bug 1369806: Fix up pkix test to correctly pass zero to CreateEncodedBasicConstraints (which takes a pointer-to-long, rather than a long).

https://reviewboard.mozilla.org/r/145448/#review149382

Cool - thanks for fixing this.

Comment 10

[Daniel Holbert [:dholbert]]

Thanks for the review & the feedback!

I believe this passed tests -- at least, the Try run has a green GTest run, whose log includes strings like...
 "pkixocsp_VerifyEncodedResponse_successful"
...which seem to come from class names in this .cpp file.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea498127969f
Fix up pkix test to correctly pass zero to CreateEncodedBasicConstraints (which takes a pointer-to-long, rather than a long). r=keeler

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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