244177 - nsScanner::Append() can overwrite the storage in the buffer it allocates.

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Johnny Stenback (:jst)]

In nsScanner::Append(const char* aBuffer, PRUint32 aLen), we're given the number
of bytes in the input, we then call:

    mUnicodeDecoder->GetMaxLength(aBuffer, aLen, &unicharBufLen);
    nsScannerString::Buffer* buffer = nsScannerString::AllocBuffer(unicharBufLen
+ 1);

And then go on to attempt to convert the given characters to unicode using the
converter for the assumed charset. If the assumed charset is wrong (i.e. UTF-16)
and the given data is single-byte data, we see that the conversion fails, and we
attempt to consume a byte at a time and convert again. Now in this case, we're
given UTF-8 (or whatever single byte-based input) and we think our max length is
the given number of bytes/2 (that's what GetMaxLength() on the UTF-16 decoder
tells us), so if we're given 30 bytes, we'll allocate storage for 15 characters,
and as long as the unicode conversion fails, we'll keep chopping of character
after character until we've gone through them all. That means we'll in this case
end up trying to store 30 characters in the buffer we allocated to hold 15
characters.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm... This could be a problem with any caller that uses our Unicode conversion
routines and does error recovery (eg the converter stream).  We probably need to
audit all callsites of GetMaxLength().  :(

I wish we had sane stream-based Unicode conversion APIs with error recovery in
the converters....

Comment 2

[Darin Fisher]

It looks like this problem exists before the string branch landing too.

Do you think it is enough to simply bounds check the unichars array and bail if
there isn't enough room?  should we return an error in that case?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Could that break fallback in some non-pathological cases?  If so, we need to do
something smarter; if not, that's probably fine.

Comment 4

[Simon Montagu :smontagu]

*** Bug 246450 has been marked as a duplicate of this bug. ***

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch

Here's the patch I had prepared for the dupe, which does what Darin suggests in
comment 2.

I think the "something smarter" that we should really be doing is to fix bug
197120, but in the meanwhile this would be a useful safeguard.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 150637 [details] [diff] [review]
Patch

Maybe if I ask for reviews someone will comment ;-)

Comment 7

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

I've always thought that we should skip 2-bytes at a type for a 2-byte encoding
(see bug 128896 comment 22).  That could be another possible fix, although a
little more involved...

Comment 8

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

So speaking of bug 128896, and in particular bug 128896 comment 23, do we need
to fix nsConverterInputStream::Fill as well?

The original analysis is, I assume, predicated on the unicode converter
reporting failure rather than success when it's passed a length of 0.  Is that
actually the case?

Comment 9

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

(aDestLength of 0, that is)

Comment 10

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

So, actually, based on what unicode decoders are *supposed* to be doing, I don't
see how this bug happens, since if *aDestLength is 0 the converter should return
a success value.  The converters in nsUCS2BEToUnicode.cpp don't quite follow
that contract, though, so I could see this happening if what they saw as a
backwards BOM occurred in the input.  (And I'm also a little puzzled how
nsUTF16LEToUnicode ever works on a big-endian platform for a file beginning with
a BOM, but that's getting off-topic.)

Comment 11

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

(I reread the code, so feel free to ignore my previous parenthetical.)

Comment 12

[Simon Montagu :smontagu]

This was being triggered recently by a bug (246194) in the UTF16 decoder which
has since been fixed.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 150637 [details] [diff] [review]
Patch

r=dveditz
Let's try to get this in.

Comment 14

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

Comment on attachment 150637 [details] [diff] [review]
Patch

sr=dbaron given that a concise version of comment 10 is added in a source
comment

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 150637 [details] [diff] [review]
Patch

a=asa for branches checkin.

Comment 16

[Simon Montagu :smontagu]

(In reply to comment #14)
> (From update of attachment 150637 [details] [diff] [review])
> sr=dbaron given that a concise version of comment 10 is added in a source
> comment
> 

I'm afraid I don't understand comment 10 well enough to add a concise version of
it, especially if it is based on the assumption in comment 8 which I don't think
is correct, though perhaps that is my misunderstanding.

Comment 17

[Daniel Veditz [:dveditz]]

Why didn't this land?

Comment 18

[Simon Montagu :smontagu]

Because I can't meet the conditions for sr. See comment 16.

Comment 19

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

How about:

This is only needed because some decoders don't follow the nsIUnicodeDecoder
contract by always returning NS_OK_UDEC_MOREOUTPUT when *aDestLength is 0.  See
bug 244177.

Comment 20

[Darin Fisher]

-> smontagu

Comment 21

[Daniel Veditz [:dveditz]]

I was confused by the proposed text in comment 19. Did you mean returning
NS_OK_UDEC_MOREOUTPUT was wrong, or that that is what the buggy ones are NOT
doing? I'm sure I could look up the relevant interfaces and work it out, but if
it's meant to help it should probably make sense on its own.

OK, I looked it up, and despite copious comments in nsIUnicodeDecoder.h it was
not mentioned that the decoders are supposed to return NS_OK_UDEC_MOREOUTPUT.
Maybe the @return section could be updated to not refer to the deprecated
(according to the comments near the #defines) NS_PARTIAL_MORE_OUTPUT. I can see
how implementors got confused, though, zero translation was done, not partial.

If I'm understanding this right, how about

  This is only needed because some decoders don't follow the nsIUnicodeDecoder
  contract: they return a failure when *aDestLength is 0 rather than the
  correct NS_OK_UDEC_MOREOUTPUT.  See bug 244177.

Comment 22

[Simon Montagu :smontagu]

Fix checked in to trunk and 1.7 branch.

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Backported to 1.4 branch

Here's a patch backported to 1.4 (pre-string-defragmentation).	I think this is
the correct way to do things way back when, but I'd like a sanity check.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164230 [details] [diff] [review]
Backported to 1.4 branch

r+sr=bzbarsky

Comment 25

[Daniel Veditz [:dveditz]]

This was *not* fixed on the aviary branch.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 150637 [details] [diff] [review]
Patch

Received a=drivers for 1.0.3

Comment 27

[Daniel Veditz [:dveditz]]

Fix checked into aviary branch

/cvsroot/mozilla/htmlparser/src/Attic/nsScanner.cpp,v  <--  nsScanner.cpp
new revision: 3.125.6.8.2.1; previous revision: 3.125.6.8