344914 - crash caused by GetImageData; buggy JSObject allocation

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Sebastian Tusk]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

The following code snippet from nsCanvasRenderingContext2D::GetImageData has a problem with correctly allocating JS objects.

http://lxr.mozilla.org/mozilla/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2846

2846     JSObject *dataArray = JS_NewArrayObject(ctx, w*h*4, jsvector.get());
2847     if (!dataArray)
2848         return NS_ERROR_OUT_OF_MEMORY;
2849 
2850     JSObject *result = JS_NewObject(ctx, NULL, NULL, NULL);
2851     if (!result)
2852         return NS_ERROR_OUT_OF_MEMORY;

What can happen here is that the array allocated in line 2846 gets deallocated by the garbage collector in line 2850. Obviously this leads to a crash later on. 


Reproducible: Always

Comment 1

[Sebastian Tusk]

Attachment: patch preventing interference by GC

Comment 2

[timeless]

we've changed to using localrootscopes instead of jsaddroot. 

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> we've changed to using localrootscopes instead of jsaddroot. 

Except that there isn't a public API for that (bug 332648 seems to cover that)yet, so JS_AddRoot is about the best code in content/ can do.

My only comment on the patch itself is that in Mozilla, we have an nsAutoGCRoot class that'll avoid the fragility of having to add JS_RemoveRoot calls for every early return, and IMO, makes for prettier code. Sebasitan, would you mind making a new patch using that API (also, a nitpick: please use spaces instead of tabs in your editor, since the surrounding code already does that).

Comment 4

[Sebastian Tusk]

Attachment: patch preventing interference by GC

- using nsAutoGCRoot
- no tabs

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Looks good, thanks.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Nice catch, thanks!  I can check this in when the tree opens again, not sure if you have commit privs or not.

Comment 7

[Sebastian Tusk]

I don't have write access to the repository.

Comment 8

[timeless]

sorry, i forgot :). in xpconnect we have lots of magical helpers. i should have been less specific :)

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This code doesn't exist on 1.8.0.x

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Asking for 1.8.1 approval;  potential gc crash fix in canvas code.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in on trunk.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

a=drivers, please land on the 181-branch

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Might you need to root |result| as well?

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in on 1.8.1

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: root |result| as well

Yes, yes it does.  Ugh.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #15)
> Yes, yes it does.  Ugh.

To be clear, it does because JS_DefineProperty can cause result's slots array to grow, which can cause a GC (and we've already learned that newborn roots are not sufficient to protect against objects being collected).

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 229856 [details] [diff] [review]
root |result| as well

Ok, really fully fixed now, requesting 1.8.1 approval again, this time with not one but 2 nsAutoGCRoots.  (Sorry! :/)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229856 [details] [diff] [review]
root |result| as well

a=drivers, especially if it's now "really, fully fixed" :)

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Okay, it's really truly totally fixed on trunk and branch now.