Closed Bug 863958 Opened 7 years ago Closed 7 years ago

ICO decoder assumes you can never have a write after you get the size [@mozilla::image::ImageMetadata::SetSize]

Categories

(Core :: ImageLib, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- fixed
firefox23 --- fixed

People

(Reporter: posidron, Assigned: joe)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files)

Attached image testcase
Tested with m-i changeset: 129367:9ff5c0134bbf
Attached file callstack
Crash Signature: [@ mozilla::image::ImageMetadata::SetSize] → [@ void mozilla::Maybe<nsIntSize>::construct<nsIntSize>] [@ mozilla::image::ImageMetadata::SetSize]
Assignee: nobody → joe
Summary: ICO crash [@mozilla::image::ImageMetadata::SetSize] → ICO decoder assumes you can never have a write after you get the size [@mozilla::image::ImageMetadata::SetSize]
In this case the ICO decoder's logic was just plain wrong; it assumed it could call SetSize as willy-nilly as it liked, which isn't the case. If we happened to have a sufficiently-large ICO, we'd get this assertion failure.

https://tbpl.mozilla.org/?tree=Try&rev=9663d8686bc0
Attachment #740386 - Flags: review?(seth)
Attached patch crashtestSplinter Review
This is Christoph's testcase.
Attachment #740387 - Flags: review?(seth)
Another try at try, since we had infra problems that gave us lots of false timeouts: https://tbpl.mozilla.org/?tree=Try&rev=d3f8ea4f8696
Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

Review of attachment 740386 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.
Attachment #740386 - Flags: review?(seth) → review+
Ironically this patch has almost identical code to the patch for bug 858818, which didn't get checked in because I was having trouble producing a working test case. I'm glad that we've got one in this bug. I'll dupe that bug to this one.
Duplicate of this bug: 858818
Comment on attachment 740387 [details] [diff] [review]
crashtest

Review of attachment 740387 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm a bit puzzled as to why I couldn't reproduce this in the .png I generated for the other bug. I had a much bigger one (more than 10 times the size!) but it still didn't work. Strange. At any rate, it's great that we have a test case for this now!
Attachment #740387 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/8658d3607e1a
https://hg.mozilla.org/mozilla-central/rev/506e1fb34b30
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140
User impact if declined: Assertion failures in debug builds
Testing completed (on m-c, etc.): On m-c for a couple of days
Risk to taking this patch (and alternatives if risky): Not risky.
String or IDL/UUID changes made by this patch: none
Attachment #740386 - Flags: approval-mozilla-aurora?
Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

In support of developers, approving for uplift to Aurora.
Attachment #740386 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.