All users were logged out of Bugzilla on October 13th, 2018

Import Chromium's upstream to avoid rand_s() crash

RESOLVED FIXED in Firefox 45



3 years ago
3 years ago


(Reporter: cpeterson, Assigned: cpeterson)




Firefox Tracking Flags

(firefox43 wontfix, firefox44 wontfix, firefox45 fixed, firefox46 fixed)



(1 attachment)



3 years ago
Created attachment 8709147 [details] [diff] [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 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, which no longer calls rand_s(), and copies the RandBytes() function prototype from rand_util.h.
Attachment #8709147 - Flags: review?(jld)
Assignee: nobody → cpeterson
Comment on attachment 8709147 [details] [diff] [review]

Review of attachment 8709147 [details] [diff] [review]:

Looks good to me.
Attachment #8709147 - Flags: review?(jld) → review+

Comment 3

3 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.
status-firefox44: affected → wontfix
status-firefox46: affected → fixed

Comment 4

3 years ago
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Comment 5

3 years ago
Comment on attachment 8709147 [details] [diff] [review]

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.|+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?
status-firefox43: affected → wontfix
Comment on attachment 8709147 [details] [diff] [review]

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

3 years ago
status-firefox45: affected → fixed
You need to log in before you can comment on or make changes to this bug.