Bug 30347 - Uninitialized conditional in WebCore::CSSParser::validUnit with "width: %" style
Summary: Uninitialized conditional in WebCore::CSSParser::validUnit with "width: %" style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 22772 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-10-13 16:20 PDT by Matt Mueller
Modified: 2009-10-15 16:55 PDT (History)
2 users (show)

See Also:


Attachments
initialize fValue (1013 bytes, patch)
2009-10-13 16:24 PDT, Matt Mueller
no flags Details | Formatted Diff | Diff
Move the non-negative check after the switch (1.34 KB, patch)
2009-10-13 17:06 PDT, Matt Mueller
no flags Details | Formatted Diff | Diff
Move the non-negative check after the switch (1.44 KB, patch)
2009-10-13 17:09 PDT, Matt Mueller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Mueller 2009-10-13 16:20:08 PDT
LayoutTests/fast/css/invalid-percentage-property.html causes a valgrind "Conditional jump or move depends on uninitialised value(s)" error.

validUnit is called with the FNonNeg flag, which checks the fValue before checking anything else, but the "width: %" grammar does not set any fValue.

This does not seem to cause any misbehavior in this case since validUnit will always return false regardless if the FNonNeg check fails or the value->unit tests falls through.  However, it does create valgrind noise which is nice to avoid.

Very similar to bug 22772.

Will attach a patch which addresses this particular case be initializing fValue in the grammar, though I don't know if this is the best way to go about it.  validUnit could be refactored so the check is only done for units where it makes sense, though that might introduce a slight runtime or code size cost.

Chromium bug:
http://code.google.com/p/chromium/issues/detail?id=20939
Comment 1 Matt Mueller 2009-10-13 16:24:40 PDT
Created attachment 41137 [details]
initialize fValue
Comment 2 Darin Adler 2009-10-13 16:36:09 PDT
Comment on attachment 41137 [details]
initialize fValue

I think a better fix for this would be to move the negative number check after the switch statement in CSSParser::validUnit.

    if (b && unitflags & FNonNeg && value->fValue < 0)
        b = false;
Comment 3 Matt Mueller 2009-10-13 17:06:09 PDT
Created attachment 41139 [details]
Move the non-negative check after the switch

Yeah, that does look more general.  Should fix 22772 too, though I don't have a test case for that.  I've updated the patch.
Comment 4 Matt Mueller 2009-10-13 17:09:42 PDT
Created attachment 41140 [details]
Move the non-negative check after the switch

(Fixed changelog)
Comment 5 WebKit Commit Bot 2009-10-14 23:12:47 PDT
Comment on attachment 41140 [details]
Move the non-negative check after the switch

Clearing flags on attachment: 41140

Committed r49609: <http://trac.webkit.org/changeset/49609>
Comment 6 WebKit Commit Bot 2009-10-14 23:12:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Adam Langley 2009-10-15 16:55:28 PDT
*** Bug 22772 has been marked as a duplicate of this bug. ***