Closed Bug 1141788 Opened 8 years ago Closed 7 years ago

nsSHistory uses questionable lifetimes for pointers with GetNext()


(Core :: DOM: Navigation, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: mccr8, Assigned: mccr8)


(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main50-])


(1 file)

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;

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();

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+
Closed: 7 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.