Bug 151825 (CVE-2016-4592) - Place an upper bound on canvas pixel counts
Summary: Place an upper bound on canvas pixel counts
Status: RESOLVED FIXED
Alias: CVE-2016-4592
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 152143
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-03 14:43 PST by Brent Fulgham
Modified: 2016-07-19 17:25 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.59 KB, patch)
2015-12-03 14:57 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2015-12-03 17:19 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (295.74 KB, application/zip)
2015-12-03 17:49 PST, Build Bot
no flags Details
Patch (6.52 KB, patch)
2015-12-04 15:27 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2015-12-04 15:59 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (789.31 KB, application/zip)
2015-12-04 16:25 PST, Build Bot
no flags Details
Patch (6.69 KB, patch)
2015-12-04 16:42 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2015-12-04 17:10 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (Revised with memory limit bumped to satisfy test criteria) (7.50 KB, patch)
2015-12-18 10:20 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-12-03 14:43:16 PST
Malformed JavaScript can attempt to allocate massive numbers of canvas buffers, slowing down the user's system. We should place an upper sanity bound on this value (similar to what Blink does).
Comment 1 Brent Fulgham 2015-12-03 14:44:09 PST
<rdar://problem/23324916>
Comment 2 Brent Fulgham 2015-12-03 14:57:41 PST
Created attachment 266563 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-12-03 15:07:01 PST
Comment on attachment 266563 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:109
> +        unsigned pixelsReleased = width() * height();

Why not use the existing HTMLCanvasElement::memoryCost() ?

I think this should also only do decrease the number if there is a m_imageBuffer.

> Source/WebCore/html/HTMLCanvasElement.cpp:195
> +    return ramSize() / (4 * 8);

Why?

> Source/WebCore/html/HTMLCanvasElement.cpp:219
> +            size_t requestedPixels = width() * height();
> +            if (numActivePixels + requestedPixels > maxActivePixels())

Use the same math as memoryCost().
Comment 4 Brent Fulgham 2015-12-03 15:13:06 PST
Comment on attachment 266563 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:109
>> +        unsigned pixelsReleased = width() * height();
> 
> Why not use the existing HTMLCanvasElement::memoryCost() ?
> 
> I think this should also only do decrease the number if there is a m_imageBuffer.

memoryCost only returns a value once the image buffer is created. I want to know if I'm going to exceed the limit before then.

>> Source/WebCore/html/HTMLCanvasElement.cpp:195
>> +    return ramSize() / (4 * 8);
> 
> Why?

It's arbitrary, but based on the following thoughts: Blink limits the process to 1 GB. Assuming our typical-to-lower-end systems are using 8 GB, this calculation gives the number of 32 bit pixels that match that 1 GB limit.

>> Source/WebCore/html/HTMLCanvasElement.cpp:219
>> +            if (numActivePixels + requestedPixels > maxActivePixels())
> 
> Use the same math as memoryCost().

To use memoryCost we need to create the image buffer first.
Comment 5 Brent Fulgham 2015-12-03 15:15:11 PST
Comment on attachment 266563 [details]
Patch

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

>>> Source/WebCore/html/HTMLCanvasElement.cpp:219
>>> +            if (numActivePixels + requestedPixels > maxActivePixels())
>> 
>> Use the same math as memoryCost().
> 
> To use memoryCost we need to create the image buffer first.

... but, I could use memoryCost once the canvas context is created (and memoryCost when the context is destroyed) to keep track of actual memory use. That way, I don't improperly mark memory in use for a canvas that didn't actually use memory (if such things ever happen).
Comment 6 Geoffrey Garen 2015-12-03 15:32:13 PST
Comment on attachment 266563 [details]
Patch

I think we should record explicitly the number of pixels we added to numActivePixels so that we subtract that number back later. Relying on width and height seems risky, since they can change.
Comment 7 Simon Fraser (smfr) 2015-12-03 15:38:21 PST
(In reply to comment #6)
> Comment on attachment 266563 [details]
> Patch
> 
> I think we should record explicitly the number of pixels we added to
> numActivePixels so that we subtract that number back later. Relying on width
> and height seems risky, since they can change.

size changes are handled via setSurfaceSize(), and this code needs to account for them.
Comment 8 Brent Fulgham 2015-12-03 17:19:08 PST
Created attachment 266580 [details]
Patch
Comment 9 Build Bot 2015-12-03 17:49:55 PST
Comment on attachment 266580 [details]
Patch

Attachment 266580 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/513362

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2015-12-03 17:49:58 PST
Created attachment 266583 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Simon Fraser (smfr) 2015-12-03 17:54:55 PST
Comment on attachment 266580 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:618
> +    if (m_imageBuffer)
> +        removeFromActivePixelMemory(memoryCost());

You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it.
Comment 12 Brent Fulgham 2015-12-03 18:11:37 PST
Comment on attachment 266580 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:618
>> +        removeFromActivePixelMemory(memoryCost());
> 
> You could tie the book-keeping more closely with the buffer allocation/deallocation by adding HTMLCanvasElement::setImageBuffer(), and doing the add/remove pixel count stuff inside it.

Sure!
Comment 13 Brent Fulgham 2015-12-04 09:58:08 PST
Note that the actual memory cost of canvas elements is difficult to assess, because there are code paths where CanvasRenderingContext2D creates and returns blank image buffers. So, the actual memory use is more than just the internal image buffer.
Comment 14 Brent Fulgham 2015-12-04 15:27:11 PST
Created attachment 266666 [details]
Patch
Comment 15 Simon Fraser (smfr) 2015-12-04 15:43:53 PST
Comment on attachment 266666 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:123
> +    // Do not manually delete the image buffer -- it will be cleaned up by the JSC collector.
> +    removeFromActivePixelMemory(memoryCost());
> +
>      m_context = nullptr; // Ensure this goes away before the ImageBuffer.
>  }

Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.
Comment 16 Brent Fulgham 2015-12-04 15:51:20 PST
Comment on attachment 266666 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>  }
> 
> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.

For the same reason as the comment in 'getContext':
    // FIXME: The code depends on the context not going away once created, to prevent JS from
    // seeing a dangling pointer. So for now we will disallow the context from being changed"

If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.

I can add a comment to that effect.

> Source/WebCore/html/HTMLCanvasElement.cpp:204
> +        maxPixelMemory = std::max(ramSize() / 4, 1024 * MB);

I started off with ramSize() / 8, but some of our tests want to allocate more than that. (ramSize() / 8) + 1 seems to work, but this might be a slightly less drastic change from our current "allocate whatever you want" approach.

Some test machines are resource constrained, so I set a lower bound on the max memory.
Comment 17 Brent Fulgham 2015-12-04 15:56:47 PST
Comment on attachment 266666 [details]
Patch

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

>>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>>  }
>> 
>> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.
> 
> For the same reason as the comment in 'getContext':
>     // FIXME: The code depends on the context not going away once created, to prevent JS from
>     // seeing a dangling pointer. So for now we will disallow the context from being changed"
> 
> If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.
> 
> I can add a comment to that effect.

Oh! Yes, your way works perfectly. Much better -- I'll change it.
Comment 18 Brent Fulgham 2015-12-04 15:59:16 PST
Created attachment 266674 [details]
Patch
Comment 19 Simon Fraser (smfr) 2015-12-04 16:05:05 PST
Comment on attachment 266674 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:608
> +        stringBuilder.appendLiteral("Total canvas memory use exceeds the maximum limit ( ");

No space after the paren in the string.

> Source/WebCore/html/HTMLCanvasElement.cpp:611
> +        document().addConsoleMessage(MessageSource::JS, MessageLevel::Warning, stringBuilder.toString());

Does this string look OK in the console?
Comment 20 Build Bot 2015-12-04 16:25:10 PST
Comment on attachment 266674 [details]
Patch

Attachment 266674 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/517210

Number of test failures exceeded the failure limit.
Comment 21 Build Bot 2015-12-04 16:25:14 PST
Created attachment 266678 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 22 Brent Fulgham 2015-12-04 16:40:52 PST
Comment on attachment 266666 [details]
Patch

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

>>>> Source/WebCore/html/HTMLCanvasElement.cpp:123
>>>>  }
>>> 
>>> Why not setImageBuffer(nullptr) at the end of the function, so you don't need to call removeFromActivePixelMemory() explicitly.
>> 
>> For the same reason as the comment in 'getContext':
>>     // FIXME: The code depends on the context not going away once created, to prevent JS from
>>     // seeing a dangling pointer. So for now we will disallow the context from being changed"
>> 
>> If I call 'setImageBuffer(nullptr)' here, JSC gets very upset that the image buffer went away without it's control.
>> 
>> I can add a comment to that effect.
> 
> Oh! Yes, your way works perfectly. Much better -- I'll change it.

Wait -- this is still failing on the debug test bots due to the GC being angry about the ImageBuffer going missing. I'll switch back to my previous version and retry the test.
Comment 23 Brent Fulgham 2015-12-04 16:42:30 PST
Created attachment 266681 [details]
Patch
Comment 24 Brent Fulgham 2015-12-04 17:10:58 PST
Created attachment 266686 [details]
Patch
Comment 25 Brent Fulgham 2015-12-04 17:12:56 PST
Comment on attachment 266686 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:121
> +    releaseImageBufferAndContext();

Maybe name this 'releaseImageBufferAndContextStateSaver"?
Comment 26 Brent Fulgham 2015-12-04 17:23:25 PST
(In reply to comment #22)
> > Oh! Yes, your way works perfectly. Much better -- I'll change it.
> 
> Wait -- this is still failing on the debug test bots due to the GC being
> angry about the ImageBuffer going missing. I'll switch back to my previous
> version and retry the test.

Okay -- as we discussed in person, this was due to the graphics context state saver not being cleared when the ImageBuffer was deallocated. This is now fixed, and with that everything works properly.
Comment 27 Brent Fulgham 2015-12-04 17:44:53 PST
Comment on attachment 266686 [details]
Patch

Actually rubber-stamped by Simon Fraser.
Comment 28 Brent Fulgham 2015-12-04 17:45:25 PST
Resetting the r+ based on Simon's earlier review (plus in-person discussion of final fix) to allow webkit-patch to do its thing.
Comment 29 Brent Fulgham 2015-12-04 17:45:48 PST
Committed r193500: <http://trac.webkit.org/changeset/193500>
Comment 30 Ryan Haddad 2015-12-07 15:32:38 PST
This change seems to be causing the following layout test to fail on El Capitan WK2 Debug:

fast/canvas/canvas-too-large-to-draw.html

<https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r193649%20(1513)/results.html>
Comment 31 WebKit Commit Bot 2015-12-10 10:02:07 PST
Re-opened since this is blocked by bug 152143
Comment 32 Brent Fulgham 2015-12-18 09:51:09 PST
The test bots that were failing are 8 GB Mac Minis. This means that we set the upper limit of memory use to 2 GB.

These large canvas tests use ~268 MB canvas sizes. Even if we made two of them that should only reach about 0.5 GB of memory use.

I think the issue is that the canvas elements from previous test runs remain in the JSC heap until a garbage collection runs. This test case gets hit in the midst of a number of canvas tests, which consume enough “pixel memory” that we can’t allocate the gargantuan canvas and we fail the test.
Comment 33 Brent Fulgham 2015-12-18 10:07:51 PST
Wait. I can't do math.

The tests try to allocate:
(1) a 16384 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,741,824 bytes.
(2) a 16385 x 16384 pixel canvas, which consumes (at 4 bytes per pixel) = 1,073,807,360 bytes.

On an 8 GB machine, we allow 2 GB of RAM to be used: 2,147,483,648 bytes.

(1) + (2) = 2,147,549,184 which exceeds the allowed memory.

I'll adjust the allowed memory minimum to handle this case.
Comment 34 Brent Fulgham 2015-12-18 10:20:57 PST
Created attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)
Comment 35 Brent Fulgham 2015-12-18 10:24:37 PST
Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)

This is a small update to satisfy 'fast/canvas/canvas-stroke-path.html'. I'm uploading it for "review" so that I can get EWS confirmation on a wider range of hardware before I re-land.
Comment 36 Brent Fulgham 2015-12-18 11:06:53 PST
Tests are green. I'm relanding the change.
Comment 37 Brent Fulgham 2015-12-18 13:29:19 PST
Committed r194290: <http://trac.webkit.org/changeset/194290>
Comment 38 Brent Fulgham 2015-12-18 14:12:32 PST
Comment on attachment 267638 [details]
Patch (Revised with memory limit bumped to satisfy test criteria)

Clearing review flag now that this is landed.