337421 - Should we protect against mRefCnt integer overflow?

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Jesse Ruderman]

Should we protect against mRefCnt integer overflow?

I haven't tested this, but I think an some "addref without release" leaks could lead to an integer overflow of mRefCnt, which would then leave dangling nsCOMPtrs.  Luckily, that type of leak is rare compared to leaks due to cycles.

Comment 1

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

Hrm.  I thought I filed this bug a while back, but I can't find it.  Maybe I just sent email somewhere.

The way you'd have to fix this is probably to make PRUint32(-1) never be decremented once it's hit, in case all the references in question are real.  This is easy to do for the non-threadsafe versions, but I'm not sure how to do it for the threadsafe ones.

Comment 2

[Jesse Ruderman]

> make PRUint32(-1) never be decremented once it's hit

And never incremented either?  So mRefCnt == -1 would mean "never to be freed"?

Comment 3

[chris hofmann]

still a problem?

Comment 4

[Jesse Ruderman]

yes

Comment 6

[Johnny Stenback (:jst)]

Attachment: WIP

This is at least a start if we choose to go down this path. I haven't measured performance or codesize impacts of this, and all that would need to happen etc, and I don't think I'll have time to work more on this any time soon. If anyone wants to pick this up from here, feel free...

Comment 7

[Daniel Veditz [:dveditz]]

We've had one sg:critical abuse of int16 refcounters (and fixed them), but for now 32-bit seems impractical to overflow so I'm guessing at sg:moderate.

Comment 8

[Daniel Veditz [:dveditz]]

Given the advisory for the 16bit version (bug 440230) this probably doesn't
warrant being security-sensitive. Objections to unhiding it?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

We could probably more easily measure perf impact now (assuming we still care about this bug).

Comment 10

[Andrew McCreight [:mccr8]]

Ref counts were changed to be pointer sized in bug 798158, so I think overflow shouldn't be a real issue any more.

Comment 11

[Ben Bucksch (:BenB)]

Time to unhide bug 798158?