482439 - potential null-pointer dereference in nsDocument::ElementFromPointHelper

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: fix while() condition to avoid possible null-deref

This function includes the fragment:

  while (ptContent &&
         !ptContent->IsNodeOfType(nsINode::eELEMENT) ||
         ptContent->IsInAnonymousSubtree()) {
    // XXXldb: Faster to jump to GetBindingParent if non-null?
    ptContent = ptContent->GetParent();
  }

(see http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2674).

This code is incorrect because && has higher precedence than ||, and therefore if ptContent ever becomes NULL during this loop, it will still attempt to call IsInAnonymousSubtree(). It seems clear that the intention was to check for non-NULL ptContent before calling either of the Is... methods.

I don't have an actual test-case that encounters this situation; perhaps it cannot arise in actual practice. However, the fact that the code tests for non-NULL ptContent at all (both here, and before calling CallQueryInterface in the next couple of lines of code) implies that it is considered possible. Either these tests are completely redundant, or the while() code is unsafe.

The attached patch fixes the unsafe test by simply adding parens around the || alternatives, providing the semantics that were presumably intended all along.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 366537 [details] [diff] [review]
fix while() condition to avoid possible null-deref

I guess you were going to ask review from someone.

Comment 2

[Jonathan Kew [:jfkthame]]

Uhhhh.... yeah! Thanks.

Comment 3

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/cf256da7436b