Closed Bug 1076161 Opened 5 years ago Closed 5 years ago

Prevent JS_StealArrayBufferContents from being used with mapped array buffers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(2 files)

As Luke suggested, it would be far better to check for mapped array buffers than to crash when someone tried to free data stolen from them.
Comment on attachment 8498459 [details] [diff] [review]
Prevent JS_StealArrayBufferContents from being used with mapped array buffers

IIUC, the neutering isn't necessary for safety but, rather, in line with the general "any touching a neutered buffer throws" trend?
Attachment #8498459 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 8498459 [details] [diff] [review]
> Prevent JS_StealArrayBufferContents from being used with mapped array buffers
> 
> IIUC, the neutering isn't necessary for safety but, rather, in line with the
> general "any touching a neutered buffer throws" trend?

Yes, sorry, it's really an unrelated change. The extent of my thinking was "if you're doing this, you're probably confused and would be happier with an exception than something silent."

Maybe it's not a good idea. And maybe these should be asserts.
No, I think it's a great idea.  Runtime errors seem better since I think normal FFOS apps could actually hit this.
Actually, I had another thought on this:

1. neutered buffers should still throw since they always throw when touched in the spec now.

2. stealContents should take 'hasStealableContents' argument, where it is assumed that hasStealableContents implies buffer->hasStealableContents.  The structured-clone caller would pass buffer->hasStealableContents, JS_StealArrayBufferContenst would pass hasMallocContents() (where we'd add a new hasMallocContents() = hasStealableContents() && bufferKind() == PLAIN_BUFFER).

The benefit is that, since WebAudio uses JS_StealArrayBufferContents, it is semantically visible if we throw so this could actually affect FFOS apps.  But more importantly, I want to remove canNeuter() and make hasStealableContents() true for asm.js buffers, so I'd have the same problem but for normal web content.
Like so.
Attachment #8502047 - Flags: review?(sphink)
Comment on attachment 8502047 [details] [diff] [review]
fix-steal-contents-api

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

Sorry, I understood what you meant, just never got around to it. lgtm
Attachment #8502047 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/afacf93a94ff
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.