Bug 159586 – Infinite Canvas context save() causes WebKit to crash

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

Created attachment 283216 [details]
canvas-context-infinite-save

If a developer adds a call to CanvasRenderingContext2D.save() in an animation without adding the corresponding CanvasRenderingContext2D.restore(), Webkit might end up crashing. Neither the code nor the specs species any limit on how many context state can be saved.

[Said Abou-Hallawa]

Created attachment 283233 [details]
Patch

[Said Abou-Hallawa]

The link to the specs is: https://www.w3.org/TR/2dcontext/#the-canvas-state

[Chris Dumez]

Comment on attachment 283233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283233&action=review

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=159586

Duplicate line?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:388
> +    static const unsigned m_maxSaveCount = 1024;

We don't use m_prefix for statics AFAIK.

> LayoutTests/fast/canvas/canvas-context-save-limit.html:9
> +        const RED = '#ff0000';

Missing a description();

> LayoutTests/fast/canvas/canvas-context-save-limit.html:14
> +        var c=document.getElementById("canvas");

missing spaces around the =

> LayoutTests/fast/canvas/canvas-context-save-limit.html:15
> +        var ctx=c.getContext("2d");

missing spaces around the =

[Said Abou-Hallawa]

Created attachment 283244 [details]
Patch

[Alex Christensen]

Here's the stack trace when it crashes:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BREAKPOINT (SIGTRAP)
Exception Codes:       0x0000000000000002, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

...

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fffbe1378b2 WTF::Vector<WebCore::CanvasRenderingContext2D::State, 1ul, WTF::CrashOnOverflow, 16ul>::reserveCapacity(unsigned long) + 258
1   com.apple.WebCore             	0x00007fffbe137919 void WTF::Vector<WebCore::CanvasRenderingContext2D::State, 1ul, WTF::CrashOnOverflow, 16ul>::appendSlowCase<WebCore::CanvasRenderingContext2D::State const&>(WebCore::CanvasRenderingContext2D::State const&&&) + 89
2   com.apple.WebCore             	0x00007fffbe03d528 WebCore::CanvasRenderingContext2D::realizeSavesLoop() + 72
3   com.apple.WebCore             	0x00007fffbe1311a4 WebCore::CanvasRenderingContext2D::setFillColor(WTF::String const&, WTF::Optional<float>) + 132
4   com.apple.WebCore             	0x00007fffbe5f0012 WebCore::JSCanvasRenderingContext2D::setFillStyle(JSC::ExecState&, JSC::JSValue) + 82
5   com.apple.WebCore             	0x00007fffbe5e57f6 WebCore::setJSCanvasRenderingContext2DFillStyle(JSC::ExecState*, long long, long long) + 150
6   com.apple.JavaScriptCore      	0x00007fffb9d2c2cf JSC::callCustomSetter(JSC::ExecState*, JSC::JSValue, bool, JSC::JSObject*, JSC::JSValue, JSC::JSValue) + 31

[Alex Christensen]

Comment on attachment 283244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283244&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:388
> +    static const unsigned MaxSaveCount = 1024;

I can safely go much higher than this.  Where did 1024 come from?

[Said Abou-Hallawa]

Comment on attachment 283244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283244&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:388
>> +    static const unsigned MaxSaveCount = 1024;
> 
> I can safely go much higher than this.  Where did 1024 come from?

Yes it can go up to std::numeric_limits<unsigned>::max() / sizeof(State). But actually it is hard to track even tens of stacked states. And I think the only possible case for reaching this limit (1024) is a bug in the developer javascript code. I think also the specs does not mention it because it is system and implementation details. In our case we put the limit as std::numeric_limits<unsigned>::max() / sizeof(State) but the problem we crash when we reach this limit. Increasing this limit will not allow more flexibility for the developer to stack more states, but rather it will waste more memory for a buggy code.

[Simon Fraser (smfr)]

Comment on attachment 283244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283244&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=159586

Is there a corresponding radar?

>>> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:388
>>> +    static const unsigned MaxSaveCount = 1024;
>> 
>> I can safely go much higher than this.  Where did 1024 come from?
> 
> Yes it can go up to std::numeric_limits<unsigned>::max() / sizeof(State). But actually it is hard to track even tens of stacked states. And I think the only possible case for reaching this limit (1024) is a bug in the developer javascript code. I think also the specs does not mention it because it is system and implementation details. In our case we put the limit as std::numeric_limits<unsigned>::max() / sizeof(State) but the problem we crash when we reach this limit. Increasing this limit will not allow more flexibility for the developer to stack more states, but rather it will waste more memory for a buggy code.

What do other browsers do?

[Daniel Bates]

Firefox for Mac version 47.0.1 crashes when I visit attachment 283216 [details].

[Chris Dumez]

(In reply to comment #9)
> Firefox for Mac version 47.0.1 crashes when I visit attachment 283216 [details]
> [details].

Ditto for Chrome 52 on Mac.

[Darin Adler]

Seems like a reasonable change to me. Not sure why 1024 specifically is best. I wonder if maybe it would be even better to do something like throw an exception when save is called instead of silently not saving.

[Said Abou-Hallawa]

<rdar://problem/26759984>

[Said Abou-Hallawa]

Created attachment 284548 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 284548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284548&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:360
> +        static NeverDestroyed<String> consoleMessage(ASCIILiteral("The number of unbalanced CanvasRenderingContext2D::save() has exceeded the limit."));

This should refer not to the C++ CanvasRenderingContext2D::save(), but to something the JS author is familiar with. It should also say what the limit is.

[Said Abou-Hallawa]

(In reply to comment #11)
> Seems like a reasonable change to me. Not sure why 1024 specifically is
> best. I wonder if maybe it would be even better to do something like throw
> an exception when save is called instead of silently not saving.

The current limit is just an arbitrary number as well. It is limit of the Vector<State>. It is equal to std::numeric_limits<unsigned>::max() / sizeof(State). If the size of State increases or decreases, the limit will change as well.

I view MaxSaveCount as the size of the stack for drawing a canvas which can be something like that:

function draw1(ctx) {
    ctx.save();
    draw2(ctx);
    ctx.restore();
}

function draw2(ctx) {
    ctx.save();
    draw3(ctx);
    ctx.restore();
}

And so on for MaxSaveCount calls.

How deep a calls tack can JSC handle? I do not know but I can't imagine a drawing scenario which can contain more than 10 unbalanced save() calls. To be in the safe side, I changed MaxSaveCount to be 1024 * 16.

I added a console message to be printed when the limit is reached. The purpose of this fix/message is to notify the developer that his JS code has bug. Instead of just letting WebKit crashes because a bug in the developer's code, we print a message to show the problem.

[Said Abou-Hallawa]

Comment on attachment 284548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284548&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:360
>> +        static NeverDestroyed<String> consoleMessage(ASCIILiteral("The number of unbalanced CanvasRenderingContext2D::save() has exceeded the limit."));
> 
> This should refer not to the C++ CanvasRenderingContext2D::save(), but to something the JS author is familiar with. It should also say what the limit is.

The message is changed to be "CanvasRenderingContext2D.save() has been called without a matching restore() too many times. Ignoring save()."

> LayoutTests/fast/canvas/canvas-context-save-limit-expected.txt:2
> +CONSOLE MESSAGE: line 29: The number of unbalanced CanvasRenderingContext2D::save() has exceeded the limit.
> +CONSOLE MESSAGE: line 29: The number of unbalanced CanvasRenderingContext2D::save() has exceeded the limit.

This message is printed by CanvasRenderingContext2D::realizeSaves(). It is printed twice because of the statement "ctx.fillStyle = RED" in the test file. CanvasRenderingContext2D::setFillColor() calls realizeSaves() and then it calls setFillStyle() which calls realizeSaves() also.

[Said Abou-Hallawa]

Created attachment 284554 [details]
Patch

[Said Abou-Hallawa]

Created attachment 284557 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284557 [details]
Patch

Rejecting attachment 284557 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 284557, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1756859

[Said Abou-Hallawa]

Created attachment 284596 [details]
Patch

[Said Abou-Hallawa]

Created attachment 284607 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284607 [details]
Patch

Clearing flags on attachment: 284607

Committed r203729: <http://trac.webkit.org/changeset/203729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.