NagyDonat wrote: > Let me forward something. It looks relevant and actionable: > > * The new report is emitted through `reportOOB` → `generateErrorNode`, i.e. a > sink, even though `mayBeInBounds()` is true on these paths. Previously an > underflow-possible-but-in-bounds tainted access continued with > `Res.getInBoundsState()` and only got the "Assuming offset is non-negative" > note tag. Cutting the path is consistent with the existing tainted-overflow > behavior, but it does mean the rest of such a function is no longer > explored. Was that trade-off considered, or would a non-fatal error node > plus the in-bounds transition be preferable for the taint reports?
I thought about the non-fatal vs fatal nature of the "potential out of bounds" reports and discussed it within my colleagues; IIRC we came to the conclusion that it is slightly better to use fatal error nodes. @steakhal If you disagree, feel free to start a discussion (e.g. discord, discourse etc.) > * Is there a new false-positive class where the lower-bound validation is > lost > to the unmodeled-`SymbolCast` problem ([[Umbrella] Constraint solver > improvements. #39492](https://github.com/llvm/llvm-project/issues/39492)) > while the upper bound _is_ > provable? **Checked: no.** With the patch, > ```c > int index; scanf("%d", &index); > if ((unsigned)index < 10) > TenElements[index] = 5; // no warning > unsigned uidx = (unsigned)index; > if (uidx < 10) > TenElements[index] = 5; // no warning > ``` > > both stay silent, while the signed > upper-bound-only guard in the same file does produce the new > "may be negative" report. The range inferrer pushes the constraint > through > the signed→unsigned cast, so the idiom is safe. Still might be worth a > regression test, since nothing currently pins this down. I added this test in commit https://github.com/llvm/llvm-project/pull/216077/commits/1c13e375c0ef0de30a50dd4ce31e5516761dadf0 . > * Would you add a pointer-arithmetic counterpart of the new test (tainted > _offset_ that can only underflow, e.g. `p = TenElements + index; p[0]` > with > `index` bounded above) so the "tainted offset that may be negative" > wording > is covered too? The use of "offset" vs "index" is clearly orthogonal to the use of "negative" vs "too large" vs "negative or too large" (and I don't foresee that future changes could mess this up), so I don't want to test each possible combination. https://github.com/llvm/llvm-project/pull/216077 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
