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

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Bob Clary [:bc] (inactive)]

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.

Comment 1

[Bob Clary [:bc] (inactive)]

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

Comment 2

[Simon Montagu :smontagu]

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.

Comment 3

[Simon Montagu :smontagu]

Attachment: CSS file for testcase

Comment 4

[Simon Montagu :smontagu]

Attachment: Minimized testcase

Comment 5

[chris hofmann]

minimized test case crashes for me on mac too
https://crash-stats.mozilla.com/report/index/bp-4c50b1cd-b3fc-41f1-b33c-d2fcc2110302

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Marcia Knous [:marcia]]

My Win crash report: https://crash-stats.mozilla.com/report/index/2896b195-7e97-4b7d-bf56-b85b22110302

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix converter

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)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... 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 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for converter input stream

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 17

[Boris Zbarsky [:bzbarsky]]

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...

Comment 18

[Boris Zbarsky [:bzbarsky]]

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....

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Crashes in the wild, dated specifically from before we were looking at it, are:
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=ConverterInputStream%3A%3AFill&date=03%2F02%2F2011%2000%3A00%3A00&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=nsConverterInputStream%3A%3AFill%28unsigned%20int*%29
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=ConverterInputStream%3A%3AFill&date=03%2F02%2F2011%2000%3A00%3A00&range_value=4&range_unit=weeks&hang_type=any&process_type=any&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=nsConverterInputStream%3A%3AFill

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 21

[Henri Sivonen (:hsivonen)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 516479 [details] [diff] [review]
fix converter

It seems that returning NS_OK shouldn't confuse the HTML5 parser at least.

Comment 24

[Simon Montagu :smontagu]

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).

Comment 25

[Simon Montagu :smontagu]

(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.

Comment 26

[Henri Sivonen (:hsivonen)]

(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.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 28

[Simon Montagu :smontagu]

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.

Comment 29

[Simon Montagu :smontagu]

Comment on attachment 516481 [details] [diff] [review]
fix for converter input stream

Oops, wrong attachment

Comment 30

[Simon Montagu :smontagu]

Attachment: The test as crashtest, for check in after we release with the fixes

Comment 31

[LegNeato]

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

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This one is needed; it's "fix converter" that should be rolled in to the other patch.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/925f4985ee72
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c11daae5821

Want to check that the other bug gets in before marking as fixed, though...

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, the approved patch in the other bug has "fix converter" incorporated, so we're ok.