Bug 30502 – Dereference of uninitialized variable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kent Tamura]

WebCore/dom/Element.cpp setBooleanAttribute();

        ExceptionCode ex;
        removeAttribute(name, ex);

removeAttribute():
  void Element::removeAttribute(const QualifiedName& name, ExceptionCode& ec)
  {
      if (namedAttrMap) {
          namedAttrMap->removeNamedItem(name, ec);
          if (ec == NOT_FOUND_ERR)
              ec = 0;
      }
  }

removeNamedItem() doesn't set any value to ec if the specified attribute exists.  So, if removeAttribute() is called by setBooleanAttribute(), uninitialized ec can be referred at "if (ec == NOT_FOUND_ERR)".

Note: This never makes a real bug because ex in setBooleanAttribute() is not used after removeAttribute() call.
Valgrind complaints about this.

[Kent Tamura]

Created attachment 41398 [details]
Proposed patch

[Mark Rowe (bdash)]

Wouldn’t it make more sense to initialize the variable at the level that it matters?  setBooleanAttribute doesn’t care about the exception code so there’s no need for it to do anything with it.  After your change the code in removeAttribute will still do the wrong thing if called by another caller.

[Kent Tamura]

(In reply to comment #2)
> Wouldn’t it make more sense to initialize the variable at the level that it
> matters?  setBooleanAttribute doesn’t care about the exception code so there’s
> no need for it to do anything with it.  After your change the code in
> removeAttribute will still do the wrong thing if called by another caller.

I think our convention for ExceptionCode is "initialize it with 0 when it is declared."

[Kent Tamura]

> I think our convention for ExceptionCode is "initialize it with 0 when it is
> declared."

So, if removeAttribute() set 0 before removenamedItem() and other callers initialize ec with 0, we would have redundant initialization.

[Mark Rowe (bdash)]

(In reply to comment #3)
> (In reply to comment #2)
> > Wouldn’t it make more sense to initialize the variable at the level that it
> > matters?  setBooleanAttribute doesn’t care about the exception code so there’s
> > no need for it to do anything with it.  After your change the code in
> > removeAttribute will still do the wrong thing if called by another caller.
> 
> I think our convention for ExceptionCode is "initialize it with 0 when it is
> declared.”

A quick grep in WebCore for “ExceptionCode ec;” shows that is not the case.

[Alexey Proskuryakov]

Comment on attachment 41398 [details]
Proposed patch

As Mark said - if a caller doesn't plan to look at ExceptionCode, it can pass an uninitialized value. See bug 21939 for more detailed discussion of the current policy (especially comments 3 and 12 there).

[Kent Tamura]

Created attachment 41400 [details]
Proposed patch (rev.2)

ok, I follow it.

Updated the patch so that removeAttribute() initializes ec.

[Alexey Proskuryakov]

Comment on attachment 41400 [details]
Proposed patch (rev.2)

r=me

It may make sense to add a comment explaining that this assignment is only needed to please tools like valgrind - since it's useless otherwise, people will be tempted to remove it.

[Yong Li]

Comment on attachment 41400 [details]
Proposed patch (rev.2)

Let commit bot land it

[WebKit Commit Bot]

Comment on attachment 41400 [details]
Proposed patch (rev.2)

Rejecting patch 41400 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11468 test cases.
http/tests/navigation/postredirect-reload.html -> crashed

Exiting early after 1 failures. 8689 tests run.
390.95s total testing time

8688 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Comment on attachment 41400 [details]
Proposed patch (rev.2)

That crash looks unrelated to your patch.  I've filed bug 30519.  Adding this back to the commit-queue.

[WebKit Commit Bot]

Comment on attachment 41400 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 41400

Committed r49792: <http://trac.webkit.org/changeset/49792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.