Closed
Bug 1240589
Opened 9 years ago
Closed 9 years ago
Import Chromium's upstream rand_util_win.cc to avoid rand_s() crash
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
(Keywords: crash)
Attachments
(1 file)
(deleted),
patch
|
jld
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Bug 1167248 has the long backstory, but the short story is the Windows' rand_s() can crash when some bad third-party software injects advapi32.dll hooks and Firefox has been built with VS2013. rand_s() just loads advapi32.dll and calls RtlGenRandom(). Instead we can just call RtlGenRandom() ourselves. Chrome did this with https://crbug.com/348400 and we did this in SpiderMonkey with bug 1167248.
bp-a74be01f-edd6-432f-a19f-eae272160108 is one example of a crash in ipc/chromium's call to rand_s(). This patch imports the latest snapshot of Chromium's rand_util_win.cc, which no longer calls rand_s(), and copies the RandBytes() function prototype from rand_util.h.
https://chromium.googlesource.com/chromium/src.git/+/8c37218a4cfa8725c0617db940f2d90b844f5023
Attachment #8709147 -
Flags: review?(jld)
Updated•9 years ago
|
Assignee: nobody → cpeterson
Comment 1•9 years ago
|
||
Comment on attachment 8709147 [details] [diff] [review]
import-RtlGenRandom-fix.patch
Review of attachment 8709147 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me.
Attachment #8709147 -
Flags: review?(jld) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Thanks, Jed!
Because this will fix some crashes seen in the field, I will ask for uplift after this change baked on Nightly for a couple days.
Comment 4•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8709147 [details] [diff] [review]
import-RtlGenRandom-fix.patch
Approval Request Comment
I fixed this crash in Nightly 46. I'd like to uplift this fix to 45, wherever that may be: Aurora 45 or Beta 45. :)
[Feature/regressing bug #]: None
[User impact if declined]: There were only 6 instances of this crash in the last 28 days, possibly from just 2 users. It is admittedly a very low frequency crash.
https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=invalid_parameter_noinfo+|+rand_s#tab-reports
[Describe test coverage new/current, TreeHerder]: This patch has been baking on Nightly for 3 days. This crash happens in our mozilla-central fork of Chromium's IPC code. This patch just imports the latest snapshot of two short files from Chromium's upstream repo, where Google has worked around the crash.
[Risks and why]: I think this is a low risk because this patch just imports Google's tested fix.
[String/UUID change made/needed]: None
Attachment #8709147 -
Flags: approval-mozilla-beta?
Attachment #8709147 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Comment 6•9 years ago
|
||
Comment on attachment 8709147 [details] [diff] [review]
import-RtlGenRandom-fix.patch
Fix a crash, taking it.
Attachment #8709147 -
Flags: approval-mozilla-beta?
Attachment #8709147 -
Flags: approval-mozilla-aurora?
Attachment #8709147 -
Flags: approval-mozilla-aurora+
Comment 7•9 years ago
|
||
bugherder uplift |
Comment 9•9 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•