[all-commits] [llvm/llvm-project] c87729: [analyzer] Wrap SymbolicRegions by ElementRegions ...

[Balazs Benics via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: c8772940ee4d85b1a4578b3faea7c825300ce59f
      https://github.com/llvm/llvm-project/commit/c8772940ee4d85b1a4578b3faea7c825300ce59f
  Author: Balazs Benics <benicsbalazs at gmail.com>
  Date:   2024-03-21 (Thu, 21 Mar 2024)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
    M clang/lib/StaticAnalyzer/Core/ProgramState.cpp
    M clang/test/Analysis/inlining/false-positive-suppression.cpp

  Log Message:
  -----------
  [analyzer] Wrap SymbolicRegions by ElementRegions before getting a FieldRegion (#85211)

Inside the ExprEngine when we process the initializers, we create a
PostInitializer program-point, which will refer to the field being
initialized, see `FieldLoc` inside `ExprEngine::ProcessInitializer`.

When a constructor (of which we evaluate the initializer-list) is
analyzed in top-level context, then the `this` pointer will be
represented by a `SymbolicRegion`, (as it should be).

This means that we will form a `FieldRegion{SymbolicRegion{.}}` as the
initialized region.

```c++
class Bear {
public:
  void brum() const;
};
class Door {
public:
  // PostInitializer would refer to "FieldRegion{SymRegion{this}}"
  // whereas in the store and everywhere else it would be:
  // "FieldRegion{ELementRegion{SymRegion{Ty*, this}, 0, Ty}".
  Door() : ptr(nullptr) {
    ptr->brum(); // Bug
  }
private:
  Bear* ptr;
};
```

We (as CSA folks) decided to avoid the creation of FieldRegions directly
of symbolic regions in the past:

https://github.com/llvm/llvm-project/commit/f8643a9b31c4029942f67d4534c9139b45173504

---

In this patch, I propose to also canonicalize it as in the mentioned
patch, into this: `FieldRegion{ElementRegion{SymbolicRegion{Ty*, .}, 0,
Ty}`

This would mean that FieldRegions will/should never simply wrap a
SymbolicRegion directly, but rather an ElementRegion that is sitting in
between.

This patch should have practically no observable effects, as the store
(due to the mentioned patch) was made resilient to this issue, but we
use `PostInitializer::getLocationValue()` for an alternative reporting,
where we faced this issue.

Note that in really rare cases it suppresses now dereference bugs, as
demonstrated in the test. It is because in the past we failed to follow
the region of the PostInitializer inside the StoreSiteFinder visitor -
because it was using this code:
```c++
// If this is a post initializer expression, initializing the region, we
// should track the initializer expression.
if (std::optional<PostInitializer> PIP =
        Pred->getLocationAs<PostInitializer>()) {
  const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue();
  if (FieldReg == R) {
    StoreSite = Pred;
    InitE = PIP->getInitializer()->getInit();
  }
}
```
Notice that the equality check didn't pass for the regions I'm
canonicalizing in this patch.

Given the nature of this change, we would rather upstream this patch.

CPP-4954



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications