NagyDonat wrote: Quick replies to to the quick reply (Iwill try to give a proper review later :sweat_smile:):
> 3\. > [vim/src/syntax.c](https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=off&diff-type=New&checker-name=alpha.unix.cstring.UninitializedRead&report-hash=1fe0c22eeaa0c31c184abf14b09dbf92&report-id=8060099&report-filepath=vim%2Fsrc%2Fsyntax.c): > It's somewhat funny in the example that it reports that `char_u > buf_chartab[32]` was "'buf_chartab' initialized here" - while in fact it was > just declared there without any initialization and that was the point of the > report. I think this will be confusing to our users. IIRC that message comes from the bug reporter visitors and we get it automagically via `trackExpressionValue()`. Definitely deserves a simple patch to fix it and I think this is feasible to fix this without getting mobbed by the skeletons in the ~~closet~~ bug reporter visitor code. > As a generic note, the error message could/should be probably improved, > because right now the ` The first element of the 2nd argument is undefined` > is not too helpful. The problem is that it could mean two things in the > engine: > > * The location it refers to was never initialized. (like in example 3) > > * The location might have been initialized, but we formed an out-of-bounds > pointer (such as a pointer to the end of a buffer, aka. 1 past last element), > and we pass that to something that will dereference it. -- I find these cases > a lot more difficult to decipher in practice. The problem is that the engine is repeating the job of the `security.ArrayBound` checker and produces `Undefined` values when _it_ sees an out-of-bounds access that was somehow not detected by the `ArrayBound` checker. In practice this discrepancy may occur in two situations: 1. The `ArrayBound` checker is disabled by the user. In this case it is reasonable to say that they don't want to see these indirect out of bounds reports either. 2. Either `ArrayBound` or the logic in the engine is buggy. As I spent lots of time on improving `ArrayBound` but AFAIK the engine still uses the logic of `ArrayBoundV1`, I would rather trust the `ArrayBound` checker. To fix this, we could ensure that the engine _also_ invokes the up-to-date logic that is used by the `ArrayBound` checker, but that still doesn't resolve the "doing the same job twice" situation. Therefore I propose that **the engine should return `UnknownVal` instead of `UndefinedVal` when it thinks that it is reading out-of-bounds memory**. This behavior is only relevant if `ArrayBound` is not enabled, and in that case I think it is better to not report these indirect reports either (which are IMO significantly harder to understand than the direct out-of-bounds reports). @steakhal @gamesh411 What do you think about this proposal? https://github.com/llvm/llvm-project/pull/196292 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
