Closed
Bug 172700
Opened 22 years ago
Closed 22 years ago
NS_ConvertUTF8ToUCS2() surrogates buffer overflow
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: jgmyers, Assigned: shanjian)
References
Details
Attachments
(2 files, 1 obsolete file)
868 bytes,
patch
|
shanjian
:
review+
|
Details | Diff | Splinter Review |
792 bytes,
patch
|
shanjian
:
review+
dbaron
:
superreview+
blizzard
:
approval+
|
Details | Diff | Splinter Review |
The following patch supports characters outside of the BMP.
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Comment 2•22 years ago
|
||
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
Reporter | ||
Comment 3•22 years ago
|
||
Buffer overflow. Increasing severity to "critical".
Severity: normal → critical
Reporter | ||
Comment 4•22 years ago
|
||
Change summary.
Summary: NS_ConvertUTF8ToUCS2() doesn't support surrogates → NS_ConvertUTF8ToUCS2() surrogates buffer overflow
Assignee | ||
Comment 5•22 years ago
|
||
Oops, John is right. Without this fix, bug 134053 will cause buffer overflow.
Steal this bug and drive it in.
Assignee: jaggernaut → shanjian
Assignee | ||
Comment 7•22 years ago
|
||
Comment on attachment 102855 [details] [diff] [review]
Revised fix
r=shanjian,
Attachment #102855 -
Flags: review+
Updated•22 years ago
|
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.
Reporter | ||
Comment 11•22 years ago
|
||
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.
Comment 13•22 years ago
|
||
jrgm: that's ok, 1.2b shipped with this wart among many others.
Assignee | ||
Comment 14•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #103367 -
Flags: review+
Assignee | ||
Comment 15•22 years ago
|
||
dbaron, can you give sr?
Comment on attachment 103367 [details] [diff] [review]
revised for ++ operator.
sr=dbaron
Attachment #103367 -
Flags: superreview+
Comment 17•22 years ago
|
||
Comment on attachment 103367 [details] [diff] [review]
revised for ++ operator.
a=blizzard on behalf of drivers for 1.2final
Attachment #103367 -
Flags: approval+
Assignee | ||
Comment 18•22 years ago
|
||
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•