Bug 30502

Summary: Dereference of uninitialized variable
Product: WebKit Reporter: Kent Tamura <tkent>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, commit-queue
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
ap: review-
Proposed patch (rev.2) none

Kent Tamura
Reported 2009-10-18 22:48:34 PDT
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.
Attachments
Proposed patch (1.36 KB, patch)
2009-10-18 23:11 PDT, Kent Tamura 		ap: review-
DetailsFormatted DiffDiff
Proposed patch (rev.2) (1.31 KB, patch)
2009-10-19 00:30 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2009-10-18 23:11:45 PDT
Created attachment 41398 [details] Proposed patch
Mark Rowe (bdash)
Comment 2 2009-10-18 23:52:14 PDT
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
Comment 3 2009-10-18 23:55:30 PDT
(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
Comment 4 2009-10-19 00:00:56 PDT
> 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)
Comment 5 2009-10-19 00:14:54 PDT
(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 6 2009-10-19 00:20:04 PDT
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
Comment 7 2009-10-19 00:30:01 PDT
Created attachment 41400 [details] Proposed patch (rev.2) ok, I follow it. Updated the patch so that removeAttribute() initializes ec.
Alexey Proskuryakov
Comment 8 2009-10-19 08:14:34 PDT
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 9 2009-10-19 08:49:35 PDT
Comment on attachment 41400 [details] Proposed patch (rev.2) Let commit bot land it
WebKit Commit Bot
Comment 10 2009-10-19 09:42:44 PDT
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 11 2009-10-19 10:11:57 PDT
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 12 2009-10-19 10:36:06 PDT
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
Comment 13 2009-10-19 10:36:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.