1208440 - [Coverity 1122446] NULL deref in CERT_CreateEncodedOCSPSuccessResponse

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

> done:
>     if (privKey)
>         SECKEY_DestroyPrivateKey(privKey);
>     if (br->responseSignature.signature.data)
>         SECITEM_FreeItem(&br->responseSignature.signature, PR_FALSE);
>     PORT_FreeArena(tmpArena, PR_FALSE);
> 
>     return result;
> }

10. var_deref_op: Dereferencing null pointer br.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

Comment 2

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

Comment on attachment 8665946 [details] [diff] [review]
0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

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

LGTM. Technically I'm not an NSS peer, and in any case I think usually NSS patches need a reviewer from another organization, so maybe Kai or Wan-Teh should review this. For patches like these, maybe we should come up with some criteria by which we don't need to go through quite so much process. I'll bring it up in next week's meeting.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Ah, okay. I was mistaken and thought you were an NSS peer, and that we already had a policy for minor changes like this not needing an out-of-your-org reviewer. Thanks for bringing this up in the next call!

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8665946 [details] [diff] [review]
0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

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

LGTM.

Comment 5

[Eric Rescorla (:ekr)]

This is small enough that it doesn't need approval from another organization.
It may need approval from a second NSS peer (MT is also a peer).

needinfoing wtc for the general policy.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8665946 [details] [diff] [review]
0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

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

r=wtc. I verified it is possible to go to |done| when |br| is
still NULL (the initial value of |br|).

In general the best reviewer for a bug fix is the author of
the original code. Kai wrote ocspsig.c in
https://hg.mozilla.org/projects/nss/rev/d010b9d186e0
so I requested a review from him.

Since this is a simple fix and has been reviewed by Eric and
me, please consider Kai's review as optional.

We should also clarify the policy about requiring a reviewer
from another organization. That is only applicable to features
that are potentially controversial or not of general interest.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 8665946 [details] [diff] [review]
0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

Thank you. r=kaie

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Martin, could you check this in please? Thanks!

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8665946 [details] [diff] [review]
0001-Bug-1208440-Fix-NULL-deref-in-CERT_CreateEncodedOCSP.patch

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

https://hg.mozilla.org/projects/nss/rev/92cb85d4f959

This might be the most over-reviewed patch I've seen.

Comment 10

[Martin Thomson [:mt:]]

Clearing n-i on Wan-Teh since I see that he gave this an r+ already.