Closed Bug 843434 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 - affected
firefox21 + fixed
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: jaas)

References

Details

(4 keywords, Whiteboard: [adv-main21+])

Attachments

(4 files, 2 obsolete files)

Attached file 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);
Attached file stack
ISO-2022-JP decoder should honor the mErrBehavior...
is dstLen initialized to something or did line 220-222 just incorporate a random amount of junk into the string?
Keywords: sec-moderate
Assignee: nobody → sworkman
dstLen is initialized by GetMaxLength in line 206.
Attached file testcase 2: <script>
Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at ../../../../content/base/src/nsScriptLoader.cpp:1070
(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.
(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.
Attached patch fix v1.0 (obsolete) — Splinter Review
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!
Attachment #722639 - Flags: review?(VYV03354)
Comment on attachment 722639 [details] [diff] [review]
fix v1.0

You need to call CHECK_OVERRUN every time you write to dest.
Attachment #722639 - Flags: review?(VYV03354) → review-
Attached patch fix v1.1 (obsolete) — Splinter Review
Like so?
Attachment #722639 - Attachment is obsolete: true
Attachment #722911 - Flags: review?(VYV03354)
I will also write a test for this.
Assignee: sworkman → joshmoz
Attachment #722911 - Flags: review?(VYV03354) → review+
Attached patch fix v1.1 w/testSplinter Review
This test works, but there may be a better test. The test here fails if the assertion is triggered.
Attachment #722911 - Attachment is obsolete: true
Attachment #723468 - Flags: review?(VYV03354)
Attachment #723468 - Flags: review?(VYV03354) → review+
Attachment #723468 - Flags: review?(smontagu)
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.
Attachment #723468 - Flags: review?(smontagu)
https://hg.mozilla.org/mozilla-central/rev/8d7a31fbb879
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I found another testcase that still trips the assertion.  Filed bug 851982.
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
Attachment #723468 - Flags: approval-mozilla-release?
Attachment #723468 - Flags: approval-mozilla-beta?
Blocks: 858433
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 .
Attachment #723468 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #723468 - Flags: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [adv-main21+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.