987262 - mozilla::pkix::der::Input::AtEnd returns a bool, not a Result (so don't compare the two)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Attachment: patch

https://mxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixder.h#260
260   if (!nested.AtEnd() != Success) {
261     return Fail(SEC_ERROR_BAD_DER);
262   }

Line 260 should be 'if (!nested.AtEnd()) {'

This is the only instance of this I could find.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8395804 [details] [diff] [review]
patch

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

::: security/pkix/lib/pkixder.h
@@ +256,5 @@
>    if (decoder(nested) != Success) {
>      return Failure;
>    }
>  
> +  if (!nested.AtEnd()) {

This also works:

    return End(input);

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #1)
>     return End(input);

I meant End(nested);

I think there should be some compiler warnings that catch this, and warnings-as-errors should have caused the built to fail. I wonder why that didn't happen.

Comment 3

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

Attachment: patch v2

Ok - I refactored the common checks to use der::End where appropriate.

Comment 4

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

Thanks for the quick reviews.
https://hg.mozilla.org/integration/mozilla-inbound/rev/53fcaeb264ba

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53fcaeb264ba