Closed
Bug 1322112
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::image::nsIconDecoder::ReadRowOfPixels
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: martin, Assigned: martin, Mentored)
References
Details
Attachments
(1 file)
558 bytes,
patch
|
tnikkel
:
review+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
When starting firefox 50.0.2 on sparc64 I get an immediate crash like this: Thread 2 received signal SIGBUS, Bus error. [Switching to LWP 23] mozilla::image::nsIconDecoder::ReadRowOfPixels (this=0xffffffffd288ac00, aData=aData@entry=0xffffffffcf65d002 "", aLength=aLength@entry=80) at /usr/pkgobj/www/firefox/work/firefox-50.0.2/image/decoders/nsIconDecoder.cpp:105 105 }); (gdb) bt #0 mozilla::image::nsIconDecoder::ReadRowOfPixels (this=0xffffffffd288ac00, aData=aData@entry=0xffffffffcf65d002 "", aLength=aLength@entry=80) at /usr/pkgobj/www/firefox/work/firefox-50.0.2/image/decoders/nsIconDecoder.cpp:105 #1 0xffffffffe31ffa4c in mozilla::image::nsIconDecoder::<lambda(mozilla::image::nsIconDecoder::State, char const*, size_t)>::operator()(mozilla::image::nsIconDecoder::State, const char *, size_t) const (__closure=0xffffffffd01ff5c8, aState=<optimized out>, aData=0xffffffffcf65d002 "", aLength=80) at /usr/pkgobj/www/firefox/work/firefox-50.0.2/image/decoders/nsIconDecoder.cpp:41 #2 0xffffffffe3205d30 in mozilla::image::StreamingLexer<mozilla::image::nsIconDecoder::State, 16ul>::BufferedRead<mozilla::image::nsIconDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::<lambda(mozilla::image::nsIconDecoder::State, char const*, size_t)> > (aFunc=..., aIterator=..., this=0xffffffffd288ad50) at /usr/pkgobj/www/firefox/work/firefox-50.0.2/image/StreamingLexer.h:615 #3 mozilla::image::StreamingLexer<mozilla::image::nsIconDecoder::State, 16ul>::Lex<mozilla::image::nsIconDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*)::<lambda(mozilla::image::nsIconDecoder::State, char const*, size_t)> >(mozilla::image::SourceBufferIterator &, mozilla::image::IResumable *, mozilla::image::nsIconDecoder::<lambda(mozilla::image::nsIconDecoder::State, char const*, size_t)>) (this=this@entry=0xffffffffd288ad50, aIterator=..., aOnResume=aOnResume@entry=0xffffffffd2847b00, aFunc=...) Note the aData pointer is not 4-byte aligned, but the code does: uint32_t pixel = *reinterpret_cast<const uint32_t*>(aData); aData += 4; aLength -= 4; I don't fully understand how aData is initialized in the closure below this call and whether this is a situation that is allowed to happen (in which case the obvious fix is memcpy() instead of *reinterpret_cast<>()) or should never happen, in which case the bug is hiding somewhere else.
Comment 1•8 years ago
|
||
memcpy looks like the correct to do here; can you test that patch?
Flags: needinfo?(martin)
Assignee | ||
Comment 2•8 years ago
|
||
Use memcpy() instead of dereferencing a casted pointer where the cast would increase alignement requirements on some architectures
Flags: needinfo?(martin)
Assignee | ||
Comment 3•8 years ago
|
||
I have been running with that patch for two days and FF 50.0.2 is happy on my sparc64 desktop machine.
Comment 4•8 years ago
|
||
Comment on attachment 8817015 [details] [diff] [review] patch-image_decoders_nsIconDecoder.cpp Yeah, this is the right fix. This icon format (that is internal to gecko) uses one byte each for the width and height at the start of the data. This is probably silly because it limits icons to 256x256 and means that every following row is not 4 byte aligned (if the start of the data is 4 byte aligned). Andrew mentioned a possible endian-ness issue, which I looked into. The platform specific code that gets the icon image data from the OS handles this and puts it into the expected order. Ex https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/image/decoders/icon/gtk/nsIconChannel.cpp#72
Attachment #8817015 -
Flags: review+
Updated•8 years ago
|
Assignee: nobody → martin
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dad797ebc115350cfd14a63ef9411d9daf9d5d4e
Pushed by tnikkel@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd9bb569ce9 Use memcpy() instead of dereferencing a casted pointer where the cast would increase alignement requirements on some architectures in nsIconDecoder. r=tnikkel
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5fd9bb569ce9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817015 [details] [diff] [review] patch-image_decoders_nsIconDecoder.cpp [Approval Request Comment] ESR consideration: Improve stability on architectures with strict alignment requirement User impact if declined: Startup crash on sparc64 Fix Landed on Version: 53.0 Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None
Attachment #8817015 -
Flags: approval-mozilla-esr52?
Comment 10•7 years ago
|
||
Comment on attachment 8817015 [details] [diff] [review] patch-image_decoders_nsIconDecoder.cpp alignment fix, for esr52.1.0
Attachment #8817015 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/01f58e0bfe9d
status-firefox-esr52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•