1346610 - HTML doc triggers runaway CPU usage in Firefox Nightly (DoS threat)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Carpenter [:geeknik]]

Attachment: 100% CPU Usage HTML doc

While fuzzing Firefox Nightly, I discovered this HTML doc that triggers runaway CPU usage, thus posing a DoS threat. Memory usage is pretty flat, but CPU is pegged at 100% and the browser stops responding. Tested with Windows Nightly 55.0a1 Build ID: 20170311030203 (dirty profile + extensions + e10s), ASAN Nightly 55.0a1 Build ID 20170311030203 (clean profile + no extensions + no e10s) and Firefox 45.3.0 ESR Build ID 20160802213348 (clean profile + no extensions). I left the browser running with this page loaded for ~30 minutes and it did not result in a crash.

Comment 1

[Brian Carpenter [:geeknik]]

Attachment: Plaintext that won't lock up your browser. (;

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Henri, are you the right person to take a look at this to see what's up here? Thanks!

Comment 3

[Henri Sivonen (:hsivonen)]

I'll take a look.

Comment 4

[Henri Sivonen (:hsivonen)]

The slowness seems to be in the SVG code, not in the parser.

Comment 5

[violet.bugreport]

After some investigation, the fuzzing isn't the essence, it's actually caused by many nested svg elements.

A reduced testcase would be simply:

$ printf '%.s<svg>' {1..64} > evilsvg.html

The cause is some exponentially wasted computation, I will submit a patch.

Comment 6

[violet.bugreport]

Attachment: Bug 1346610 - Compute width and height only when necessary to avoid exponential time complexity

When only width is needed, we should not compute height, and vice versa. Otherwise
there are exponentailly repeated computations for nested svg elements, which will
make the tab unresponsive due to 100% CPU usage for exponentailly long time.

Comment 7

[violet.bugreport]

Hi Jonathan,

I've submitted 3 patches (including this one) related to SVG component a week ago, would you mind reviewing?

Thanks!

Comment 8

[Robert Longson [:longsonr]]

If Jonathan doesn't respond, I can review them if you want me to. Just change the reviewer to me.

Comment 9

[violet.bugreport]

I've changed the reviewer for this bug to you since you've already reviewed. Could you also assign this bug to me since I don't have the privilege to edit?

Comment 10

[Robert Longson [:longsonr]]

Do you have try server access? have you sent this to try? I suspect the testcase will work on Desktop platforms but fail on mobile. Do you need me to send it to the try server for you.

Otherwise I'd give it an r+, I'm only hesitant to do so because it might get backed out for timing out on mobile.

Comment 11

[violet.bugreport]

Do you have try server access? have you sent this to try?

I don't have any access because I just registered here ~10 days ago.

I suspect the testcase will work on Desktop platforms but fail on mobile.

After the patch, the code has O(N^2) complexity, so I guess 128 is not a big value. On my desktop the render completes instantly, but I don't have a mobile dev environment to verify on mobile platform.

What's the timeout requirement for mochitest? Do you think I need to reduce the test to 64 before sending to try server?

Comment 12

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d07937ebcbf5c2c269505f980dd46cb2975da30a

Comment 13

[Robert Longson [:longsonr]]

Quite a number of try failures on several platforms here. Let me know if there's some other patch you want to send to try. You can either

a) reduce the nesting
b) skip platforms entirely with skip_if

Comment 14

[violet.bugreport]

Thanks. I will update the patch later due to some project config issue.

Could you also review this patch of mine? Note that it only has effects when webrender is enabled, otherwise sime previous check will guarantee the bad state won't happen.

Comment 15

[Robert Longson [:longsonr]]

See https://searchfox.org/mozilla-central/source/dom/animation/test/crashtests/crashtests.list#3 for how to enable prefs in a crashtest.

Comment 16

[violet.bugreport]

I have reduced the depth to 32, I hope it won't timeout anymore. Actually with 32, the original code can complete within minutes on my desktop.

Comment 17

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01075548137854ec85571593247e111676e4c31c

Comment 18

[violet.bugreport]

This is surprising. I found there isn't any difference when reducing from 128 to 32, and most of the timeouts are from e10s. Do you think the following skip-if condition is ok?

skip-if = (!e10s || toolkit == 'android' || debug)

There are also some timeout from QuantumRender, I don't know how to skip it.

Comment 19

[Robert Longson [:longsonr]]

It's looking like this patch doesn't really fix this bug. Why not simply raise another bug and we'll land it there without a test as it isn't a functional change.

Comment 20

[violet.bugreport]

I'm quite certain that the root caused is fixed by this patch. After the patch, even 10000 nesting can be rendered within 6 seconds on one of my very old linux desktop.

I didn't know before what e10s is, so I just searched it, it turns out to be the multiprocess windows in about:support which is already enabled on my desktop. So I have exactly the same environment as the M-e10s failures on the try server, which took 370s+ for rendering 32 nesting. It's almost unbelievable.

I will have try server access soon per suggestion from a reviewer when fixing another layout bug, so maybe I will give you update after looking at what's going on on the try server.

Comment 21

[violet.bugreport]

I found out the reason. The test should not be put in mochitest directory this way. I've run mochitest on my local machine, the test utility hangs waiting for some JavaScript events. It seems mochitest cannot be used to test svg rendering timeout.

I actually asked the question when submitting the patch, but got no answer.

So I can't put it in mochitest, but it's not a crashtest either. Do you know where to put timeout test?

Comment 22

[Robert Longson [:longsonr]]

I don't know. You could try asking in https://groups.google.com/forum/#!forum/mozilla.dev.platform if that's the wrong place someone should at least tell you the right place.

Comment 23

[violet.bugreport]

Per response from my question, the correct place for timeout test is crashtest. I've updated the diff, please check, now it should be ok.

Comment 24

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0c702f22e1dfa2bb9d4ed59fa494ed41bead766

FWIW there's no need to use needinfo if you submiy a new patch and I'm the reviewer, you're just sending me 2 emails each time instead of one.

Comment 25

[violet.bugreport]

Sorry I'm not aware of the email to the reviewer when I submit a new patch.

So my patch already fixed the problem according to your test result. All of the failures are from other irrelevant components and tests like media and devtool, which has nothing to do with this bug. And they are known intermittent failure as they have their own bug id.

Would you accept the patch now?

Comment 26

[Robert Longson [:longsonr]]

Sorry for the delay, seems like I needed to press submit but didn't.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/e1459b0afa258f53b7799447c063654e994e4d7f

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e1459b0afa25