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




6 years ago
6 years ago


(Reporter: decoder, Unassigned)


(Blocks: 1 bug)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 years ago
The following (almost identical) reports have been generated by static analysis using Clang:

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.

Comment 2

6 years ago
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:

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
Created attachment 584430 [details] [diff] [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.
I reported
Doesn't seem valid as an SVG bug.
Last Resolved: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.