crash in memmove | LocalBaseRegQueryValue

VERIFIED FIXED in Firefox 30

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: jkitch, Assigned: jkitch)

Tracking

({crash, regression})

unspecified
mozilla31
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30+ verified, firefox31+ verified, b2g-v1.4 fixed)

Details

(crash signature)

Attachments

(2 attachments)

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.
Keywords: regression
Posted patch patchSplinter Review
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)
Attachment #8411634 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05519890c1cc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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 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)
(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)
Attachment #8418042 - Flags: review?(benjamin) → review+
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?
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 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+
You need to log in before you can comment on or make changes to this bug.