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

RESOLVED INVALID

Status

()

Core
SVG
RESOLVED INVALID
6 years ago
6 years ago

People

(Reporter: decoder, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

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:

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
Created attachment 584430 [details] [diff] [review]
hack

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 llvm.org/pr11658.
Doesn't seem valid as an SVG bug.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.