Closed Bug 638236 Opened 14 years ago Closed 14 years ago

Crash [@ nsConverterInputStream::Fill(unsigned int *) ] | ASSERTION: Whoa. The converter should have returned NS_OK_UDEC_MOREINPUT before this point!: 'srcConsumed <= mByteData->GetLength()'

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+
status1.9.2 --- .17-fixed
status1.9.1 --- .19-fixed

People

(Reporter: bc, Assigned: dbaron)

References

(
URL
)

Details

(4 keywords, Whiteboard: [sg:critical?][hardblocker])

Crash Data

Attachments

(5 files)

CSS file for testcase
1 byte, text/css
Details
Minimized testcase
1.05 KB, text/html; charset=UTF-16
Details
fix converter
2.47 KB, patch
smontagu
: review+
hsivonen
: feedback+
Details | Diff | Splinter Review
fix for converter input stream
951 bytes, patch
bzbarsky
: review+
christian
: approval1.9.2.17+
christian
: approval1.9.1.19+
Details | Diff | Splinter Review
The test as crashtest, for check in after we release with the fixes
1.15 KB, patch
Details | Diff | Splinter Review
1. http://zchoin.fct.put.poznan.pl/en.html 2. assert like a mad man and then crash (wait for it) 1.9.2 doesn't seem to crash but 2.0.0 does. (these files haven't changed since the switch from cvs...) 1.9.2: ###!!! ASSERTION: Whoa. The converter should have returned NS_OK_UDEC_MOREINPUT before this point!: 'srcConsumed <= mByteData->GetLength()', file /work/mozilla/builds/1.9.2/mozilla/intl/uconv/src/nsConverterInputStream.cpp, line 255 ###!!! ASSERTION: Decoder returned an error but filled the output buffer! Should not happen.: '0 < mUnicharData->GetBufferSize() - mUnicharDataLength', file /work/mozilla/builds/1.9.2/mozilla/intl/uconv/src/nsConverterInputStream.cpp, line 246 2.0.0: ASSERTION: Whoa. The converter should have returned NS_OK_UDEC_MOREINPUT before this point!: 'srcConsumed <= mByteData->GetLength()', file /work/mozilla/builds/2.0.0/mozilla/intl/uconv/src/nsConverterInputStream.cpp, line 255 ss since I don't like the write aspect and the assert. On Mac: Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0xfffdfffd [Switching to process 44119 thread 0x4603] 0x04cc8b42 in nsRefPtr<nsIRunnable>::~nsRefPtr (this=0xb032abc0) at nsAutoPtr.h:969 969 mRawPtr->Release(); (gdb) bt #0 0x04cc8b42 in nsRefPtr<nsIRunnable>::~nsRefPtr (this=0xb032abc0) at nsAutoPtr.h:969 #1 0x0646fc53 in nsEventQueue::PutEvent (this=0x51839c, runnable=0x23ad8920) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsEventQueue.cpp:140 #2 0x06471332 in nsThread::nsChainedEventQueue::PutEvent (this=0x518394, event=0x23ad8920) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsThread.cpp:760 #3 0x064715c7 in nsThread::PutEvent (this=0x518360, event=0x23ad8920) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsThread.cpp:389 #4 0x06472b6c in nsThread::Dispatch (this=0x518360, event=0x23ad8920, flags=0) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsThread.cpp:433 #5 0x0647a79b in nsTimerImpl::PostTimerEvent (this=0x581980) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsTimerImpl.cpp:552 #6 0x0647b41a in TimerThread::Run (this=0x518620) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/TimerThread.cpp:313 #7 0x06471acc in nsThread::ProcessNextEvent (this=0x5806c0, mayWait=1, result=0xb032aecc) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsThread.cpp:633 #8 0x063f808a in NS_ProcessNextEvent_P (thread=0x5806c0, mayWait=1) at nsThreadUtils.cpp:250 #9 0x064725f3 in nsThread::ThreadFunc (arg=0x5806c0) at /work/mozilla/builds/2.0.0/mozilla/xpcom/threads/nsThread.cpp:278 #10 0x00084c49 in _pt_root (arg=0x583ec0) at /work/mozilla/builds/2.0.0/mozilla/nsprpub/pr/src/pthreads/ptthread.c:187 #11 0x95f28155 in _pthread_start () #12 0x95f28012 in thread_start () On Windows: Operating system: Windows NT 5.1.2600 Service Pack 3 CPU: x86 GenuineIntel family 6 model 44 stepping 2 1 CPU Crash reason: EXCEPTION_ACCESS_VIOLATION_WRITE Crash address: 0x4281000 Thread 0 (crashed) 0 xul.dll!nsConverterInputStream::Fill(unsigned int *) [nsConverterInputStream.cpp : 247 + 0x29] eip = 0x10088306 esp = 0x0012d1c8 ebp = 0x0012d210 ebx = 0x00000001 esi = 0x0000a770 edi = 0x00000000 eax = 0x0426c120 ecx = 0x041fe9b8 edx = 0x1153fffd efl = 0x00010246 Found by: given as instruction pointer in context 1 xul.dll!nsConverterInputStream::Read(unsigned short *,unsigned int,unsigned int *) [nsConverterInputStream.cpp : 105 + 0xe] eip = 0x10087d84 esp = 0x0012d218 ebp = 0x0012d228 Found by: call frame info 2 xul.dll!nsCSSScanner::EnsureData() [nsCSSScanner.cpp : 634 + 0x2e] eip = 0x109d3663 esp = 0x0012d230 ebp = 0x0012d24c Found by: call frame info 3 xul.dll!nsCSSScanner::Read() [nsCSSScanner.cpp : 653 + 0x1b] eip = 0x109d3714 esp = 0x0012d254 ebp = 0x0012d25c Found by: call frame info 4 xul.dll!nsCSSScanner::Next(nsCSSToken &) [nsCSSScanner.cpp : 754 + 0x7] eip = 0x109d3a51 esp = 0x0012d264 ebp = 0x0012d284 Found by: call frame info 5 xul.dll!`anonymous namespace'::CSSParserImpl::GetToken(int) [nsCSSParser.cpp : 1292 + 0x11] eip = 0x109d6a9c esp = 0x0012d28c ebp = 0x0012d294 Found by: call frame info 6 xul.dll!`anonymous namespace'::CSSParserImpl::Parse(nsIUnicharInputStream *,nsIURI *,nsIURI *,nsIPrincipal *,unsigned int,int) [nsCSSParser.cpp : 916 + 0x9] eip = 0x109d5abc esp = 0x0012d29c ebp = 0x0012d2c4 Found by: call frame info 7 xul.dll!nsCSSParser::Parse(nsIUnicharInputStream *,nsIURI *,nsIURI *,nsIPrincipal *,unsigned int,int) [nsCSSParser.cpp : 9045 + 0x21] eip = 0x109e7de9 esp = 0x0012d2cc ebp = 0x0012d2e8 Found by: call frame info 17 crashes on windows in last week in socorro.
I'm getting several other signatures on this url as well: Operating system: Windows NT 6.1.7601 Service Pack 1 CPU: x86 GenuineIntel family 6 model 44 stepping 2 1 CPU Crash reason: EXCEPTION_ACCESS_VIOLATION_READ Crash address: 0xfffffffffffe000d Thread 8 (crashed) 0 xul.dll!nsHttpConnectionMgr::EnsureSocketThreadTargetIfOnline() [nsHttpConnectionMgr.cpp : 120 + 0xc] eip = 0x646bb4d9 esp = 0x02eef61c ebp = 0x02eef668 ebx = 0x0047fa80 esi = 0x00000000 edi = 0x00000000 eax = 0x02eef630 ecx = 0xfffdfffd edx = 0x02eef664 efl = 0x00010246 Found by: given as instruction pointer in context 1 xul.dll!nsHttpConnectionMgr::PostEvent(void ( nsHttpConnectionMgr::*)(int,void *),int,void *) [nsHttpConnectionMgr.cpp : 195 + 0x7] eip = 0x646bb741 esp = 0x02eef670 ebp = 0x02eef6a8 Found by: call frame info 2 xul.dll!nsHttpConnectionMgr::ReclaimConnection(nsHttpConnection *) [nsHttpConnectionMgr.cpp : 363 + 0x12] eip = 0x646bbf9f esp = 0x02eef6b0 ebp = 0x02eef6c4 Found by: call frame info 3 xul.dll!nsHttpHandler::ReclaimConnection(nsHttpConnection *) [nsHttpHandler.h : 159 + 0xe] eip = 0x646bddf6 esp = 0x02eef6cc ebp = 0x02eef6d4 Found by: call frame info 4 xul.dll!nsHttpConnectionMgr::nsConnectionHandle::~nsConnectionHandle() [nsHttpConnectionMgr.cpp : 1069 + 0x11] eip = 0x646bddab esp = 0x02eef6dc ebp = 0x02eef6e4 Found by: call frame info 5 xul.dll!nsHttpConnectionMgr::nsConnectionHandle::`scalar deleting destructor'(unsigned int) + 0xe eip = 0x646bd05f esp = 0x02eef6ec ebp = 0x02eef6f0 Found by: call frame info Operating system: Windows NT 6.1.7601 Service Pack 1 CPU: x86 GenuineIntel family 6 model 44 stepping 2 2 CPUs Crash reason: EXCEPTION_ACCESS_VIOLATION_READ Crash address: 0xfffffffffffe0000 Thread 8 (crashed) 0 nspr4.dll!PR_EnterMonitor [prmon.c : 96 + 0x6] eip = 0x718f9f4a esp = 0x0442f89c ebp = 0x0442f8a0 ebx = 0x00449350 esi = 0x00000000 edi = 0x00000000 eax = 0x05203620 ecx = 0xfffdfffd edx = 0x0442f8ec efl = 0x00010202 Found by: given as instruction pointer in context 1 xul.dll!nsAutoMonitor::nsAutoMonitor(PRMonitor *,mozilla::GuardObjectNotifier const &) [nsAutoLock.h : 310 + 0xc] eip = 0x6bd61917 esp = 0x0442f8a8 ebp = 0x0442f8b0 Found by: call frame info 2 xul.dll!nsHttpConnectionMgr::EnsureSocketThreadTargetIfOnline() [nsHttpConnectionMgr.cpp : 120 + 0x17] eip = 0x6c3ab4e5 esp = 0x0442f8b8 ebp = 0x0442f908 Found by: call frame info 3 xul.dll!nsHttpConnectionMgr::PostEvent(void ( nsHttpConnectionMgr::*)(int,void *),int,void *) [nsHttpConnectionMgr.cpp : 195 + 0x7] eip = 0x6c3ab741 esp = 0x0442f910 ebp = 0x0442f948 Found by: call frame info 4 xul.dll!nsHttpConnectionMgr::ReclaimConnection(nsHttpConnection *) [nsHttpConnectionMgr.cpp : 363 + 0x12] eip = 0x6c3abf9f esp = 0x0442f950 ebp = 0x0442f964 Found by: call frame info 5 xul.dll!nsHttpHandler::ReclaimConnection(nsHttpConnection *) [nsHttpHandler.h : 159 + 0xe] eip = 0x6c3addf6 esp = 0x0442f96c ebp = 0x0442f974 Found by: call frame info Operating system: Windows NT 5.1.2600 Service Pack 3 CPU: x86 GenuineIntel family 6 model 44 stepping 2 1 CPU Crash reason: EXCEPTION_ACCESS_VIOLATION_READ Crash address: 0xfffffffffffdfffd Thread 15 (crashed) 0 ntdll.dll + 0x190f7 eip = 0x7c9190f7 esp = 0x048ffa5c ebp = 0x048ffa6c ebx = 0x02c60000 esi = 0x04221d58 edi = 0x00000806 eax = 0x04221d60 ecx = 0xfffdfffd edx = 0xfffdfffd efl = 0x00010246 Found by: given as instruction pointer in context 1 ntdll.dll + 0x12481 eip = 0x7c912482 esp = 0x048ffa74 ebp = 0x048ffc98 Found by: previous frame's frame pointer 2 msvcr80d.dll + 0x12f4b eip = 0x002b2f4c esp = 0x048ffca0 ebp = 0x048ffcb4 Found by: previous frame's frame pointer
http://zchoin.fct.put.poznan.pl/en.html is in UTF-16, even though it contains <meta http-equiv="Content-Type" content="text/html; charset=iso 8859-2" /> so this is probably a regression from bug 634257.
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Keywords: testcase-wanted → regression, testcase
Blocks: 634257
Summary: Crash [@ nsConverterInputStream::Fill(unsigned int *)] | ASSERTION: Whoa. The converter should have returned NS_OK_UDEC_MOREINPUT before this point!: 'srcConsumed <= mByteData->GetLength()' → Crash [@ nsConverterInputStream::Fill(unsigned int *) ] | ASSERTION: Whoa. The converter should have returned NS_OK_UDEC_MOREINPUT before this point!: 'srcConsumed <= mByteData->GetLength()'
Simon: did you prove the regression by backing out bug 634257? If that's the cause maybe we should just do that for FF4.0 and figure out the right fix for 634257 in 4.0.1 Chofmann says he sees this signature going back prior to that checkin though, so it's not guaranteed to be the cause.
Whiteboard: [sg:critical?]
Blocking as this is something that's happening in the wild, and constructing an exploit from this is probably not that hard. We need to decide if we want to fix this by backing out the fix for bug 634257, or if there's a super super safe fix for this w/o backing out the fix for bug 634257.
blocking2.0: ? → final+
Whiteboard: [sg:critical?] → [sg:critical?][hardblocker]
FWIW, backing out the fix for bug 634257 does *not* seem to eliminate all problems here. So far I have not seen a crash here myself, but I do see what looks like an infinite loop when loading the testcase, both with and w/o the fix for bug 634257 in my tree.
On a 64-bit Linux mozilla-central debug build, I see an infinite loop of assertions on attachment 516422 [details] both with or without http://hg.mozilla.org/mozilla-central/rev/aa28638dc457 backed out. On m-c, the loop only fires one repeating assertion; with the patch backed out I see two assertions repeating in alternation.
So I think the underlying crash here shows two bugs: (1) The converter is returning a failure result when given a 0-byte input buffer. (It's given a 0-byte buffer by nsConvertInputStream::Fill the second time around, after it fails with the 1-byte buffer, we reset and skip the 1 byte, and then try again.) (2) nsConverterInputStream::Fill writes replacement characters into its output buffer regardless of the available space in that buffer, so a repeated sequence of encoding errors (in any encoding) can overrun that stack buffer with 0xFFFDFFFD.
Attached patch fix converter — — Splinter Review
It seems like, maybe, it ought to be considered an invariant that if a convert is given a zero-byte input buffer, it shouldn't return failure. Not that we document our converter invariants... but it's not an entirely unreasonable assumption. So this fixes (1) above. But (1) is not, I think, sg:critical; based on my description of my understanding of what's happening, dveditz believes (2) is. I expect (2) can be exploited using a sequence of UTF-8 continuation bytes (since they're all invalid)
Attachment #516479 - Flags: review?(smontagu)
... and such an exploit would allow the caller to write a exploiter-determined number of 0xFFFD 16-bit units into and past the end of the output buffer.
Comment on attachment 516481 [details] [diff] [review] fix for converter input stream ... though it's possible that this could still lead to a one byte overrun *if* a decoder can fill its output buffer *and* return failure... which seems like it's probably a bad thing to do. Again, something we ought to figure out regarding what the rules on decoders are.
Comment on attachment 516481 [details] [diff] [review] fix for converter input stream r=me If we want to guard against the case you mention, we could just use dstLen one smaller than we use now (and change your new check accordingly). But I really hope none of our decoders are doing that...
Attachment #516481 - Flags: review?(bzbarsky) → review+
The converter input stream change looks very safe to me. The other change I don't really know enough about to evaluate, but if we don't need it to fix this bug (which is the case, I think), I'd be tempted to hold it till .next. It's probably safe too, but I don't trust the callers of that code very much without auditing them first....
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.1.18?
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker][has patch]
I landed the "fix for converter input stream": http://hg.mozilla.org/mozilla-central/rev/8be874c41712 I believe that fixes the known sg:critical aspects of this bug. I'd like to leave this open overnight so that Simon, Henri, and anyone else who cares to comment can express an opinion about what we want to do regarding any converter invariants that may or may not exist which we may or may not be disobeying as a result of bug 634257. We have *at least* the following options for 4.0 (for which we'd hope to give a go-to-build for a release candidate any day now), for 4.0.x, and for 5.0: * revert bug 634257 * keep the current state * land attachment 516479 [details] [diff] [review] There's a triage meeting at 9am Pacific tomorrow, so it would be good if you can express whatever thoughts you have on the matter before then (and sooner is always better anyway).
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker]
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker][has patch][one patch landed][remaining parts of bug might not block]
Comment on attachment 516479 [details] [diff] [review] fix converter Marking feedback-, because converters should never return NS_OK. The permitted return values are NS_ERROR_ILLEGAL_INPUT, NS_PARTIAL_MORE_OUTPUT and NS_PARTIAL_MORE_INPUT. Feedback+ if you change NS_OK to NS_PARTIAL_MORE_INPUT.
Attachment #516479 - Flags: feedback?(hsivonen) → feedback-
Converters return NS_OK all the time. NS_OK_UDEC_MOREINPUT means that the input so far hasn't been fully converted, but more input is required to finish outputting the result (e.g., the input stopped in the middle of a multi-byte UTF-8 sequence, or between the bytes of a 2-byte unit in UTF-16).
Attachment #516479 - Flags: feedback- → feedback?(hsivonen)
Comment on attachment 516479 [details] [diff] [review] fix converter It seems that returning NS_OK shouldn't confuse the HTML5 parser at least.
Attachment #516479 - Flags: feedback?(hsivonen) → feedback+
Comment on attachment 516479 [details] [diff] [review] fix converter Bug 634257 was also a hardblocker, though less critical than this, so I think we should land this rather than revert bug 634257 or leave it in its current state (i.e. with a bad fix).
Attachment #516479 - Flags: review?(smontagu)
Attachment #516479 - Flags: review+
Attachment #516479 - Flags: approval2.0?
(In reply to comment #21) > Marking feedback-, because converters should never return NS_OK. The permitted > return values are NS_ERROR_ILLEGAL_INPUT, NS_PARTIAL_MORE_OUTPUT and > NS_PARTIAL_MORE_INPUT. I suppose this is based on http://mxr.mozilla.org/mozilla-central/source/intl/uconv/public/nsIUnicodeDecoder.h#148, but it doesn't seem reasonable to me. I think it's only common sense that any interface can always return NS_OK even if the documentation doesn't explicitly say so.
Assignee: smontagu → dbaron
(In reply to comment #25) > (In reply to comment #21) > > Marking feedback-, because converters should never return NS_OK. The permitted > > return values are NS_ERROR_ILLEGAL_INPUT, NS_PARTIAL_MORE_OUTPUT and > > NS_PARTIAL_MORE_INPUT. > > I suppose this is based on > http://mxr.mozilla.org/mozilla-central/source/intl/uconv/public/nsIUnicodeDecoder.h#148, > but it doesn't seem reasonable to me. I think it's only common sense that any > interface can always return NS_OK even if the documentation doesn't explicitly > say so. I was lacking common sense when I wrote code that called the interface and got it right by luck. :-( It would be really nice for the interface to say explicitly which situation maps to NS_OK.
I landed the converter patch as well; my inclination is that we're safer with it than without it since other commonly used decoders (nsUTF8ToUnicode, nsUnicodeDecodeHelper::ConvertBy*) seem to return NS_OK when given empty source buffers, so we're better off sticking to that behavior: http://hg.mozilla.org/mozilla-central/rev/f76fac273005
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][hardblocker][has patch][one patch landed][remaining parts of bug might not block] → [sg:critical?][hardblocker]
Attachment #516479 - Flags: approval2.0?
Attachment #516479 - Flags: approval1.9.2.15?
Attachment #516479 - Flags: approval1.9.1.18?
Comment on attachment 516481 [details] [diff] [review] fix for converter input stream I'll rerequest approval in bug 634257 for a patch including this correction.
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.1.18?
Comment on attachment 516481 [details] [diff] [review] fix for converter input stream Oops, wrong attachment
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.1.18?
Attachment #516479 - Flags: approval1.9.2.15?
Attachment #516479 - Flags: approval1.9.1.18?
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.1.18?
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.1.18?
Comment on attachment 516481 [details] [diff] [review] fix for converter input stream Approved for branches, though this may be covered in the patch for bug 634257 based on comments
Attachment #516481 - Flags: approval1.9.2.15?
Attachment #516481 - Flags: approval1.9.2.15+
Attachment #516481 - Flags: approval1.9.1.18?
Attachment #516481 - Flags: approval1.9.1.18+
This one is needed; it's "fix converter" that should be rolled in to the other patch.
Actually, the approved patch in the other bug has "fix converter" incorporated, so we're ok.
status1.9.1: --- → .18-fixed
status1.9.2: --- → .16-fixed
Group: core-security
Crash Signature: [@ nsConverterInputStream::Fill(unsigned int *) ]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: