Convert XPCStringConvert::ZoneStringCache::mBuffer to void*

RESOLVED FIXED in mozilla38

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → ehsan
Blocks: 1114683
(Assignee)

Comment 1

3 years ago
Created attachment 8550944 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8550944 - Flags: review?(bobbyholley)
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?
Attachment #8550944 - Flags: review?(bobbyholley) → review?(continuation)
(Assignee)

Comment 3

3 years ago
I expect nothing!
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 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. ;)
Attachment #8550944 - Flags: review?(continuation) → review+
Summary: Convert XPCStringConvert::ZoneStringCache::mBuffer to void*; r=bholley → Convert XPCStringConvert::ZoneStringCache::mBuffer to void*
(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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 7

3 years ago
(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!
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/d8c0fee0015a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.