Last Comment Bug 710974 - Possible bad parsing in SVGNumberList::SetValueFromString()
: Possible bad parsing in SVGNumberList::SetValueFromString()
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
: Jet Villegas (:jet)
Depends on:
Blocks: 710966 589439
  Show dependency treegraph
Reported: 2011-12-14 22:51 PST by Justin Dolske [:Dolske]
Modified: 2012-07-31 06:14 PDT (History)
5 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (915 bytes, patch)
2011-12-15 01:58 PST, Tim Taubert [:ttaubert]
dholbert: review+
Details | Diff | Splinter Review
tests (2.89 KB, patch)
2012-07-29 02:10 PDT, Robert Longson
dholbert: review+
Details | Diff | Splinter Review

Description User image Justin Dolske [:Dolske] 2011-12-14 22:51:00 PST

Example 7. A non-dereferenced pointer

SVGNumberList::SetValueFromString(const nsAString& aValue)
  const char *token = str.get();
  if (token == '\0') {
    return NS_ERROR_DOM_SYNTAX_ERR; // nothing between commas

PVS-Studio diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *token == '\0'. svgnumberlist.cpp 96

The code checking that there's nothing between the commas does not work. To find out whether or not the string is empty, we can compare the first character to '\0'. But it is the pointer which is compared to null instead of the first character. This pointer is never equal to zero. This is the correct check: (*token == '\0').
Comment 1 User image Tim Taubert [:ttaubert] 2011-12-15 01:55:37 PST
This was introduced in bug 589439.
Comment 2 User image Tim Taubert [:ttaubert] 2011-12-15 01:58:15 PST
Created attachment 581917 [details] [diff] [review]
patch v1

Trivial patch.
Comment 3 User image Jared Wein [:jaws] (please needinfo? me) 2011-12-15 02:39:05 PST
Sorry, I didn't realize this was already taken.
Comment 4 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-12-15 03:16:18 PST
Comment on attachment 581917 [details] [diff] [review]
patch v1

Good catch!

(Side note: It looks like this isn't dangerous at all, nor does it even affect behavior, I think.  In cases where we *should* take the early-return but fail to do so, we'll just end up taking the slightly-later early return a few lines later, because (*end != '\0') will be true.)
Comment 5 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-12-15 03:22:18 PST
oh, I misread the (*end != '\0') as being == instead of !=.  So maybe this does affect behavior, not sure.  If so, we should add a test for it.
Comment 6 User image Tim Taubert [:ttaubert] 2012-07-11 06:39:19 PDT
Has a patch and needs a test! I won't have the time to work on this anytime soon, though.
Comment 7 User image Robert Longson 2012-07-29 02:10:51 PDT
Created attachment 646953 [details] [diff] [review]
Comment 8 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-07-29 14:38:42 PDT
Comment on attachment 646953 [details] [diff] [review]

>diff --git a/content/svg/content/test/test_SVGLengthList.xhtml b/content/svg/content/test/test_SVGLengthList.xhtml
>+  // -- Invalid attribute
>+  eventChecker.expect("modify");
>+  text.setAttribute("x", ",20");
>+  is(lengths.numberOfItems, 0, 'Checking numberOfItems');

In the event that this fails, "Checking numberOfItems" is a bit of a cryptic error message. Maybe "Checking that invalid values won't successfully parse" or something?

r=me with something like that. :) Thanks!

Note You need to log in before you can comment on or make changes to this bug.