634257 - nsUCS2BEToUnicode fails to adhere to the API contract when given a buffer with one byte

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

The nsIUnicodeDecoder documentation says:
"The eventual incomplete final character data will be stored internally in the converter and used when the method is called again for continuing the conversion. This way, the caller will not have to worry about managing incomplete input data by mergeing it with the next buffer."
and
"When partial input, it will be consumed and cached."

Furthermore, the docs say:
"@param aDestLength [IN/OUT] the length of the destination data buffer; after conversion will contain the number of Unicode characters written"

However, if a one-byte buffer is passed to nsUCS2BEToUnicode, it returns NS_ERROR_ILLEGAL_INPUT in violation of the requirement for decoders to cache partial input and, much worse, it fails to set aDestLength to 0.

This looks to the caller as if the entire output buffer was written to but then an error occurred. This means that uninitialized memory shows to content, because in fact 0 code units were written to the output.

The decoder should work even if it were fed one byte at a time and it should always adjust aDestLength to reflect the actual number of bytes written before it returns.

I've seen crashes that have to be somehow related to this, but I don't have an explanation for how the crashes happen.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Test case

Comment 2

[chris hofmann]

I just saw a crash too, right after display on junk in the content window.  no breakpad

Comment 3

[Daniel Veditz [:dveditz]]

We're at least leaking chunks of memory. chofmann crashed (without getting breakpad) so could be worse

Comment 4

[Simon Montagu :smontagu]

Attachment: Fix for the crash

This returns the correct value in aDestLength and should prevent the crash.

> The decoder should work even if it were fed one byte at a time

I'm deferring this part until after FF 4. The UTF-16 decoder is much too byzantine already, and this will be a relatively complex change.

Comment 5

[chris hofmann]

crash w/o breakpad is pretty reproducible for me on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110214 Firefox/4.0b12pre

Comment 6

[Simon Montagu :smontagu]

Attachment: Tests

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Risk/reward?

Comment 8

[chris hofmann]

things mentioned on the risk side in critkill:

leaking chunks of memory could revile private info like credit cards, browsing history, etc. if the attacker could figure out how to control the behavior.  do we know if that might be possible?

the simple test case thats resulting in the crash could be bad if the crash is exploitable.  this is also hard to determine  so far since we haven't been able to get a stack though breakpad or someone runniung in the debugger.

Comment 9

[Mike Shaver (:shaver emeritus)]

is this ready to land?

Comment 10

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/aa28638dc457

I'll check in the test case after we release with the patch.

Comment 11

[Henri Sivonen (:hsivonen)]

While the test case here does not show the problem in Firefox 3.6, shouldn't the patch also go on branches--just in case there are other ways to get a one-byte buffer passed to the UTF-16 decoders on the branches?

Comment 12

[Simon Montagu :smontagu]

Attachment: Tests

Reftests are better than crashtests for this, to ensure that we are really only seeing one character in the results.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 513251 [details] [diff] [review]
Fix for the crash

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 14

[Daniel Veditz [:dveditz]]

fwiw the testcase does not crash 3.5.18pre or 3.6.15pre on Mac.

Comment 15

[Simon Montagu :smontagu]

Holding the branch checkin for a fix to bug 638236

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 513251 [details] [diff] [review]
Fix for the crash

On second thought, please don't check these in until we understand bug 638236

Comment 17

[Simon Montagu :smontagu]

Attachment: Patch for branches including dbaron's fix from bug 638236

transferring r=emk and r=smontagu from the original patches (attachment 513251 [details] [diff] [review] and attachment 516479 [details] [diff] [review])

Comment 18

[LegNeato]

Comment on attachment 516532 [details] [diff] [review]
Patch for branches including dbaron's fix from bug 638236

Approved for branch

Comment 19

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/11cf54bab5f9
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/facd39641206