[clang] [analyzer] TaintPropagation checker strlen() should not propagate (PR #66086)

[Balazs Benics via cfe-commits]

steakhal wrote:

I finished the review of this PR.

By looking at the disappeared reports you attached, I'm convinced that the `MsgTaintedBufferSize` diagnostics give little to no benefit in general. On the other side, I've seen good hits for OOBV2 in the presence of taint - even if that's rarely the case. On the theory side, I also believe that propagation should happen on `strlen` and similar functions.

Consequently, I agree with the raised problems, but I disagree with the approach.
I would rather remove the `MsgTaintedBufferSize` diagnostic to resolve those FPs.
Alternatively, we can also think of creating a heuristic to reduce such FPs. For e.g. check if the most significant bit of the allocation size is proven to be unset (aka. checked some meaningful upperbounds)  and suppress reports in that case, but report otherwise.
Would it be okay with you to proceed by not removing taint propagation?

On the same token, I think we should be able to separately enable/disable diagnostics on the GenericTaintChecker (including that they should not sink execution paths if they are disabled); but that's a different subject.


https://github.com/llvm/llvm-project/pull/66086