Closed Bug 172700 Opened 22 years ago Closed 22 years ago

NS_ConvertUTF8ToUCS2() surrogates buffer overflow

Categories

(Core :: XPCOM, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jgmyers, Assigned: shanjian)

References

Details

Attachments

(2 files, 1 obsolete file)

The following patch supports characters outside of the BMP.
Attached patch Proposed fix (obsolete) — Splinter Review
Attached patch Revised fixSplinter Review
Half of this fix was landed independently by bug 134053. As that bug only addressed half of the issue, it introduced a buffer overflow. Removing the part of this patch that is no longer needed due to the landing of bug 134053.
Attachment #101775 - Attachment is obsolete: true
Buffer overflow. Increasing severity to "critical".
Severity: normal → critical
Change summary.
Summary: NS_ConvertUTF8ToUCS2() doesn't support surrogates → NS_ConvertUTF8ToUCS2() surrogates buffer overflow
Oops, John is right. Without this fix, bug 134053 will cause buffer overflow. Steal this bug and drive it in.
Assignee: jaggernaut → shanjian
Scott, could you give sr=?
Status: NEW → ASSIGNED
Comment on attachment 102855 [details] [diff] [review] Revised fix r=shanjian,
Attachment #102855 - Flags: review+
This should be fixed before shipping 1.2beta.
Blocks: 1.2b
Blocks: 1.2
No longer blocks: 1.2b
Comment on attachment 102855 [details] [diff] [review] Revised fix This code should use |++mLength;| rather than |mLength++;| for stylistic consistency (and it's better style in C++, given the existence of iterators, since it forms a good habit for when you use iterators, along with "say what you mean"). See bug 78032 comment 1. Also, shouldn't this only increment |mLength| if you're actually going to be able to use surrogates? I think the exact condition is: if (((*p & 0x3) << 6) | (*(p+1) & 0x3f) < 0x00110000)
A few other comments on this function while I'm looking at it: * It might be better to test (while p < end), in case there's an error. * The |is4byte| case should be |-ing with 0x001C0000L, for consistency with all the other parts which use the minimum set of bits that they need. (Right?) * The "// ignore BOM" code is overindented by two spaces.
I'm not happy with the way this converter errors out on illegal input. That given, I think these concerns should be handled in a follow-up bug, after 1.2beta is released. IMO, none of these concerns are important enough to justify the risk of fixing them this close to 1.2beta. By the way, the condition is: if (((*p & 0x7) << 6) | (*(p+1) & 0x3f)) < (0x00110000 >> 12)) but I don't consider it a big deal if the converter slightly overalloates memory on illegal input.
This converter is not intended to be used for user-input, only for internal UTF-8. But anyway... The patch seems like it would be acceptable given the change from mLength++ to ++mLength.
jrgm: that's ok, 1.2b shipped with this wart among many others.
Attachment #103367 - Flags: review+
dbaron, can you give sr?
Comment on attachment 103367 [details] [diff] [review] revised for ++ operator. sr=dbaron
Attachment #103367 - Flags: superreview+
Comment on attachment 103367 [details] [diff] [review] revised for ++ operator. a=blizzard on behalf of drivers for 1.2final
Attachment #103367 - Flags: approval+
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: