711479 - potential NULL pointer dereference in content/base/src/nsDocument.cpp

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Julia Lawall]

User Agent: Mozilla/5.0 (Ubuntu; X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111115183158

Steps to reproduce:

The following potential NULL pointer dereference was found by Coccinelle (http://coccinelle.lip6.fr) in the file content/base/src/nsDocument.cpp in the function nsDocument::FullScreenStackPop.  Element can be NULL in the then branch of the conditional, in which case the dereference is invalid.

    if (!element || !element->IsInDoc() || element->OwnerDoc() != this) {
      NS_ASSERTION(!element->IsFullScreenAncestor(),
                   "Should have already removed full-screen styles");

Comment 1

[:aceman]

Thanks, the problem looks valid.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In fact, the null-check is redundant. FullScreenStackTop() asserts that it returns non-null if !mFullScreenStack.IsEmpty(), and this check is in a while (!mFullScreenStack.IsEmpty()) loop.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Chris, can you take a look?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Assert that FullScreenStackTop() returns a non-null element when mFullScreenStack.IsEmpty() is false;

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 582760 [details] [diff] [review]
Assert that FullScreenStackTop() returns a non-null element when mFullScreenStack.IsEmpty() is false;

Review of attachment 582760 [details] [diff] [review]:
-----------------------------------------------------------------

FullScreenStackTop() can return a null element; it returns the front element in a queue of weak references, the weak references can be annulled if the object they point to are GC'd.

We should just change the assertion on line 8777 to:

      NS_ASSERTION(!element || !element->IsFullScreenAncestor(),
                   "Should have already removed full-screen styles");

Thanks for pointing out this issue!

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So the assertions in <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8757> are all lies?

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes. Oops.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Actually no, they're not. FullScreenStackTop() will always return non-null, because the only way the full-screen element can be GC'd is if it's removed from the document, and we exit full-screen in that case.

The only way the full-screen element can change is when we call FullScreenStackPop() which makes the top-most non-null element as the full-screen element, so then we'd be covered by the former case to ensure it's non-null.

So those assertions are still correct, but we could definitely use a comment explaining the above...

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Correct assertions around FullScreenStackTop();

Like this, then?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Assert that FullScreenStackTop() returns a non-null element when mFullScreenStack.IsEmpty() is false;

Or like this?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 583104 [details] [diff] [review]
Correct assertions around FullScreenStackTop();

Review of attachment 583104 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with the assertion "NS_ASSERTION(element, "Should have full-screen element!");" put back in.

::: content/base/src/nsDocument.cpp
@@ -8760,5 @@
>      return nsnull;
>    }
>    PRUint32 last = mFullScreenStack.Length() - 1;
>    nsCOMPtr<Element> element(do_QueryReferent(mFullScreenStack[last]));
> -  NS_ASSERTION(element, "Should have full-screen element!");

Leave this assertion in; if the full-screen stack non-empty and the top-most element in the stack is null, we're in an incorrect state.

@@ -8762,5 @@
>    PRUint32 last = mFullScreenStack.Length() - 1;
>    nsCOMPtr<Element> element(do_QueryReferent(mFullScreenStack[last]));
> -  NS_ASSERTION(element, "Should have full-screen element!");
> -  NS_ASSERTION(element->IsInDoc(), "Full-screen element should be in doc");
> -  NS_ASSERTION(element->OwnerDoc() == this, "Full-screen element should be in this doc");

Yup, go ahead and remove these assertions and replace them with your new one.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 583109 [details] [diff] [review]
Assert that FullScreenStackTop() returns a non-null element when mFullScreenStack.IsEmpty() is false;

Review of attachment 583109 [details] [diff] [review]:
-----------------------------------------------------------------

Let's take the other patch instead. Thanks!

::: content/base/src/nsDocument.cpp
@@ +8753,5 @@
>      }
>    }
>  }
>  
> +// FullScreenStackTop() will always return non-null, because the only way the

Let's move this comment into the other patch, but let's make that "FullScreenStackTop() should always return non-null..."

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.