Closed
Bug 1000079
Opened 10 years ago
Closed 10 years ago
crash in memmove | LocalBaseRegQueryValue
Categories
(Toolkit :: General, defect)
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: jkitch, Assigned: jkitch)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
1.75 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
907 bytes,
patch
|
benjamin
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-9fe2e803-7538-4f57-9c5f-eb3022140418. ============================================================= 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.
Updated•10 years ago
|
tracking-firefox30:
--- → +
tracking-firefox31:
--- → +
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Comment 1•10 years ago
|
||
This patch copies over the changes applied in bug 936886 as the registry value isn't guaranteed to be null-terminated. One potential side effect to this patch: Registry strings that are not null-terminated will return an extra character, with the potential to cause subtle differences. However, this change has been applied to another part of the code since Gecko28 without any apparent issues (bug 936886).
Attachment #8411634 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8411634 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 2•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/05519890c1cc
Keywords: checkin-needed
Comment 3•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/05519890c1cc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8411634 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 328755 User impact if declined: Repeated crashes for users with specific registry strings. Testing completed (on m-c, etc.): On MC for the past week without any apparent problems. Risk to taking this patch (and alternatives if risky): Lowish - See comment 1 for a side effect. A variant patch which fixes the crash without the behaviour change can easily be used instead. String or IDL/UUID changes made by this patch: None
Attachment #8411634 -
Flags: approval-mozilla-beta?
Comment 5•10 years ago
|
||
Comment on attachment 8411634 [details] [diff] [review] patch Why would we willingly take on that behaviour change? What 'different' results might the user experience if they have registry entries without null character? Other kinds of crashes? Seems to me that we should take the option of not changing the behaviour if we can avoid it.
Flags: needinfo?(jkitch.bug)
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #5) > Comment on attachment 8411634 [details] [diff] [review] > patch > > Why would we willingly take on that behaviour change? What 'different' > results might the user experience if they have registry entries without null > character? Other kinds of crashes? We look for the Firefox profile folder in the wrong location. Given my limited knowledge of this bit of the tree, I cannot predict the weirdness that this would cause. The problem dates back to 2009, so it is either exceedingly rare or there is some fallback behaviour of which I am not aware which mitigates it. > Seems to me that we should take the > option of not changing the behaviour if we can avoid it. The attached patch fixes the crash without the behaviour change by returning success early. I now agree this new patch is the safer course of action.
Attachment #8418042 -
Flags: review?(benjamin)
Flags: needinfo?(jkitch.bug)
Updated•10 years ago
|
Attachment #8418042 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8411634 [details] [diff] [review] patch Cancelling approval request. This patch needs to be championed by someone with a much better understanding of the code if it is to be backported to beta.
Attachment #8411634 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8418042 [details] [diff] [review] Alternative patch for beta [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.): It compiles. Firefox launches. That is the limit of my testing - I cannot reproduce the conditions which exercise the code and it cannot land on m-c without backing out the previous patch. Risk to taking this patch (and alternatives if risky): This is a null-check. It has also been minimally tested. Unless it is deemed sufficiently simple, you might prefer it to temporarily land on m-c. String or IDL/UUID changes made by this patch: None
Attachment #8418042 -
Flags: approval-mozilla-beta?
Comment 9•10 years ago
|
||
Comment on attachment 8418042 [details] [diff] [review] Alternative patch for beta We're in the early days of beta so go ahead with uplift and we can get the most time out of the larger audience to inform the low risk of hitting the registry issue.
Attachment #8418042 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/594456499788
status-b2g-v1.4:
--- → fixed
Comment 12•10 years ago
|
||
Results from Socorro for [@ memmove | LocalBaseRegQueryValue] signature: - 0 crashes for Firefox 31: https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A31.0b&range_value=4&range_unit=weeks&date=06%2F24%2F2014+11%3A00%3A00&query_search=signature&query_type=contains&query=memmove+%7C+LocalBaseRegQueryValue&reason=&release_channels=&build_id=&process_type=any&hang_type=any - 1 crash for Firefox 30: https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A30.0&range_value=4&range_unit=weeks&date=06%2F24%2F2014+11%3A00%3A00&query_search=signature&query_type=contains&query=memmove+|+LocalBaseRegQueryValue&reason=&release_channels=&build_id=&process_type=any&hang_type=any Based on this reports, marking as verified fixed.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•