================
@@ -838,8 +860,15 @@ void 
CStringChecker::emitUninitializedReadBug(CheckerContext &C,
 void CStringChecker::emitOutOfBoundsBug(CheckerContext &C,
                                         ProgramStateRef State, const Stmt *S,
                                         StringRef WarningMsg) const {
+  // FIXME: This is absurd. If a checker is disabled, we just emit the bug
----------------
isuckatcs wrote:

After digging further, I think one of these names is never used. Below is the 
only callsite of the function.

```c++
ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C,
                                              ProgramStateRef state,
                                              AnyArgExpr Buffer, SVal Element,
                                              AccessKind Access,
                                              CharKind CK) const {

  ...
  if (StOutBound && !StInBound) {
    // These checks are either enabled by the CString out-of-bounds checker
    // explicitly or implicitly by the Malloc checker.
    // In the latter case we only do modeling but do not emit warning.
    if (!Filter.CheckCStringOutOfBounds)
      return nullptr;

    // Emit a bug report.
    ErrorMessage Message =
        createOutOfBoundErrorMsg(CurrentFunctionDescription, Access);
    emitOutOfBoundsBug(C, StOutBound, Buffer.Expression, Message);
    return nullptr;
  }

  ...
}
```

It is clear that if `Filter.CheckCStringOutOfBounds` is not enabled, the bug is 
never emited, so it can only be that check. 

I was doing my homework properly, so here is the full story. Initially it's 
been only one name, and 
[2ff57bc](https://github.com/llvm/llvm-project/commit/2ff57bcd18d3514c3f5baed6d34e02f885e81b28)
 introduced the other one. Later [D48831](https://reviews.llvm.org/D48831) has 
basically reverted the change, because the warning couldn't be disabled. 

As for why the other name became `Filter.CheckCStringNullArg` in the first 
place, I have the following theory. `CheckLocation()` is only called from 
`CheckBufferAccess()`, where it is guarded by `Filter.CheckCStringOutOfBounds` 
and `evalStrcpyCommon()`, where the message with the second name can come from. 

In `evalStrcpyCommon()` we call `checkNonNull()`, `CheckOverlap()`, 
`checkAdditionOverflow()` and `CheckLocation()`. Before the first patch, only 
`checkNonNull()` and `CheckLocation()` could report any warning when 
`Filter.CheckCStringOutOfBounds` was disabled, however `checkNonNull()` only 
reports if `Filter.CheckCStringNullArg` is enabled. 

The author of the commit must have thought there is a connection. Maybe the 
malloc checker only enabled `CStringNullArg` by default (though this no longer 
seems to be the case) and it looked like that checker is causing the warnings. 
In theory the warning would have been reported regardless of which checks were 
enabled or disabled I think.

https://github.com/llvm/llvm-project/pull/113312
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to