Closed
Bug 963223
Opened 11 years ago
Closed 3 years ago
SVG: stack-buffer-overflow / OOM crash [@unw_init_local/mozilla::gfx::FlattenedPath::LineTo]
Categories
(Core :: Graphics, defect, P2)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: posidron, Assigned: bas.schouten)
References
Details
(5 keywords)
Attachments
(5 files)
Takes a few minutes to trigger. With my MacOS build the testcase results in a stack-buffer-overflow crash (read) while on Linux in an OOM crash.
Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/c0df37c18e54
Reporter | ||
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
This might be a false positive, my sec-rating might not be appropriate.
Reporter | ||
Updated•11 years ago
|
Summary: SVG: stack-buffer-overflow / OOM crash [@unw_init_local] → SVG: stack-buffer-overflow / OOM crash [@unw_init_local/mozilla::gfx::FlattenedPath::LineTo]
Comment 3•11 years ago
|
||
I see this crash on Windows 7 too. Bas might know about this. I saw a stack overflow with the original patch for bug 941585 although stack looks somewhat different.
Flags: needinfo?(bas)
OS: Mac OS X → All
Comment 4•11 years ago
|
||
Regression window on Linux64: 2013-11-17-03-02-03 -- 2013-11-18-03-02-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=beddd6d4bcdf
This looks like a possible candidate:
changeset: 155112:f2e964f10799
user: Bas Schouten <bschouten@mozilla.com>
date: Mon Nov 11 12:42:07 2013 +1300
summary: Bug 939049 - Part 2: Add generic ComputeLength code for backends with no such functionality. r=jrmuizel
It seems the bug number in that summary is wrong though.
Keywords: regression,
testcase
Priority: -- → P2
Comment 5•11 years ago
|
||
Actually, one of these is probably the more likely culprit:
changeset: 155121:af0931327e49
user: Jonathan Watt <jwatt@jwatt.org>
date: Mon Nov 18 01:29:06 2013 +0000
summary: Bug 938388 - Convert all remaining code for calculating path lengths and position at an offset along a path in content/svg to Moz2D (kill off all uses of gfxPath). r=dholbert
changeset: 155120:4f086025350f
user: Jonathan Watt <jwatt@jwatt.org>
date: Mon Nov 18 01:29:06 2013 +0000
summary: Bug 930577 - Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D. r=heycam
Flags: needinfo?(jwatt)
Comment 6•11 years ago
|
||
Seems like there may still be some degeneracies that Bas's algo doesn't handle. Bas, can you take a look at this one?
Flags: needinfo?(jwatt)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #6)
> Seems like there may still be some degeneracies that Bas's algo doesn't
> handle. Bas, can you take a look at this one?
I'll have a look later, in the meanwhile could somebody post the stack overflow's stack trace?
Flags: needinfo?(bas)
Comment 8•11 years ago
|
||
On Linux, it's an infinite loop. I added this printf and get:
t=0.000361 s3=-1020.856873 aTolerance=0.000100 currentCP.mCP1=-45533196.000000,21729924.000000 currentCP.mCP2=-45534984.000000,21727158.000000 currentCP.mCP3=-45535716.000000,21724392.000000 currentCP.mCP4=-45530196.000000,21721626.000000
The values does not change... let me know if you need more data.
Flags: needinfo?(bas)
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #8)
> Created attachment 8365682 [details] [diff] [review]
> debug
>
> On Linux, it's an infinite loop. I added this printf and get:
>
> t=0.000361 s3=-1020.856873 aTolerance=0.000100
> currentCP.mCP1=-45533196.000000,21729924.000000
> currentCP.mCP2=-45534984.000000,21727158.000000
> currentCP.mCP3=-45535716.000000,21724392.000000
> currentCP.mCP4=-45530196.000000,21721626.000000
>
> The values does not change... let me know if you need more data.
For now, no, that's very interesting! Thank you.
Flags: needinfo?(bas)
Comment 10•11 years ago
|
||
During Critsmash we thought this should be lowered to sec-high. Bas?
Keywords: sec-critical → sec-high
Updated•11 years ago
|
Flags: needinfo?(bas)
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to David Bolter [:davidb] from comment #10)
> During Critsmash we thought this should be lowered to sec-high. Bas?
I agree, I don't see any obvious ways this is exploitable since it seems to be a stack overflow from what I read here. I'll look into this soon.
Flags: needinfo?(bas)
Updated•11 years ago
|
Component: SVG → Graphics
Assignee | ||
Comment 13•11 years ago
|
||
I looked at this a little, but not in any great detail, it has some SVG and JS stuff in there that I'm not completely sure what it will do. I still think this is not exploitable. When JWatt comes back, or somebody else, could somebody provide me with a more reduced, straight-forward test case that causes a path that has this issue? Fwiw, this causes an OOM on Windows too. If it's not reducible any further I'll try and figure out from the stack I have currently.
Flags: needinfo?(bas)
Updated•11 years ago
|
Group: gfx-core-security
Comment 14•11 years ago
|
||
JWatt, wondering if you might have a reduced test case idea? (Bas is wanting one in comment 13).
Flags: needinfo?(jwatt)
Comment 15•10 years ago
|
||
Flags: needinfo?(jwatt)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
Comment 17•10 years ago
|
||
Bas, you mentioned in comment 13 that you felt that this was probably not exploitable. The CritSmash team wanted to know if you could take one last look. If this is the case, we'd probably change the security rating, but we'd feel more comfortable with your input. Thank you.
Flags: needinfo?(bas)
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Matt Wobensmith from comment #17)
> Bas, you mentioned in comment 13 that you felt that this was probably not
> exploitable. The CritSmash team wanted to know if you could take one last
> look. If this is the case, we'd probably change the security rating, but
> we'd feel more comfortable with your input. Thank you.
Yeah, I really don't see how this could be exploitable in any way. We simply get stuck in a loop, allocate more and more memory, and then crash.
I should fix this anyway, so it's good you drew my attention to it :)
Flags: needinfo?(bas)
Assignee | ||
Comment 19•10 years ago
|
||
So the problem here is we're using numbers that are so big, that single precision floating point can't represent them very well.
I believe SVG doesn't guarantee double precision, so this is probably okay. But we shouldn't crash, there's a couple of things we could do here:
1. Force the algorithm along, i.e. if (t == oldt) t += epsilon;
2. Cap coordinates at some value somewhere
3. Bail if the algorithm notes it doesn't have precision to continue i.e. if (t == oldt) return;
4. Suggestions welcome...
Do you have a preference Jonathan?
Flags: needinfo?(jwatt)
Comment 20•10 years ago
|
||
I don't really mind, as long as we have a debug assertion or something that makes in quick to locate the issue if someone files a bug about content mis-rendering.
Flags: needinfo?(jwatt)
Updated•10 years ago
|
Comment 21•10 years ago
|
||
s/assertion/warning
Updated•9 years ago
|
Group: core-security
Assignee | ||
Comment 22•3 years ago
|
||
I cannot reproduce this, I suspect we fixed this elsewhere.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Comment 23•3 years ago
|
||
It seems likely bug 1322537 fixed this, which also landed crashtests.
https://searchfox.org/mozilla-central/rev/f213971fbd82ada22c2c4e2072f729c3799ec563/gfx/2d/Path.cpp#244-254
Depends on: 1322537
Updated•1 year ago
|
Group: gfx-core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•