Closed Bug 1369806 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.)
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...)
Attached patch strawman fix: just use nullptr (obsolete) — Splinter Review
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.
Attachment #8873907 - Flags: feedback?(dkeeler)
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.
Attachment #8873907 - Flags: feedback?(dkeeler)
(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
Blocks: buildwarning
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.
Assignee: nobody → dholbert
See Also: → 1369864
Depends on: 1369871
Attachment #8873907 - Attachment is obsolete: true
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.)
Here's a try run (linux only), to be sure this test still passes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d0c49e0c897
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.
Attachment #8873998 - Flags: review?(dkeeler) → review+
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.
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ea498127969f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.