1123002 - Convert XPCStringConvert::ZoneStringCache::mBuffer to void*

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[(no longer active)]

This variable is only used for pointer comparison in order to compute
a cache match.  This patch converts the type of the variable to void*
so that it can still be used for the pointer comparison, but not for
any other purpose, including dereferencing, since the pointer may
potentially be dangling if the string buffer dies.

Comment 1

[(no longer active)]

Attachment: Convert XPCStringConvert::ZoneStringCache::mBuffer to void*

This variable is only used for pointer comparison in order to compute
a cache match.  This patch converts the type of the variable to void*
so that it can still be used for the pointer comparison, but not for
any other purpose, including dereferencing, since the pointer may
potentially be dangling if the string buffer dies.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8550944 [details] [diff] [review]
Convert XPCStringConvert::ZoneStringCache::mBuffer to void*

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

Hm, this is Andrew's code. I'm confused though - if this pointer can dangle, what's to stop us from getting false cache hits with another string allocated in the same region of memory?

Comment 3

[(no longer active)]

I expect nothing!

Comment 4

[Andrew McCreight [:mccr8]]

mString owns mBuffer.  mString is a JS thing, so it can only die during GC.  We clear mString and mBuffer during GC.  So, yes, if the pointer is dangling we're probably in trouble anyways.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8550944 [details] [diff] [review]
Convert XPCStringConvert::ZoneStringCache::mBuffer to void*

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

I guess this is okay to appease the analysis, but we're going to be in trouble in other ways if this is ever a dangling pointer. ;)

Comment 6

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> mString owns mBuffer.  mString is a JS thing, so it can only die during GC. 
> We clear mString and mBuffer during GC.  So, yes, if the pointer is dangling
> we're probably in trouble anyways.

Can this be added as a comment in the patch Ehsan?

Comment 7

[(no longer active)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #6)
> (In reply to Andrew McCreight [:mccr8] from comment #4)
> > mString owns mBuffer.  mString is a JS thing, so it can only die during GC. 
> > We clear mString and mBuffer during GC.  So, yes, if the pointer is dangling
> > we're probably in trouble anyways.
> 
> Can this be added as a comment in the patch Ehsan?

Of course!

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8c0fee0015a