Closed
Bug 995278
Opened 10 years ago
Closed 10 years ago
JS_NewArrayBufferContents frees user data on error
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: sfink, Assigned: anujagarwal464)
References
Details
(Whiteboard: [mentor=sfink][lang=C++])
Attachments
(1 file, 3 obsolete files)
5.67 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
Now that we don't have headers on ArrayBuffers any more, JS_NewArrayBufferWithContents is totally usable for wrapping external data in an ArrayBuffer. But I noticed one oddity -- if the object allocation fails, it frees any data buffer you pass in. That seems unexpected and unfortunate for this use case.
Reporter | ||
Comment 1•10 years ago
|
||
Fixing this is pretty trivial, though you'd need to audit all in-tree callers to make sure they dispose of the data now. I expect there are very few.
Whiteboard: [mentor=sfink][lang=C++]
Assignee | ||
Comment 2•10 years ago
|
||
I would like to work on this. Could you provide few pointers?
Flags: needinfo?(sphink)
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Anuj Agarwal [:anujagarwal464] from comment #2) > I would like to work on this. Could you provide few pointers? Sorry, was on vacation last week. http://dxr.mozilla.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#648 frees the passed-in data if the preceding NewBuiltinClassInstance() call fails. That's not right when using JS_NewArrayBufferWithContents: the caller has some data that it wants to create an ArrayBuffer for, so it calls JS_NewArrayBufferWithContents(), which fails for some internal reason. Should the caller discard the data pointer it passed in or not? It depends on how far JS_NewArrayBufferWithContents made it before detecting a failure. It would be nicer if JS_NewArrayBufferWithContents either succeeded (and therefore took ownership of that data pointer) or failed and did not take ownership. The comments for the function should describe the new behavior, and all callers should be audited to make sure they free the pointer on failure. (Or do something else -- they could force some memory to be released and try again, though I doubt that'll ever happen in practice.)
Flags: needinfo?(sphink)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee: nobody → anujagarwal464
Attachment #8411202 -
Flags: feedback?(sphink)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8411202 [details] [diff] [review] bug995278.diff Review of attachment 8411202 [details] [diff] [review]: ----------------------------------------------------------------- This is basically right. One more thing, though -- can you document JS_NewArrayBufferWithContents and JS_NewMappedArrayBufferWithContents to say that the ownership of the pointer is not transferred on a failed call? (Or say "on success, the ownership is transferred...". Though explicit is good.) Then flag it for r? :sfink Thank you for taking this on! ::: content/base/src/nsXMLHttpRequest.cpp @@ +3928,5 @@ > JSObject* obj = JS_NewArrayBufferWithContents(aCx, mLength, mDataPtr); > + > + mDataPtr = nullptr; > + mLength = mCapacity = 0; > + Remove the whitespace from these blank lines. ::: js/src/jsapi-tests/testMappedArrayBuffer.cpp @@ +64,5 @@ > close(fd); > if (!ptr) > return nullptr; > JSObject *obj = JS_NewMappedArrayBufferWithContents(cx, length, ptr); > + More whitespace. ::: toolkit/components/osfile/NativeOSFileInternals.cpp @@ +364,5 @@ > > JS::Rooted<JSObject*> > arrayBuffer(cx, JS_NewArrayBufferWithContents(cx, contents.nbytes, contents.data)); > if (!arrayBuffer) { > + js_free(contents.data); I don't think this is necessary (and was a double free before this patch). ArrayBufferContents looks like it will auto-free unless you call forget() on it. So I think you should just leave this one alone.
Attachment #8411202 -
Flags: feedback?(sphink) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8411202 -
Attachment is obsolete: true
Attachment #8411570 -
Flags: review?(sphink)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8411570 [details] [diff] [review] bug995278.diff Review of attachment 8411570 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1039,5 @@ > JS_PUBLIC_API(JSObject *) > JS_NewArrayBufferWithContents(JSContext *cx, size_t nbytes, void *contents) > { > JS_ASSERT(contents); > + // on success, the ownership is transferred... Er, no, I meant in the jsapi.h header comments, so users know what the rules are.
Attachment #8411570 -
Flags: review?(sphink)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8411570 -
Attachment is obsolete: true
Attachment #8413177 -
Flags: review?(sphink)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8413177 [details] [diff] [review] bug995278.diff Review of attachment 8413177 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +3100,5 @@ > JS_SetAllNonReservedSlotsToUndefined(JSContext *cx, JSObject *objArg); > > /* > + * Create a new array buffer with the given contents. On success, the ownership > + * is transferred to new array buffer. to *the* new array buffer. @@ +3133,5 @@ > JS_ReallocateArrayBufferContents(JSContext *cx, uint32_t nbytes, void *oldContents, uint32_t oldNbytes); > > /* > + * Create a new mapped array buffer with the given memory mapped contents. On success, > + * the ownership is transferred to new mapped array buffer. to *the* new mapped array buffer. And there's an extra space at the end of the line.
Attachment #8413177 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Thanks for help.
Attachment #8413177 -
Attachment is obsolete: true
Attachment #8413331 -
Flags: review?(sphink)
Reporter | ||
Updated•10 years ago
|
Attachment #8413331 -
Flags: review?(sphink) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdd57580809 Thanks for the patch! Just a friendly reminder that your commit message should be summarizing what the patch is doing, not restating the problem it's fixing. Thanks! https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Flags: in-testsuite+
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ebdd57580809
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•