Closed Bug 639303 Opened 14 years ago Closed 14 years ago

FF4 crashes on multipart image/motion jpeg stream changing image size [@ @0x0 | mozilla::imagelib::Decoder::Finish() ]

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla6
Tracking Flags:
Tracking Status
firefox5 + fixed
firefox6 + fixed
blocking2.0 --- -
status2.0 --- .x-fixed
blocking1.9.2 --- .18+
status1.9.2 --- .18-fixed

People

(Reporter: bugzilla, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

(Keywords: crash, reproducible, verified1.9.2, Whiteboard: [sg:critical][bug 638018 is fixing this on the 1.9.2 branch])

Crash Data

Attachments

(5 files)

stacks
12.08 KB, text/plain
Details
stack2
6.13 KB, text/plain
Details
fix v1
11.69 KB, patch
joe
: review+
johnath
: approval-mozilla-aurora+
dveditz
: approval2.0+
Details | Diff | Splinter Review
WriteToDecoder null-ptr crash
5.28 KB, text/plain
Details
mochitest.diff
3.43 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.15) Gecko/20110303 Firefox/3.6.15 Build Identifier: Nightly Build 5.March 2011 downloaded from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-4.0b13pre.en-US.mac.dmg Web page containing UI elements (select box). Changing the select box element will remotely (on the server) start a CGI script and set the relevant parameter. Something crashes FF4. Reproducible: Always Steps to Reproduce: 1. Open URL http://appcam.mobotixcam.de:2080 and authorize. Authorization Credentials (type without the quotes!) user : "user" password: "Firefox" 2. From select box (the one in the middle) choose "Image Size" 3. From select box (the one on the right) choose "Mega" or "VGA", whatever is not selected Actual Results: FF4 crashes. Expected Results: Image size is set to the selected size.
Version: unspecified → Trunk
Crashes at least on Windows XP and Mac Os 10.6.x
OS: Mac OS X → All
Confirmed on: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre bp-55cbacf9-17fb-4ee2-a162-952a32110306
Status: UNCONFIRMED → NEW
Component: General → ImageLib
Ever confirmed: true
Keywords: crash, reproducible
Product: Firefox → Core
QA Contact: general → imagelib
Summary: FF4 crashes (involving: simple javascript remote call) → FF4 crashes (involving: simple javascript remote call) [@ @0x0 | mozilla::imagelib::Decoder::Finish() ]
The problem might not be directly connected to using Javascript for simple remote scripting. It might by that changing the image size alone just confuses image rendering.
Confirmed on Mac OS X 10.5 with last night's nightly; it hung my browser for a really, really, really long time, then the browser eventually crashed with a SIGBUS. This is a readily-reproduceable crash. Nominating for blocking. This may also be a dupe of bug 623187. Before the crash, it's was hung on thread 1, in mozilla::imagelib::Decoder::PostDataError(): (gdb) thread apply all backtrace Thread 40 (process 4959 thread 0x16bcf): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x96011c62 in TSWaitOnConditionTimedRelative () #4 0x96032a60 in MPWaitOnQueue () #5 0x95f6bdb4 in TFolderSizeTask::FolderSizeTaskProc () #6 0x96030fbb in PrivateMPEntryPoint () #7 0x973f3155 in _pthread_start () #8 0x973f3012 in thread_start () Thread 39 (process 4959 thread 0x16a03): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 38 (process 4959 thread 0x1aa57): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x974199f8 in pthread_cond_timedwait$UNIX2003 () #3 0x0004ff77 in pt_TimedWait () #4 0x00050d17 in PR_WaitCondVar () #5 0x02bc9c5c in nsHostResolver::GetHostToLookup () Previous frame inner to this frame (gdb could not unwind past this frame) Thread 37 (process 4959 thread 0x12a2f): #0 0x973f29c6 in kevent () #1 0x93daae5f in __monitor_file_descriptor__ () #2 0x973f3155 in _pthread_start () #3 0x973f3012 in thread_start () Thread 36 (process 4959 thread 0x129f7): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x96032de3 in TSWaitOnCondition () #4 0x96011c36 in TSWaitOnConditionTimedRelative () #5 0x96032a60 in MPWaitOnQueue () #6 0x95f69e6a in TNodeSyncTask::SyncTaskProc () #7 0x96030fbb in PrivateMPEntryPoint () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 35 (process 4959 thread 0x1bbe3): #0 0x973c2266 in mach_msg_trap () #1 0x973c9a5c in mach_msg () #2 0x93dd5e7e in CFRunLoopRunSpecific () #3 0x93dd6b04 in CFRunLoopRun () #4 0x95f5ee88 in TFSEventsNotificationTask::FSEventsNotificationTaskProc () #5 0x96030fbb in PrivateMPEntryPoint () #6 0x973f3155 in _pthread_start () #7 0x973f3012 in thread_start () Thread 34 (process 4959 thread 0x1932b): #0 0x973c2266 in mach_msg_trap () #1 0x973c9a5c in mach_msg () #2 0x93dd5e7e in CFRunLoopRunSpecific () #3 0x93dd6b04 in CFRunLoopRun () #4 0x95f5ed0f in TSystemNotificationTask::SystemNotificationTaskProc () #5 0x96030fbb in PrivateMPEntryPoint () #6 0x973f3155 in _pthread_start () #7 0x973f3012 in thread_start () Thread 33 (process 4959 thread 0x6437): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x974199f8 in pthread_cond_timedwait$UNIX2003 () #3 0x0004ff77 in pt_TimedWait () #4 0x00050d17 in PR_WaitCondVar () #5 0x02bc9c5c in nsHostResolver::GetHostToLookup () Previous frame inner to this frame (gdb could not unwind past this frame) Thread 32 (process 4959 thread 0x1acc3): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x23623f87 in unregister_ShockwaveFlash () #4 0x23512768 in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 31 (process 4959 thread 0x19c2b): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x23623f87 in unregister_ShockwaveFlash () #4 0x233da01e in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 30 (process 4959 thread 0x12c17): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x23623f87 in unregister_ShockwaveFlash () #4 0x23512768 in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 29 (process 4959 thread 0xd813): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x23623f87 in unregister_ShockwaveFlash () #4 0x23512768 in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 28 (process 4959 thread 0x1c1f7): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x23623f87 in unregister_ShockwaveFlash () #4 0x23512768 in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 27 (process 4959 thread 0x18a8f): #0 0x973c22ae in semaphore_wait_signal_trap () #1 0x973f42c6 in _pthread_cond_wait () #2 0x97439539 in pthread_cond_wait () #3 0x23623fbf in unregister_ShockwaveFlash () #4 0x232175af in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 26 (process 4959 thread 0x8283): #0 0x973c22ae in semaphore_wait_signal_trap () #1 0x973f42c6 in _pthread_cond_wait () #2 0x97439539 in pthread_cond_wait () #3 0x23623fbf in unregister_ShockwaveFlash () #4 0x232175af in dyld_stub_AECountItems () #5 0x236240ac in unregister_ShockwaveFlash () #6 0x236240f0 in unregister_ShockwaveFlash () #7 0x23624216 in unregister_ShockwaveFlash () #8 0x973f3155 in _pthread_start () #9 0x973f3012 in thread_start () Thread 25 (process 4959 thread 0x156a3): #0 0x973c22c6 in semaphore_timedwait_signal_trap () #1 0x973f42af in _pthread_cond_wait () #2 0x973f5b33 in pthread_cond_timedwait_relative_np () #3 0x936b9dbc in -[NSCondition waitUntilDate:] () #4 0x936b9bd0 in -[NSConditionLock lockWhenCondition:beforeDate:] () #5 0x936b9b35 in -[NSConditionLock lockWhenCondition:] () #6 0x96b436e8 in -[NSUIHeartBeat _heartBeatThread:] () #7 0x93673dfd in -[NSThread main] () #8 0x936739a4 in __NSThread__main__ () #9 0x973f3155 in _pthread_start () #10 0x973f3012 in thread_start () Thread 24 (process 4959 thread 0x15c03): #0 0x974116fa in select$DARWIN_EXTSN () #1 0x93de12ef in __CFSocketManager () #2 0x973f3155 in _pthread_start () #3 0x973f3012 in thread_start () Thread 23 (process 4959 thread 0x12583): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 22 (process 4959 thread 0xd003): #0 0x973c2266 in mach_msg_trap () #1 0x973c9a5c in mach_msg () #2 0x93dd5e7e in CFRunLoopRunSpecific () #3 0x93dd6aa8 in CFRunLoopRunInMode () #4 0x95ac55f8 in HALRunLoop::OwnThread () #5 0x95ac5480 in CAPThread::Entry () #6 0x973f3155 in _pthread_start () #7 0x973f3012 in thread_start () Thread 21 (process 4959 thread 0xa00b): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 20 (process 4959 thread 0x8e03): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 19 (process 4959 thread 0x8b1b): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 18 (process 4959 thread 0x892b): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 17 (process 4959 thread 0x8a0f): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 16 (process 4959 thread 0x8d0b): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 15 (process 4959 thread 0x8303): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 14 (process 4959 thread 0x7d03): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 13 (process 4959 thread 0x6903): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x03721a83 in nsCertVerificationThread::Run () Thread 12 (process 4959 thread 0x6707): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x037202de in nsSSLThread::Run () Cannot access memory at address 0x9 Thread 11 (process 4959 thread 0x6803): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 10 (process 4959 thread 0x590f): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x00050e09 in PR_Wait () #5 0x03a69a9a in nsEventQueue::GetEvent () Cannot access memory at address 0x5 Thread 9 (process 4959 thread 0x5103): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x947e9b32 in glvmDoWork () #4 0x973f3155 in _pthread_start () #5 0x973f3012 in thread_start () Thread 8 (process 4959 thread 0x3803): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x03576a57 in XPCJSRuntime::WatchdogMain () #5 0x973f3155 in _pthread_start () #6 0x973f3012 in thread_start () Thread 7 (process 4959 thread 0x3703): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x03c2d682 in js::GCHelperThread::threadLoop () #5 0x973f3155 in _pthread_start () #6 0x973f3012 in thread_start () Thread 6 (process 4959 thread 0x3603): #0 0x974116fa in select$DARWIN_EXTSN () #1 0x00057d6e in poll () #2 0x00052979 in PR_Poll () #3 0x02bbcbdd in nsSocketTransportService::Poll () Previous frame inner to this frame (gdb could not unwind past this frame) Thread 5 (process 4959 thread 0x3107): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x974199f8 in pthread_cond_timedwait$UNIX2003 () #3 0x0004ff77 in pt_TimedWait () #4 0x00050d17 in PR_WaitCondVar () #5 0x03a6fa23 in TimerThread::Run () Previous frame inner to this frame (gdb could not unwind past this frame) Thread 4 (process 4959 thread 0x200b): #0 0x973c944e in __semwait_signal () #1 0x973f43e6 in _pthread_cond_wait () #2 0x973f3dcd in pthread_cond_wait$UNIX2003 () #3 0x00050db1 in PR_WaitCondVar () #4 0x03a7c08b in nsCycleCollectorRunner::Run () Thread 3 (process 4959 thread 0x2103): #0 0x973f29c6 in kevent () #1 0x03aa5b90 in kq_dispatch () Previous frame inner to this frame (gdb could not unwind past this frame) Thread 2 (process 4959 thread 0x1a03): #0 0x973c2266 in mach_msg_trap () #1 0x973c9a5c in mach_msg () #2 0x02b85aab in google_breakpad::ExceptionHandler::WaitForMessage () #3 0x973f3012 in thread_start () Thread 1 (process 4959 thread 0x20b): #0 0x02cea4b4 in mozilla::imagelib::Decoder::PostDataError () #1 0x02d035bc in mozilla::imagelib::nsJPEGDecoder::WriteInternal () (gdb) info registers eax 0x14e 334 ecx 0xb0a41d2c -1331421908 edx 0x973c944e -1757637554 ebx 0x973f3ded -1757463059 esp 0xb0a41d2c 0xb0a41d2c ebp 0xb0a41da8 0xb0a41da8 esi 0xb0a42000 -1331421184 edi 0x21a4b874 564443252 eip 0x973c944e 0x973c944e <__semwait_signal+10> eflags 0x246 582 cs 0x7 7 ss 0x1f 31 ds 0x1f 31 es 0x1f 31 fs 0x1f 31 gs 0x37 55 (gdb) x /8i $pc 0x973c944e <__semwait_signal+10>: jae 0x973c945e <__semwait_signal+26> 0x973c9450 <__semwait_signal+12>: call 0x973c9455 <__semwait_signal+17> 0x973c9455 <__semwait_signal+17>: pop %edx 0x973c9456 <__semwait_signal+18>: mov 0x96cdc6b(%edx),%edx 0x973c945c <__semwait_signal+24>: jmp *%edx 0x973c945e <__semwait_signal+26>: ret 0x973c945f <asprintf>: push %ebp 0x973c9460 <asprintf+1>: mov %esp,%ebp Finally, I poked around in gdb for several minutes trying get more info (did not modify the running process), and decided to step through the binary - to see if it was looping or what. This is when it crashed: (gdb) thread 1 [Switching to thread 1 (process 4959 thread 0x20b)] 0x02cea4b4 in mozilla::imagelib::Decoder::PostDataError () (gdb) n Single stepping until exit from function _ZN7mozilla8imagelib7Decoder13PostDataErrorEv, which has no line number information. 0x02d035bc in mozilla::imagelib::nsJPEGDecoder::WriteInternal () (gdb) Single stepping until exit from function _ZN7mozilla8imagelib13nsJPEGDecoder13WriteInternalEPKcj, which has no line number information. warning: Got an error handling event: "Cannot access memory at address 0x5". (gdb) Single stepping until exit from function _ZN13nsCOMPtr_baseD2Ev, which has no line number information. warning: Got an error handling event: "Cannot access memory at address 0x5". (gdb) bt #0 0x02ceac93 in mozilla::imagelib::Decoder::Finish () #1 0x2240a130 in ?? () Cannot access memory at address 0x5 (gdb) n Single stepping until exit from function _ZN7mozilla8imagelib7Decoder6FinishEv, which has no line number information. Bus error Unfortunately, the crash reporter did not launch and I didn't get a core dump. My browser was built from http://hg.mozilla.org/mozilla-central/rev/e56ecd8b3a68
blocking2.0: --- → ?
This looks like it could be the same underlying bug as in bug 638018. I can reproduce this crash in a trunk debug build on Linux which makes me happy, because bug 638018 takes hours to reproduce...
Assignee: nobody → matspal
Group: core-security
Whiteboard: [sg:critical?]
Note - if anybody is looking at my debugger output, I snagged the info at the bottom of the backtraces on the wrong thread. Sorry 'bout that.
Attached file stacks —
The PostSize() call in nsJPEGDecoder::WriteInternal() leads to an error because it's a multipart channel and the image has a different size. http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/RasterImage.cpp#907 The DoError() call on line 927 leads to ShutdownDecoder() which nulls out 'mDecoder'; this is the last ref so it's deleted.
Attached file stack2 —
ShutdownDecoder() also leads to a "Improper call to JPEG library in state 202" error with this stack.
This is likely exploitable.
Whiteboard: [sg:critical?] → [sg:critical]
Attached patch fix v1 — — Splinter Review
1. add a stack local strong ref on mDecoder before calling anything that can lead to an error 2. add a few missing null-checks on mDecoder 3. notify the decoder of the error in RasterImage::SetSize() so that it can avoid calling FinishInternal() (which would lead to the JPEG library error in stack2) 4. make RasterImage::SyncDecode() return an error code if there was an error (not necessary to fix the crash but it seems like the right thing to do) Try server results pending...
Attachment #517303 - Flags: review?(bobbyholley+bmo)
Blocks: 623187
Attachment #517303 - Flags: review?(bobbyholley+bmo) → review?(joe)
The problem is not using Javascript to change the image size. The problem is changing the image size while the server is delivering a stream of images to the browser.
Summary: FF4 crashes (involving: simple javascript remote call) [@ @0x0 | mozilla::imagelib::Decoder::Finish() ] → FF4 crashes on multipart image/motion jpeg stream changing image size [@ @0x0 | mozilla::imagelib::Decoder::Finish() ]
It's really painful to minus this as we're so close to shipping here. Talking with devs in triage (Joe and others), the fix is too complicated to take at this time. .x'ing.
blocking2.0: ? → .x+
Are you saying FF4 will be published without this bug being fixed? This will be really painful for some users as "Server Push" (content-type: multipart/x-mixed-replace) is a popular method for webcams to present a motion jpeg stream on a web page. I worry that webcam users will encounter this problem early and often.
> Are you saying FF4 will be published without this bug being fixed? Yes. > I worry that webcam users will encounter this problem early and often. Do webcams often change the image size in mid-stream?
(In reply to comment #14) > > I worry that webcam users will encounter this problem early and often. > > Do webcams often change the image size in mid-stream? Is this a trick question? :-) Yes, they do - if users change either aspect ratio ("display mode") or size of the image. As you see from the webcam above, those parameters are easily accessible.
> Is this a trick question? :-) No. This is the first time I see a multipart jpeg bug (and I've seen a number at this point) where the page included UI to modify the image size...
In any case, you answered my question. "It depends" seems to be the answer.
(In reply to comment #16) > No. This is the first time I see a multipart jpeg bug (and I've seen a number > at this point) where the page included UI to modify the image size... To trigger the bug, the image size has to change mid-stream. It is not relevant whether this is done through UI on the page where the stream is displayed, on another page or even through remote API calls (without UI e.g. using curl, wget).
Comment on attachment 517303 [details] [diff] [review] fix v1 I'm not terribly happy that we have to put more error handling in every decoder, but it's not the end of the world. >diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp >@@ -2248,27 +2256,31 @@ RasterImage::WriteToDecoder(const char * >+ nsRefPtr<Decoder> kungFuDeathGrip = mDecoder; > mInDecoder = PR_TRUE; > mDecoder->Write(aBuffer, aCount); > mInDecoder = PR_FALSE; > > // We unlock the current frame, even if that frame is different from the > // frame we entered the decoder with. (See above.) > if (mFrames.Length() > 0) { > imgFrame *curframe = mFrames.ElementAt(mFrames.Length() - 1); > curframe->UnlockImageData(); > } > >+ if (!mDecoder) >+ return NS_ERROR_FAILURE; >+ This is impossible - we hold a strong reference, and we even dereference mDecoder above. Just drop this null check. > CONTAINER_ENSURE_SUCCESS(mDecoder->GetDecoderError()); > > // Keep track of the total number of bytes written over the lifetime of the > // decoder > mBytesDecoded += aCount; > > return NS_OK; > } >@@ -2406,28 +2418,29 @@ RasterImage::SyncDecode() > rv = WriteToDecoder(mSourceData.Elements() + mBytesDecoded, > mSourceData.Length() - mBytesDecoded); > CONTAINER_ENSURE_SUCCESS(rv); > > // When we're doing a sync decode, we want to get as much information from the > // image as possible. We've send the decoder all of our data, so now's a good > // time to flush any invalidations (in case we don't have all the data and what > // we got left us mid-frame). >+ nsRefPtr<Decoder> kungFuDeathGrip = mDecoder; > mInDecoder = PR_TRUE; > mDecoder->FlushInvalidations(); > mInDecoder = PR_FALSE; > > // If we finished the decode, shutdown the decoder >- if (IsDecodeFinished()) { >+ if (mDecoder && IsDecodeFinished()) { > rv = ShutdownDecoder(eShutdownIntent_Done); > CONTAINER_ENSURE_SUCCESS(rv); > } Don't need this null check. >@@ -2688,17 +2703,17 @@ imgDecodeWorker::Run() > // flush once the whole frame is ready. > if (!image->mHasBeenDecoded) { > image->mInDecoder = PR_TRUE; > image->mDecoder->FlushInvalidations(); > image->mInDecoder = PR_FALSE; > } > > // If the decode finished, shutdown the decoder >- if (image->IsDecodeFinished()) { >+ if (image->mDecoder && image->IsDecodeFinished()) { And I'm pretty sure we don't need this null check either.
Attachment #517303 - Flags: review?(joe) → review+
Mats: is this ready to land now? I'd like to see how it does on trunk soon so we can consider taking the fix on the mozilla2.0 branch for FF4.0.1. And since there are no nightly users on that branch we need all the mozilla-central testing we can get.
blocking2.0: .x+ → ?
status2.0: --- → wanted
If this bug don't crash firefox 3.6.X , i don't think this patch is apropriate for Bug638018.
i Can't reproduce this , the password isn't "Firefox". A new password?
I can reproduce with an other testcase in 4.0 , i will try in 3.6.
Blocks: 584092
(In reply to comment #19) > >@@ -2248,27 +2256,31 @@ RasterImage::WriteToDecoder(const char * > > >+ nsRefPtr<Decoder> kungFuDeathGrip = mDecoder; > > mInDecoder = PR_TRUE; > > mDecoder->Write(aBuffer, aCount); > > mInDecoder = PR_FALSE; > > > > // frame we entered the decoder with. (See above.) > > if (mFrames.Length() > 0) { > > imgFrame *curframe = mFrames.ElementAt(mFrames.Length() - 1); > > curframe->UnlockImageData(); > > } > > > >+ if (!mDecoder) > >+ return NS_ERROR_FAILURE; > >+ > > This is impossible - we hold a strong reference, and we even dereference > mDecoder above. Just drop this null check. Just because we take a strong ref on the object mDecoder refers to doesn't prevent the mDecoder member from later being set to null. RasterImage::ShutdownDecoder does set mDecoder to null and it can be invoked while still having RasterImage methods on the stack that use mDecoder, like the method above. There's a path from the Write call above to ShutdownDecoder in the first stack I attached. I'd like to investigate these null-checks further because I think they are necessary. Unfortunately the server doesn't accept the login anymore. Jordi, do you have a alternative testcase I can use?
(In reply to comment #24) > Unfortunately the server doesn't accept the login anymore. I reactivated the login (credentials as before) to the server: http://appcam.mobotixcam.de:2080/ Please complain if login fails.
Some of the possible paths to RasterImage::ShutdownDecoder: RasterImage::SourceDataComplete imgRequest::OnStopRequest RasterImage::RequestDecode RasterImage::WantDecodedFrames imgRequest::OnDataAvailable imgRequest::RequestDecode RasterImage::SyncDecode RasterImage::Draw RasterImage::ExtractFrame RasterImage::CopyFrame RasterImage::GetFrame RasterImage::DoError CONTAINER_ENSURE_SUCCESS RasterImage::Init RasterImage::DecodingComplete RasterImage::AddSourceData RasterImage::NewSourceData RasterImage::InitDecoder RasterImage::WriteToDecoder RasterImage::DecodeSomeData RasterImage::UnlockImage CONTAINER_ENSURE_TRUE RasterImage::GetImgFrame RasterImage::GetDrawableImgFrame RasterImage::GetCurrentDrawableImgFrame RasterImage::GetCurrentImgFrame RasterImage::GetCurrentFrameIsOpaque RasterImage::GetCurrentFrameRect RasterImage::StartAnimation RasterImage::ResetAnimation Image::EvaluateAnimation Image::SetAnimationMode Image::DecrementAnimationConsumers Image::IncrementAnimationConsumers RasterImage::InternalAddFrame RasterImage::AppendPalettedFrame RasterImage::Notify RasterImage::EnsureCleanFrame RasterImage::FrameUpdated Decoder::FlushInvalidations RasterImage::SetFrameDisposalMethod RasterImage::SetFrameTimeout RasterImage::SetFrameBlendMethod RasterImage::SetFrameHasNoAlpha RasterImage::SetSize Decoder::PostSize nsPNGDecoder::WriteInternal Decoder::Write RasterImage::WriteToDecoder nsPNGDecoder::info_callback imgDecodeWorker::Run I can probably find more to add to that list, but as you can see most of the RasterImage methods are already tainted.
Attached file WriteToDecoder null-ptr crash —
Crash if I remove the null check I added in WriteToDecoder. (using http://appcam.mobotixcam.de:2080/)
Comment on attachment 517303 [details] [diff] [review] fix v1 Please re-review -- I want to keep the null-checks I added, see additional information above why they are needed.
Attachment #517303 - Flags: review+ → review?(joe)
Whiteboard: [sg:critical] → [sg:critical][needs review]
Attached patch mochitest.diff — — Splinter Review
multipart/x-mixed-replace jpeg image crash test
Alright. I can buy those null checks, but perhaps not where they are put. Shouldn't we null-guard mDecoder everywhere we might use it when it's null?
> Shouldn't we null-guard mDecoder everywhere we might use it when it's null? Yes, but it's more important to land this soon to bake it for inclusion on the stable branches to get the exploitable part fixed. We can worry about null-pointer crashes later. Do you have any specific null-checks in mind at this time that you want to add or move?
We're not going to hold up Macaw for this one, but we still want it on m-c and will look at future releases. Would this bug also apply to the 1.9.2 branch? That's where the crash was originally seen.
blocking2.0: ? → ---
status1.9.2: --- → wanted
Joe, Mats, can we please get this reviewed and landed, or get a new patch that's landable soon?
Comment on attachment 517303 [details] [diff] [review] fix v1 >@@ -2406,28 +2418,29 @@ RasterImage::SyncDecode() >- if (IsDecodeFinished()) { >+ if (mDecoder && IsDecodeFinished()) { > rv = ShutdownDecoder(eShutdownIntent_Done); > CONTAINER_ENSURE_SUCCESS(rv); > } >@@ -2688,17 +2703,17 @@ imgDecodeWorker::Run() > // If the decode finished, shutdown the decoder >- if (image->IsDecodeFinished()) { >+ if (image->mDecoder && image->IsDecodeFinished()) { These two null checks should be moved inside IsDecodeFinished or ShutdownDecoder, or wherever they're necessary. I'm OK with it if you do that as a followup bug.
Attachment #517303 - Flags: review?(joe) → review+
This should land on mozilla-shadow right? How do I check in there?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status-firefox5: --- → affected
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
Whiteboard: [sg:critical][needs review] → [sg:critical]
Please land on mozilla-central. After some sanity checking we'll want it on mozilla-aurora and mozilla-1.9.2 (but please request approval for those branches)
blocking1.9.2: ? → .18+
blocking2.0: ? → -
tracking-firefox5: ? → +
tracking-firefox6: ? → +
Comment on attachment 517303 [details] [diff] [review] fix v1 We probably want this on aurora once it's baked on trunk a little while.
Attachment #517303 - Flags: approval-mozilla-aurora?
Blocks: 653489
Attachment #517303 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mats, can you land this?
Whiteboard: [sg:critical] → [sg:critical][needs trunk landing, branch patches]
Please land on central and aurora. the shadow repo is not quite ready.
Mats, do we need to reassign this bug to someone who will land the patch and respond to bugmail?
(In reply to comment #43) > Mats, do we need to reassign this bug to someone who will land the patch and > respond to bugmail? Sorry, that came out sounding wrong. But we really do need to land this.
Sorry, I'm behind on reading bugmail, I'll try to land this later tonight...
Landed on m-c, holding the testcase until the bug is public. Filed bug 656351 to followup on comment 36. http://hg.mozilla.org/mozilla-central/rev/86849e5fb7ca
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs trunk landing, branch patches] → [sg:critical][needs branch patches]
Target Milestone: --- → mozilla6
Please land for Aurora by Monday May 16 or the approval will potentially be lost. Please mark as status-firefox5 fixed when you do.
Last crash on Aurora in this signature showing as 20110427143820, so far the fix looks good from a crash stats point of view.
Per security group discussion, requesting landing on mozilla-2.0.
Attachment #517303 - Flags: approval2.0?
Comment on attachment 517303 [details] [diff] [review] fix v1 Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #517303 - Flags: approval2.0? → approval2.0+
Whiteboard: [sg:critical][needs branch patches] → [sg:critical][bug 638018 is fixing this on the 1.9.2 branch]
Crash Signature: [@ @0x0 | mozilla::imagelib::Decoder::Finish() ]
Verified for 1.9.2.18 with bug 638018. The testcase here doesn't crash in 1.9.2.
Keywords: verified1.9.2
Group: core-security
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 Can you please provide another link (besides this one: http://appcam.mobotixcam.de:2080/) with which I can verify this issue? Thanks!
(In reply to comment #55) > Can you please provide another link (besides this one: > http://appcam.mobotixcam.de:2080/) with which I can verify this issue? I will try to make the link working again. But this will not happen before next week.
(In reply to comment #55) > Can you please provide another link (besides this one: > http://appcam.mobotixcam.de:2080/) with which I can verify this issue? The link is working again.
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0 beta 5 I have followed the STR and I've got no crash.
Status: RESOLVED → VERIFIED
The link is permanently down.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: