Closed Bug 785167 Opened 7 years ago Closed 7 years ago

ctypes gives out relocatable pointers to ArrayBuffer data

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(2 files, 2 obsolete files)

When ctypes converts an ArrayBuffer to a pointer, it just calls JS_GetArrayBufferData. This may return an internal pointer if the ArrayBuffer data is inlined, which will break with moving GC and is dangerous even now.

The fix is probably to make JS_GetArrayBufferData un-inline its data, though we may need to add something else to allow peeking at the data.
I may need to add a JS_PeekArrayBufferData or something to avoid the side effect.
Attachment #654844 - Flags: review?(luke)
Assignee: general → sphink
Depends on: 720949
Comment on attachment 654844 [details] [diff] [review]
Copy ArrayBuffer data to separately-allocated storage when JS_GetArrayBufferData is called

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

::: js/src/jsfriendapi.h
@@ +1230,5 @@
>   * Return a pointer to an array buffer's data. The buffer is still owned by the
>   * array buffer object, and should not be modified on another thread.
>   *
> + * Calling this function will permanently prevent the ArrayBuffer from storing
> + * its data inline.

This comment doesn't seem necessary.  The fact that the function un-inlines the buffer seems like a GC implementation detail.  You could say "this function guarantees that the returned buffer's address will be stable across GCs" if you wanted...

::: js/src/jstypedarray.cpp
@@ +290,5 @@
> +       return false;
> +   elements = header->elements();
> +   setElementsHeader(getElementsHeader(), bytes);
> +
> +   // Update all views

phew, good thing you just added the view list :)

@@ +294,5 @@
> +   // Update all views
> +   intptr_t delta = intptr_t(dataPointer()) - intptr_t(oldPointer);
> +   while (view) {
> +       view->setPrivate(reinterpret_cast<uint8_t*>(intptr_t(view->getPrivate()) + delta));
> +       view = view->getFixedSlot(nextViewSlot(view)).toObjectOrNull();

Could you use the "NextView" helper proposed in the comments of the previous patch here?
Attachment #654844 - Flags: review?(luke) → review+
Comment on attachment 654844 [details] [diff] [review]
Copy ArrayBuffer data to separately-allocated storage when JS_GetArrayBufferData is called

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

::: js/src/jstypedarray.cpp
@@ +291,5 @@
> +   elements = header->elements();
> +   setElementsHeader(getElementsHeader(), bytes);
> +
> +   // Update all views
> +   intptr_t delta = intptr_t(dataPointer()) - intptr_t(oldPointer);

Oh, I just realized this subtraction can overflow if dataPointer() is > 2^31 away from oldPointer.  I think if you used uintptr_t (where overflow is defined) then the modular arithmetic would work even if there is overflow, but to make it simpler, perhaps instead you could compute a local delta for each view?
Err, not 2^31 on x64, but you know what I mean :)
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 654844 [details] [diff] [review]
> Copy ArrayBuffer data to separately-allocated storage when
> JS_GetArrayBufferData is called
> 
> Review of attachment 654844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsfriendapi.h
> @@ +1230,5 @@
> >   * Return a pointer to an array buffer's data. The buffer is still owned by the
> >   * array buffer object, and should not be modified on another thread.
> >   *
> > + * Calling this function will permanently prevent the ArrayBuffer from storing
> > + * its data inline.
> 
> This comment doesn't seem necessary.  The fact that the function un-inlines
> the buffer seems like a GC implementation detail.  You could say "this
> function guarantees that the returned buffer's address will be stable across
> GCs" if you wanted...

Ok. I just realized that my concern with this is bogus anyway. (I was worried that people who just wanted to transfer an ArrayBuffer would grab the pointer before stealing the buffer. But stealing an inlined buffer does a malloc anyway, so it makes no difference.)

> ::: js/src/jstypedarray.cpp
> @@ +290,5 @@
> > +       return false;
> > +   elements = header->elements();
> > +   setElementsHeader(getElementsHeader(), bytes);
> > +
> > +   // Update all views
> 
> phew, good thing you just added the view list :)

Yeah, that honestly wasn't planned.

> @@ +294,5 @@
> > +   // Update all views
> > +   intptr_t delta = intptr_t(dataPointer()) - intptr_t(oldPointer);
> > +   while (view) {
> > +       view->setPrivate(reinterpret_cast<uint8_t*>(intptr_t(view->getPrivate()) + delta));
> > +       view = view->getFixedSlot(nextViewSlot(view)).toObjectOrNull();
> 
> Could you use the "NextView" helper proposed in the comments of the previous
> patch here?

Done.
(In reply to Luke Wagner [:luke] from comment #3)
> Comment on attachment 654844 [details] [diff] [review]
> Copy ArrayBuffer data to separately-allocated storage when
> JS_GetArrayBufferData is called
> 
> Review of attachment 654844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jstypedarray.cpp
> @@ +291,5 @@
> > +   elements = header->elements();
> > +   setElementsHeader(getElementsHeader(), bytes);
> > +
> > +   // Update all views
> > +   intptr_t delta = intptr_t(dataPointer()) - intptr_t(oldPointer);
> 
> Oh, I just realized this subtraction can overflow if dataPointer() is > 2^31
> away from oldPointer.  I think if you used uintptr_t (where overflow is
> defined) then the modular arithmetic would work even if there is overflow,
> but to make it simpler, perhaps instead you could compute a local delta for
> each view?

Sadly, I was using signed intptr_t *because* of the overflow. It semantically makes more sense to me. I keep forgetting that signed overflow is undefined. Sigh.
The description looks good, although I do not quite understand this patch - I hope I will be excused since |stealContents| doesn't look landed yet.

What is the cost of |uninlineData|, in terms of performances?
Unfortunately, to get this working on the try server, I added an ugly allocation weirdness that I am not comfortable slipping in without review.

Specifically, JS_GetArrayBufferData allows the caller to invoke it with a NULL context, because it is very awkward for some callers to have a context available. Now that this can trigger un-inlining, it means we may need to do memory allocation. I worked around this by allocating with the runtime instead of the context, but only when the context is NULL. See AllocateArrayBufferContents and callers for the change.
Attachment #656124 - Flags: review?(luke)
Attachment #654844 - Attachment is obsolete: true
Comment on attachment 656124 [details] [diff] [review]
Copy ArrayBuffer data to separately-allocated storage when JS_GetArrayBufferData is called

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

::: js/src/jstypedarray.cpp
@@ +209,2 @@
>  static ObjectElements *
> +AllocateArrayBufferContents(JSRuntime *rt, JSContext *cx, uint32_t nbytes, uint8_t *contents)

In that case, we usually use the name 'maybecx'.

@@ +211,4 @@
>  {
> +    uint32_t size = nbytes + sizeof(ObjectElements);
> +    ObjectElements *newheader =
> +        static_cast<ObjectElements *>(cx ? cx->calloc_(size) : rt->calloc_(size));

Usually, we just use OffTheBooks::calloc_ which avoids the annoying 'rt' parameter.

@@ +292,5 @@
>  #endif
>  }
>  
> +bool
> +ArrayBufferObject::uninlineData(JSContext *cx)

maybecx

@@ +3560,5 @@
>      return obj->asArrayBuffer().byteLength();
>  }
>  
>  JS_FRIEND_API(uint8_t *)
>  JS_GetArrayBufferData(JSObject *objArg, JSContext *cx)

maybecx here too
Attachment #656124 - Flags: review?(luke) → review+
So this is messy. The typed array API allows passing in NULL for cx to some functions. Although those functions mostly can't GC, they *do* unwrap their argument, and that unwrapping can fail, which will create an exception iff there is a cx to stick the exception on.

If RootedObject's constructor were a no-op when passed a NULL cx, we would be all set. But that's pretty horrific.

For now, I hacked around it. I could create a MaybeRootedObject or something, but at this point I'm thinking I'll put this mess in now and later take another shot at seeing if there's a way to make the cx mandatory after all.

Comments welcome.
Attachment #656266 - Flags: review?(terrence)
Yeah, this is pretty ugly.  We'll have the tools soon to put these on the runtime, so until then could we just leave these as JSObject* and put SkipRoot in where needed to get analysis passing?
(In reply to Terrence Cole [:terrence] from comment #11)
> Yeah, this is pretty ugly.  We'll have the tools soon to put these on the
> runtime, so until then could we just leave these as JSObject* and put
> SkipRoot in where needed to get analysis passing?

How do I do that? It looks like SkipRoot still dereferences its cx, so I can't use it in the NULL cx case. I could put a SkipRoot into CheckedUnwrap on the non-NULL cx path -- heck, I could put a real root in CheckedUnwrap -- but that wouldn't fix the caller's stack slots.

Wait... why do I need rooting at all here? UnwrapObjectChecked can trigger a GC, but it takes an object and returns an object. So those objects don't need rooting as long as you don't use the passed-in one after it returns, and CheckedUnwrap does not. CheckedUnwrap itself is the same. So unless there's another gcthing involved, nothing needs rooting in the first place.

Except for in UnwrapObjectChecked, because it *does* use its input parameter after doing something that could GC. So I'll root there. (Other things in jswrapper.cpp look improperly rooted too, btw. And the rooting analysis would never catch it because we never hit that failure path.)
Attachment #657028 - Flags: review?(terrence)
Attachment #656266 - Attachment is obsolete: true
Attachment #656266 - Flags: review?(terrence)
Comment on attachment 657028 [details] [diff] [review]
Root while allowing cx to be NULL for some typed array APIs

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

Much nicer!
Attachment #657028 - Flags: review?(terrence) → review+
/home/sfink/src/MI-upstream/rev/c505e38af094
/home/sfink/src/MI-upstream/rev/f6d0c187d568
https://hg.mozilla.org/mozilla-central/rev/738f1ceeb51f
https://hg.mozilla.org/mozilla-central/rev/b2ba9bbf909f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
It looks like this - or perhaps bug 720949 - caused a small performance regression for the native fennec Ts. 

Was that expected?

---

Message: 6
Date: Sat, 01 Sep 2012 00:40:43 -0000
From: nobody@cruncher.build.mozilla.org
To: dev-tree-management@lists.mozilla.org
Subject: Talos Regression Ts increase 3.57% on Android 2.2 (Native)
        Mozilla-Inbound
Message-ID:
        <20120901004043.A071B1044C3@cruncher.srv.releng.scl3.mozilla.com>
Content-Type: text/plain; charset="us-ascii"

Regression Ts increase 3.57% on Android 2.2 (Native) Mozilla-Inbound
-----------------------------------------------------------------------
    Previous: avg 2954.302 stddev 34.972 of 30 runs up to revision 2b55fac2725c
    New     : avg 3059.916 stddev 9.814 of 5 runs since revision b2ba9bbf909f
    Change  : +105.614 (3.57% / z=3.020)
    Graph   : http://mzl.la/Rvu2E3

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2b55fac2725c&tochange=b2ba9bbf909f

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/102c2795bacc
    : Steve Fink <sfink@mozilla.com> - Bug 720949 - Add JSAPI for transferring ArrayBuffer contents. r=terrence
    : http://bugzilla.mozilla.org/show_bug.cgi?id=720949

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/738f1ceeb51f
    : Steve Fink <sfink@mozilla.com> - Bug 785167 - Copy ArrayBuffer data to separately-allocated storage when JS_GetArrayBufferData is called. r=luke
    : http://bugzilla.mozilla.org/show_bug.cgi?id=785167

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/b2ba9bbf909f
    : Steve Fink <sfink@mozilla.com> - Bug 785167 - Root while allowing cx to be NULL for some typed array APIs. r=terrence
    : http://bugzilla.mozilla.org/show_bug.cgi?id=785167

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=720949 - Need API for "transferring" ArrayBuffer data between runtimes (via shared memory)
  * http://bugzilla.mozilla.org/show_bug.cgi?id=785167 - ctypes gives out relocatable pointers to ArrayBuffer data
I'll let Steve chime in with a definitive answer, but I think we have to take this: the API is unsafe without this change.
Well, bug 788205 shows an even more significant regression, so I think I'll have to come up with some other alternative. I think it's doable, though it may be ugly.
You need to log in before you can comment on or make changes to this bug.