843434 - Assertion failure in nsUnicharStreamLoader::WriteSegmentFun with ISO-2022-JP

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at netwerk/base/src/nsUnicharStreamLoader.cpp:218

  213	  DebugOnly<nsresult> rv =
  214	    self->mDecoder->Convert(aSegment,
  215	                            &srcLen,
  216	                            self->mBuffer.BeginWriting() + haveRead,
  217	                            &dstLen);
* 218	  MOZ_ASSERT(NS_SUCCEEDED(rv));
  219	  MOZ_ASSERT(srcLen == static_cast<int32_t>(aCount));
  220	  haveRead += dstLen;
  221
  222	  self->mBuffer.SetLength(haveRead);

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Masatoshi Kimura [:emk]]

ISO-2022-JP decoder should honor the mErrBehavior...

Comment 3

[Daniel Veditz [:dveditz]]

is dstLen initialized to something or did line 220-222 just incorporate a random amount of junk into the string?

Comment 4

[Masatoshi Kimura [:emk]]

dstLen is initialized by GetMaxLength in line 206.

Comment 5

[Jesse Ruderman]

Attachment: testcase 2: <script>

Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at ../../../../content/base/src/nsScriptLoader.cpp:1070

Comment 6

[Josh Aas]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> ISO-2022-JP decoder should honor the mErrBehavior...

Can you explain this further?

The error code causing the assertion comes from nsISO2022JPToUnicodeV2::Convert:

================
       switch(mState)
       {
          case mState_ASCII:
            if(0x1b == *src)
            {
              mLastLegalState = mState;
              mState = mState_ESC;
            } else if(*src & 0x80) {
ERROR->       goto error2;
            } else {
              if (CHECK_OVERRUN(dest, destEnd, 1))
                goto error1;
              *dest++ = (PRUnichar) *src;
            }
          break;
================

The state is mState_ASCII, and "if(*src & 0x80)" is true.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Josh Aas (Mozilla Corporation) from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #2)
> > ISO-2022-JP decoder should honor the mErrBehavior...
> 
> Can you explain this further?

1. If mErrBehavior is not kOnError_Signal (default), the ISO-2022-JP decoder should replace the offending char with a REPLACEMENT CHARACTER (U+FFFD) rather than throwing NS_ERROR_UNEXPECTED.
2. Even if mErrBehavior is kOnError_Signal, the thrown error should be NS_ERROR_ILLEGAL_INPUT.

Comment 8

[Josh Aas]

Attachment: fix v1.0

Thanks for the info.

This avoids the assertion in the first test case, and gives what I'd expect for an error in the script case. I don't really know what this code is supposed to be doing though, so please review carefully!

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 722639 [details] [diff] [review]
fix v1.0

You need to call CHECK_OVERRUN every time you write to dest.

Comment 10

[Josh Aas]

Attachment: fix v1.1

Like so?

Comment 11

[Josh Aas]

I will also write a test for this.

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 722911 [details] [diff] [review]
fix v1.1

LGTM

Comment 13

[Josh Aas]

Attachment: fix v1.1 w/test

This test works, but there may be a better test. The test here fails if the assertion is triggered.

Comment 14

[Josh Aas]

https://tbpl.mozilla.org/?tree=Try&rev=738d670ac50d

Comment 15

[Josh Aas]

Comment on attachment 723468 [details] [diff] [review]
fix v1.1 w/test

Canceling second review, turns out emk is a peer. Had smontagu update the module owernship wiki.

Comment 16

[Josh Aas]

pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/8d7a31fbb879

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d7a31fbb879

Comment 18

[Jesse Ruderman]

I found another testcase that still trips the assertion.  Filed bug 851982.

Comment 19

[Masatoshi Kimura [:emk]]

Comment on attachment 723468 [details] [diff] [review]
fix v1.1 w/test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 638379
User impact if declined: Some Japanese production sites are broken (see bug 858433).
Testing completed (on m-c, etc.): on m-c and aurora
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 20

[bhavana bajaj [:bajaj]]

Approving on beta, as this is a recent regression from Bug 638379, which has impacted a few Japanese websites .The patch fixes the broken websites and is low risk,well baked on m-c and aurora.

Please land asap for this to make it into our next beta going to build tomorrow morning PT .

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e3bc8afeb76d

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this is sec-moderate, and we don't have indications that a large number of sites are affected, we'll ship this fix in FF21.