Closed Bug 334306 Opened 18 years ago Closed 16 years ago

useless null check in nsDestroyJSPrincipals

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: coverity)

Attachments

(1 file)

drop null check
634 bytes, patch
dbaron
: review+
dveditz
: superreview+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
Attached patch drop null check — — Splinter Review 

        Attachment #218633 -
        Flags: superreview?(dveditz)

        Attachment #218633 -
        Flags: review?(dbaron)

    
    

    
  
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.

    
    

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

    
    

    
  
(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.

    
    

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

    
    

    
  
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

        Attachment #218633 -
        Flags: superreview?(dveditz) → superreview+

    
    

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

r=dbaron

        Attachment #218633 -
        Flags: review?(dbaron) → review+

    
    

    
  
a1.9?
Flags: blocking1.9?

    
  

        Attachment #218633 -
        Flags: approval1.9+
Flags: blocking1.9? → blocking1.9-

    
  
Keywords: checkin-needed
OS: Mac OS X → All
QA Contact: caps
Hardware: Macintosh → All
Whiteboard: [needs landing]

    
    

    
  
mozilla/caps/src/nsJSPrincipals.cpp 	1.27
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Summary: hopefully useless null check in nsDestroyJSPrincipals → useless null check in nsDestroyJSPrincipals
Whiteboard: [needs landing]

          You need to log in
          before you can comment on or make changes to this bug.