Closed Bug 581264 Opened 13 years ago Closed 13 years ago

js_NewArrayObjectWithCapacity is unsafe and is being abused in several places


(Core :: DOM: Core & HTML, defect)

Other Branch
Not set



Tracking Status
blocking2.0 --- beta4+


(Reporter: gal, Assigned: automation)



(Whiteboard: fixed-in-tracemonkey [sg:critical][critsmash:patch])


(1 file)

A couple things.

Don't ever ever ever declare JS functions in your own files. If we change our headers, you are out of sync and horrible things happen. Waldo should have balked at you for this one.

Second, you can't use NewArrayObjectWithCapacity if there is any chance that a GC happens. The interior parts of it are not initialized. Best case we crash. Making that function a friend in itself is a mistake (I am deleting the function altogether).
Blocks: 580877
No longer blocks: 580877
CTypes is parenting fields of the struct array to the array. That seems .. mhm? .. bizarre? I changed that so I can use NewArrayObject. Regular objects shouldn't have parents to begin with. I don't think this is used either. dwitte will have to review.
FrameMessageManager: When you guys use new JSAPI functionality, better get someone from JS to review as well.
blocking2.0: --- → ?
Attached patch patchSplinter Review
Assignee: nobody → gal
Attachment #459700 - Flags: review?(bzbarsky)
Attachment #459700 - Flags: review?(jst)
Attachment #459700 - Flags: review?(dwitte)
Once this patch is in I will delete JS_NewArrayObjectWithCapacity.
Filed bug 581271 to get rid of JS_FRIEND_API in general.
Comment on attachment 459700 [details] [diff] [review]

r=me on the canvas bit, which is what I assume you were looking for from me.  If not, what else did you want me to look at?
Attachment #459700 - Flags: review?(bzbarsky) → review+
(In reply to comment #2)
> FrameMessageManager: When you guys use new JSAPI functionality, better get
> someone from JS to review as well.
I actually copied the code from canvas code.
I probably shouldn't have done that.
Comment on attachment 459700 [details] [diff] [review]

>diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp

> // For a struct field with 'name' and 'type', add an element to field
> // descriptor array 'arrayObj' of the form { name : type }.
> static JSBool
> AddFieldToArray(JSContext* cx,
>-                JSObject* arrayObj,

Comment is stale.

>   // Prepare a new array for the 'fields' property of the StructType.
>-  jsval* fieldsVec;
>-  JSObject* fieldsProp =
>-    js_NewArrayObjectWithCapacity(cx, len, &fieldsVec);
>-  if (!fieldsProp)
>+  Vector<jsval> fieldsVec(cx);

We don't want ContextAllocPolicy here, and it'd be nice to use an autoarray. CTypes.h declares an Array<T, N> for this purpose (derived from Vector); you should use Array<jsval, 16>.

Don't you need to root the vector elements? (We only scan the C stack, not the heap, right?)

r=dwitte with changes.
Attachment #459700 - Flags: review?(dwitte) → review+
Comment on attachment 459700 [details] [diff] [review]

>+  JSObject* fieldsProp = JS_NewArrayObject(cx, len, fieldsVec.begin());
>+  if (!fieldsProp)
>+    return NULL;

Also, just to make sure -- this doesn't need to be explicitly rooted, right?
Attachment #459700 - Flags: review?(jst) → review+
We have a conservative stack scanner now.
Whiteboard: fixed-in-tracemonkey
Blocks: 580846
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [sg:critical]
Whiteboard: fixed-in-tracemonkey [sg:critical] → fixed-in-tracemonkey [sg:critical][critsmash:patch]
blocking2.0: ? → beta4+
Closed: 13 years ago
Resolution: --- → FIXED
Assignee: gal → automation
Not accessible to reporter
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.