nsSHistory uses questionable lifetimes for pointers with GetNext()

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({sec-audit})

Trunk
mozilla50
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main50-])

Attachments

(1 attachment)

There's a number of places in nsSHistory that do something like this:

  nsCOMPtr<nsISHTransaction> trans = mListRoot;
  while (trans) {
    ... do some stuff with trans ...

    nsISHTransaction *temp = trans;
    temp->GetNext(getter_AddRefs(trans));
  }

If |trans| had the last reference to the object, then when we assign to |trans| inside of GetNext(), then |this| of |GetNext()| is dead.

As it happens, GetNext() does not look like it touches |this| after setting the return value, and it looks like the places these are being use we are never going to have the last reference to the object, so I think it is safe, but it seems like it would be good to be less fragile here.

A fixed version of the body would look like:
    nsCOMPtr<nsISHTransaction> temp = trans.forget();
    temp->GetNext(getter_AddRefs(trans));

Then we'd be sure to keep |temp| alive for the duration of the getter.  This should not cause any additional refcount traffic.

Olli, do you agree that this code is safe as-is?  Do you think we should change it?

I noticed this while looking at poiru's patch in bug 1129795.
Flags: needinfo?(bugs)
Ancient code, but yeah, looks like it is safe, but we also should fix it to be less
error prone.
Flags: needinfo?(bugs)
Assignee: nobody → continuation
Group: core-security → dom-core-security
Assignee: continuation → nobody
Assignee: nobody → continuation
I feel like I can take this review if you want to hand it off, smaug.
Comment on attachment 8775805 [details] [diff] [review]
Use strong references during iteration in nsSHistory.

thanks mconley, but I happened to look at the patch before I noticed your comment.
Attachment #8775805 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/2882620cd75f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.