Closed
Bug 1000030
Opened 10 years ago
Closed 10 years ago
crash in RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: jkitch, Assigned: jkitch)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file, 1 obsolete file)
2.28 KB,
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-dda6ef3c-25a6-4a2c-b233-8191a2140417. ============================================================= Fallout from bug 328755 which made the empty string's null terminator immutable. Like bug 936886, it seems we can't trust Windows not to overwrite the empty string.
Assignee | ||
Updated•10 years ago
|
Crash Signature: [@ RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33] → [@ RtlExpandEnvironmentStrings | kernelbase.dll@0xfc33]
[@ RtlExpandEnvironmentStrings_U | kernel32.dll@0x306bb]
Assignee | ||
Comment 1•10 years ago
|
||
The attached patch only the expands environment string if it expands to something more than a null terminator. This also includes a preventative fix to ReadBinaryValue which appears to be vulnerable to the same sort of bug.
Attachment #8411630 -
Flags: review?(dmajor)
Comment on attachment 8411630 [details] [diff] [review] patch.diff Review of attachment 8411630 [details] [diff] [review]: ----------------------------------------------------------------- I'm not convinced that ReadBinaryValue is affected. The issue with ReadStringValue is that we do some ++/-- tricks and eventually call the API telling it "you may write 1 byte here" when really it's "you may not write any bytes here". As far as I can tell, ReadBinaryValue is honest about the API parameters. I guess the check can't hurt, though.
Attachment #8411630 -
Flags: review?(dmajor) → review+
Actually I would remove the comment from ReadBinaryValue unless you're sure it would result in a crash.
Assignee | ||
Comment 4•10 years ago
|
||
ReadBinaryValue comment removed. I don't have any evidence that it is a problem, only that it fits the general pattern of similar crashes (size manipulations excepted). That being said, the null check does no harm.
Attachment #8411630 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/766648238bd3
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/766648238bd3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
So I was thinking about this some more, and isn't ReadStringValue still broken for values >1? We tell the API that it can write one more byte than the actual length of the string: --resultLen; expandedResult.SetLength(resultLen); [...] resultLen = ExpandEnvironmentStringsW(flatSource.get(), wwc(begin.get()), resultLen + 1);
Assignee | ||
Comment 8•10 years ago
|
||
My understanding is that our string length excludes the null terminator, while ExpandEnvironmentStringsW's string length includes it. Although it may result in the write exceeding the bounds of the string buffer, all it does is overwrite the next character in memory, which is always 0, with another 0. Although it may technically be a buffer overflow, it should be safe providing we can trust Microsoft to always write the null terminator in the appropriate position.
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8413169 [details] [diff] [review] patch.diff [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 328755 User impact if declined: Repeated crashes for users with specific registry entries. Testing completed (on m-c, etc.): On M-C for a week and Aurora for a few days without any apparent issues. Risk to taking this patch (and alternatives if risky): Low. Little more than a null check. String or IDL/UUID changes made by this patch: None
Attachment #8413169 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8413169 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/c66942faa3b2
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
Updated•10 years ago
|
status-b2g-v1.4:
--- → fixed
Comment 12•10 years ago
|
||
Marking this verified fixed as there are 0 reports with this signature across all branches in the last week.
You need to log in
before you can comment on or make changes to this bug.
Description
•