js_NewArrayObjectWithCapacity is unsafe and is being abused in several places

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: gal, Assigned: BMO Automation)

Tracking

Other Branch
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.

http://hg.mozilla.org/mozilla-central/diff/d15754ab3ff1/content/canvas/src/nsCanvasRenderingContext2D.cpp

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). 

http://hg.mozilla.org/mozilla-central/diff/c51c255be44d/content/base/src/nsFrameMessageManager.cpp
(Reporter)

Updated

8 years ago
Blocks: 580877
(Reporter)

Updated

8 years ago
No longer blocks: 580877
(Reporter)

Comment 1

8 years ago
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.
(Reporter)

Comment 2

8 years ago
FrameMessageManager: When you guys use new JSAPI functionality, better get someone from JS to review as well.
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
(Reporter)

Comment 3

8 years ago
Created attachment 459700 [details] [diff] [review]
patch
Assignee: nobody → gal
(Reporter)

Updated

8 years ago
Attachment #459700 - Flags: review?(bzbarsky)
(Reporter)

Updated

8 years ago
Attachment #459700 - Flags: review?(jst)
(Reporter)

Updated

8 years ago
Attachment #459700 - Flags: review?(dwitte)
(Reporter)

Comment 4

8 years ago
Once this patch is in I will delete JS_NewArrayObjectWithCapacity.
(Reporter)

Comment 5

8 years ago
Filed bug 581271 to get rid of JS_FRIEND_API in general.
Comment on attachment 459700 [details] [diff] [review]
patch

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 8

8 years ago
Comment on attachment 459700 [details] [diff] [review]
patch

>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 9

8 years ago
Comment on attachment 459700 [details] [diff] [review]
patch

>+  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?

Updated

8 years ago
Attachment #459700 - Flags: review?(jst) → review+
(Reporter)

Comment 10

8 years ago
We have a conservative stack scanner now.
(Reporter)

Comment 11

8 years ago
http://hg.mozilla.org/tracemonkey/rev/53f9014f9a23
Whiteboard: fixed-in-tracemonkey
(Reporter)

Updated

8 years ago
Blocks: 580846

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [sg:critical]

Updated

8 years ago
Whiteboard: fixed-in-tracemonkey [sg:critical] → fixed-in-tracemonkey [sg:critical][critsmash:patch]

Updated

8 years ago
blocking2.0: ? → beta4+

Comment 13

8 years ago
http://hg.mozilla.org/mozilla-central/rev/53f9014f9a23
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Assignee: gal → automation
Not accessible to reporter
(Assignee)

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.