Closed Bug 856616 Opened 7 years ago Closed 6 years ago

GIF Assertion failure: !IsSizeDecode() (Size decodes shouldn't reach gif_done) [@mozilla::image::nsGIFDecoder2::WriteInternal]

Categories

(Core :: ImageLib, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: posidron, Assigned: milan)

References

Details

(Keywords: crash, testcase)

Attachments

(4 files, 3 obsolete files)

Attached image testcase
No description provided.
Attached file callstack (obsolete) —
Is there a point of doing PostDecode... if there was no image in the first place?
Assignee: nobody → milan
Attachment #731919 - Flags: review?(joe)
Before reviewing, see conversation in bug 857623
Comment on attachment 731919 [details] [diff] [review]
We're not covering the scenario where there are no images in the container.

Joe, can you confirm that PostDecodeDone should never be called for the size decode case, thus the assert?
Attachment #731919 - Flags: review?(joe)
The solution to this and bug 857623 is the same, the test cases are different and trigger different reasons for hitting it.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 857623
The testcase still works after patch in bug 857623 landed.

Assertion failure: !IsSizeDecode() (Size decodes shouldn't reach gif_done), at /Users/cdiehl/dev/repos/mozilla/mozilla-inbound/image/decoders/nsGIFDecoder2.cpp:1038
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Keywords: testcase
Summary: GIF ABORT: Can't be done with decoding with size decode!: '!IsSizeDecode()' [@mozilla::image::Decoder::PostDecodeDone] → GIF Assertion failure: !IsSizeDecode() (Size decodes shouldn't reach gif_done) [@mozilla::image::nsGIFDecoder2::WriteInternal]
Attached file callstack
Attachment #731872 - Attachment is obsolete: true
See Also: → CVE-2013-1691
This is still reproducible with Nightly.
Right - we're still not covering the case where there are no images in the container.  Seth, the assertion we kept from bug 857623 is correct when there is an image to decode; if we go straight from global header/global colormap to GIF_TRAILER/gif_done, we hit this assertion. Now, that assertion is useful in the other scenarios, so I'd rather not just remove it.  Seth, Joe, opinions?
Flags: needinfo?(seth)
Flags: needinfo?(joe)
The easiest way is to just remove the assert. The more correct way is to PostSize() when we get the gif_global_header, and just return then.
Flags: needinfo?(joe)
My preference is to fix the code correctly, but I'll accept removing the assert (if we really want that fixed) and filing a follow-up bug to fix the code and restore the assert.
Is this the direction you had in mind?
Attachment #774800 - Flags: feedback?(joe)
Flags: needinfo?(seth)
Comment on attachment 774800 [details] [diff] [review]
PostSize in the global header, no need to continue when doing size decode

Yep!
Attachment #774800 - Flags: feedback?(joe) → feedback+
The code is the same as in the (obsolete) patch f+ by Joe; this one just adds the test.
Attachment #774800 - Attachment is obsolete: true
Attachment #807415 - Flags: review?(seth)
Comment on attachment 807415 [details] [diff] [review]
PostSize in the global header, no need to continue when doing size decode.

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

Looks good.
Attachment #807415 - Flags: review?(seth) → review+
Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=424345dcad4e.  Will do a trivial rebase and attach the new patch.
https://hg.mozilla.org/mozilla-central/rev/e0cb62be1f04
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 989687
You need to log in before you can comment on or make changes to this bug.