334306 - useless null check in nsDestroyJSPrincipals

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[timeless]

Comment 1

[timeless]

Attachment: drop null check

Comment 2

[Daniel Veditz [:dveditz]]

The null check was added to stop a free memory write (bug 52382) -- are we sure it's no longer needed?

Coverity is simply complaining about the inconsistency, it's possible it's the unchecked AddRef that's wrong. Then again if that's true then ::Subsumes and probably other places are equally dangerous.

Comment 3

[timeless]

i'm definitely not sure. hence the review requests to the relevant parties.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> The null check was added to stop a free memory write (bug 52382) -- are we sure
> it's no longer needed?

That's not where the null-check was added -- that's just where we changed from hiding the null-check within NS_IF_RELEASE since the null-assignment (after the release) within NS_IF_RELEASE was causing a free memory write.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's been null-checked, perhaps reflexively, since the beginning (revision 1.1.2.1).

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 218633 [details] [diff] [review]
drop null check

I'd be slightly happier if nsJSPrincipals::Init() returned an error (or even asserted) when passed a null aPrincipal, but we must not ever do so or we'd be crashing.

sr=dveditz

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 218633 [details] [diff] [review]
drop null check

r=dbaron

Comment 8

[Ryan VanderMeulen [:RyanVM]]

a1.9?

Comment 9

[timeless]

mozilla/caps/src/nsJSPrincipals.cpp 	1.27