Closed
Bug 710974
Opened 13 years ago
Closed 12 years ago
Possible bad parsing in SVGNumberList::SetValueFromString()
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Dolske, Unassigned)
References
Details
(Whiteboard: [pvs-studio])
Attachments
(2 files)
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
From http://www.viva64.com/en/a/0078/
Example 7. A non-dereferenced pointer
nsresult
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 2•13 years ago
|
||
Trivial patch.
Updated•13 years ago
|
Whiteboard: [pvs-studio] → [pvs-studio][good first bug][lang=c++]
Comment 3•13 years ago
|
||
Sorry, I didn't realize this was already taken.
Whiteboard: [pvs-studio][good first bug][lang=c++] → [pvs-studio]
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment 4•13 years ago
|
||
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.)
Attachment #581917 -
Flags: review?(dholbert) → review+
Comment 5•13 years ago
|
||
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•12 years ago
|
||
Has a patch and needs a test! I won't have the time to work on this anytime soon, though.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Comment 7•12 years ago
|
||
Updated•12 years ago
|
Attachment #646953 -
Flags: review?(dholbert)
Comment 8•12 years ago
|
||
Comment on attachment 646953 [details] [diff] [review]
tests
>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!
Attachment #646953 -
Flags: review?(dholbert) → review+
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ecb2b4a5a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad2a6ac68ca
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b6ecb2b4a5a5
https://hg.mozilla.org/mozilla-central/rev/cad2a6ac68ca
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•