Closed Bug 995278 Opened 6 years ago Closed 6 years ago

JS_NewArrayBufferContents frees user data on error

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: sfink, Assigned: anujagarwal464)

References

Details

(Whiteboard: [mentor=sfink][lang=C++])

Attachments

(1 file, 3 obsolete files)

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.
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++]
I would like to work on this. Could you provide few pointers?
Flags: needinfo?(sphink)
(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)
Attached patch bug995278.diff (obsolete) — Splinter Review
Assignee: nobody → anujagarwal464
Attachment #8411202 - Flags: feedback?(sphink)
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+
Attached patch bug995278.diff (obsolete) — Splinter Review
Attachment #8411202 - Attachment is obsolete: true
Attachment #8411570 - Flags: review?(sphink)
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)
Attached patch bug995278.diff (obsolete) — Splinter Review
Attachment #8411570 - Attachment is obsolete: true
Attachment #8413177 - Flags: review?(sphink)
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+
Attached patch bug995278.diffSplinter Review
Thanks for help.
Attachment #8413177 - Attachment is obsolete: true
Attachment #8413331 - Flags: review?(sphink)
Attachment #8413331 - Flags: review?(sphink) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/ebdd57580809
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1031920
You need to log in before you can comment on or make changes to this bug.