898494 - Exactly root BinaryData

Status: RESOLVED FIXED

Description

[Terrence Cole [:terrence]]

The landing of BinaryData introduced a bunch of new GC hazards for exact rooting: 

http://people.mozilla.org/~sfink/analysis/
http://people.mozilla.org/~sfink/analysis/shell/rootingHazards.txt

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Created attachment 781918 [details] [diff] [review]
Fix lack of rooting in Binary Data.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 781918 [details] [diff] [review]
Fix lack of rooting in Binary Data.

I don't understand these two reports. |obj| is rooted, but what is this field:0?

Function 'JSObject* js::ArrayType::create(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted '__temp_41' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 GetMemSize(JSContext*, class JS::Handle<JSObject*>)' at js/src/builtin/BinaryData.cpp:642
    js/src/builtin/BinaryData.cpp:642: Assign(89,90, __temp_44 := elementType*)
    js/src/builtin/BinaryData.cpp:642: Call(90,91, __temp_43 := GetMemSize(cx*,__temp_44*))
    js/src/builtin/BinaryData.cpp:642: Call(91,92, __temp_42 := Int32Value((__temp_43* * length*)))
    js/src/builtin/BinaryData.cpp:642: Call(92,93, __temp_41*.field:0.setFixedSlot(0,__temp_42))

Function 'JSObject* js::ArrayType::create(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted '__temp_45' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64 GetAlign(JSContext*, class JS::Handle<JSObject*>)' at js/src/builtin/BinaryData.cpp:644
    js/src/builtin/BinaryData.cpp:644: Assign(94,95, __temp_48 := elementType*)
    js/src/builtin/BinaryData.cpp:644: Call(95,96, __temp_47 := GetAlign(cx*,__temp_48*))
    js/src/builtin/BinaryData.cpp:644: Call(96,97, __temp_46 := Int32Value(__temp_47*))
    js/src/builtin/BinaryData.cpp:644: Call(97,98, __temp_45*.field:0.setFixedSlot(1,__temp_46))

Comment 3

[Terrence Cole [:terrence]]

(In reply to Nikhil Marathe [:nsm] from comment #2)
> Comment on attachment 781918 [details] [diff] [review]
> Fix lack of rooting in Binary Data.
> 
> I don't understand these two reports. |obj| is rooted, but what is this
> field:0?
> 
> Function 'JSObject* js::ArrayType::create(JSContext*, class
> JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted
> '__temp_41' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64
> GetMemSize(JSContext*, class JS::Handle<JSObject*>)' at
> js/src/builtin/BinaryData.cpp:642
>     js/src/builtin/BinaryData.cpp:642: Assign(89,90, __temp_44 :=
> elementType*)
>     js/src/builtin/BinaryData.cpp:642: Call(90,91, __temp_43 :=
> GetMemSize(cx*,__temp_44*))
>     js/src/builtin/BinaryData.cpp:642: Call(91,92, __temp_42 :=
> Int32Value((__temp_43* * length*)))
>     js/src/builtin/BinaryData.cpp:642: Call(92,93,
> __temp_41*.field:0.setFixedSlot(0,__temp_42))

The code in question here is:
    obj->setFixedSlot(SLOT_MEMSIZE,
                      Int32Value(::GetMemSize(cx, elementType) * length));

C++ ensures that all arguments that are calls get fully resolved before they are subsequently passed to a call, however, it does not enforce any ordering between those calls and other operations within the same sequence point. Thus, a perfectly valid ordering of the operations on this line is:

(1) RootedObject::operator->  => Unpacks raw JSObject* to stack.
(2) GetMemSize                => GC's and corrupts stack copy of obj.
(3) ObjectImpl::setFixedSlot  => |this| is now the corrupt stack copy.

You need to establish a sequence point between your GetMemSize and where you insert the thing into the heap. In general you should always assign the result of any GC function to a stack temporary.

For example, an even more subtle variant of this problem is where you have operator-> and operator= on a HeapPtr: |obj->shape = MaybeGC(...); // kaboom!|

> Function 'JSObject* js::ArrayType::create(JSContext*, class
> JS::Handle<JSObject*>, class JS::Handle<JSObject*>, uint32)' has unrooted
> '__temp_45' of type 'JSObject*' live across GC call 'BinaryData.cpp:uint64
> GetAlign(JSContext*, class JS::Handle<JSObject*>)' at
> js/src/builtin/BinaryData.cpp:644
>     js/src/builtin/BinaryData.cpp:644: Assign(94,95, __temp_48 :=
> elementType*)
>     js/src/builtin/BinaryData.cpp:644: Call(95,96, __temp_47 :=
> GetAlign(cx*,__temp_48*))
>     js/src/builtin/BinaryData.cpp:644: Call(96,97, __temp_46 :=
> Int32Value(__temp_47*))
>     js/src/builtin/BinaryData.cpp:644: Call(97,98,
> __temp_45*.field:0.setFixedSlot(1,__temp_46))

And this appears to be the same on the next line.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 781918 [details] [diff] [review]
Fix lack of rooting in Binary Data.

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

::: js/src/builtin/BinaryData.cpp
@@ -822,5 @@
>          if (valueStr)
>              valueChars = JS_EncodeString(cx, valueStr);
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INCOMPATIBLE_PROTO, "ArrayType", "repeat", valueChars);
> -        if (valueStr)
> -            JS_free(cx, valueChars);

I think you still need to free these characters; at least, nothing in this patch is changing whether you would need to or not.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Created attachment 782000 [details] [diff] [review]
Fix lack of rooting in Binary Data

Fixes pointed out by :terrence.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf208ba34b9c

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf208ba34b9c