Closed
Bug 216474
Opened 21 years ago
Closed 21 years ago
Browser Crashes in Password Manager
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firebird0.7
People
(Reporter: bugs, Assigned: bugs)
Details
Attachments
(1 file, 1 obsolete file)
1.43 KB,
patch
|
bryner
:
review+
|
Details | Diff | Splinter Review |
I visit a particular site, and as the password manager attempts to complete my login information the browser crashes in: nsPasswordManager::OnStateChange at: passField->SetValue(buffer); ('passField' is null...) Adding a null check for passField caused it to crash a couple of lines further up ('nameField' is null...) It seems that it is possible to get into a state where you exit the loop over all the SignonDataEntries and 'firstMatch' is NON-NULL and yet 'nameField' and 'passField' are null. Here's why I think this is happening - For this site my password file has THREE entries ... not exactly sure why, possibly related to the old password manager. The loop in OnStateChange iterates through each entry... the first entry is invalid - the username field is identified by a 'name' attribute that does not exist in the document, so userField is null the loop continues to the next iteration. The next entry has valid username and password form names and so firstMatch is set to the entry, and the loop continues. The third time through, the final SignonDataEntry contains bad data, i.e. strings that don't match the name attribute of any form element in the current form. As such, 'userField' is null and the loop continues, but since I only have three entries for this site, it stops iterating at this point and continues past the loop. Note now that 'firstMatch' was set when we found the correct password entry during the SECOND iteration, but that 'nameField' was set to null during the THIRD iteration when we failed to find a form element for the name field specified in that entry. Since 'firstMatch' is non-null, we enter the if { } block after the loop and subsequently crash when we try to dereference the null pointer that is nameField. My first reaction was to put a null check for nameField and passField in the if block condition, but I realized this would only fix the crash, and would result in the fields not being autocompleted because of the third, bad data entry interfering with the second, good one. So I'm proposing this fix... Change this pattern: form->ResolveName(e->userField, getter_AddRefs(foundNode)); userField = do_QueryInterface(foundNode); if (!userField) continue; to this one: form->ResolveName(e->userField, getter_AddRefs(foundNode)); if (foundNode) userField = do_QueryInterface(foundNode); else continue; where the userField variable is only updated if this data entry is legitimate. A patch is attached.
Assignee | ||
Comment 1•21 years ago
|
||
This patch avoids the crash and makes sure login information from bad SignonDataEntries is ignored.
Assignee | ||
Comment 2•21 years ago
|
||
Need this for .7 ;-)
Priority: -- → P1
Target Milestone: --- → Firebird0.7
Assignee | ||
Comment 3•21 years ago
|
||
bryner points out that the previous patch will fail if foundNode is non-null but won't QI to an input element. This patch does the QI and returns to a temp, then only assigns the temp to the final variable if the QI succeeded (temp non-null)
Attachment #129947 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #129949 -
Flags: review+
Assignee | ||
Comment 5•21 years ago
|
||
fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•