Closed Bug 713577 Opened 14 years ago Closed 14 years ago

Clang Static Analysis: Operand in multiplication is garbage value in content/svg/content/src/nsSVGFilters.cpp

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The following (almost identical) reports have been generated by static analysis using Clang: http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-Nd3e7L.html#EndPath http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-yCjuGu.html#EndPath http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-R2SqdD.html#EndPath It would be good if someone familiar with the particular code could check if - this is really a bug or a false positive - and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization). In these particular reports, the problems seems to be that depending on the value of | values.Length(); |, one of the multiplications later may be using uninitialized/garbage values. The length assumed by the analysis can be seen in line 1203, depending on how many times the loop is executed.
The code ensures that values.Length() == 20 and that all 20 entries of colorMatrix are initialized. That's the loop on lines 1203 and 1204. The loop condition on line 1276 ensures the following invariants: 1) 0 <= i < 4 2) row = 5*i. So the maximal value of |row| is 15. Hence row+3 and row+4 are at most 18 and 19, which are inside the initialized colorMatrix array (the last two entries of it, in fact). So this looks like a false-positive to me at first glance. I really don't know how to make clang not flag this offhand short of changing the line 1276 loop condition to compare row to 20 instead of i to 4, maybe? It may be more worthwhile to see whether the clang bug can be fixed.
First of all, thanks for looking into this :) (In reply to Boris Zbarsky (:bz) from comment #1) > The code ensures that values.Length() == 20 Where is that ensured? It's probably just that what's being missed by the analysis, because it assumes | values.Length() | to me smaller (and in that case, the result would be correct).
The value of values.Length() is checked somewhat higher up in a switch statement. If type == SVG_FECOLORMATRIX_TYPE_MATRIX then we have this: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.cpp#1200 All 20 values in the colorMatrix are always initialised. And the index never exceeds 19 i < 4 therefore row < 15 and so row + 4 < 19
Attached patch hack — — Splinter Review
It looks like the problem is that it is not figuring out that the two calls to Length return the same value. The attached patch hides the warning. I will try figure out if this can be improved on the clang side.
Doesn't seem valid as an SVG bug.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: