[all-commits] [llvm/llvm-project] f4fc3f: [analyzer] Treat system globals as mutable if they...

[Balazs Benics via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: f4fc3f6ba319c3c571b6a713a1c38ca1e1e3aae5
      https://github.com/llvm/llvm-project/commit/f4fc3f6ba319c3c571b6a713a1c38ca1e1e3aae5
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2022-06-15 (Wed, 15 Jun 2022)

  Changed paths:
    M clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
    M clang/lib/StaticAnalyzer/Core/MemRegion.cpp
    A clang/test/Analysis/Inputs/some_system_globals.h
    A clang/test/Analysis/Inputs/some_user_globals.h
    A clang/test/Analysis/globals-are-not-always-immutable.c

  Log Message:
  -----------
  [analyzer] Treat system globals as mutable if they are not const

Previously, system globals were treated as immutable regions, unless it
was the `errno` which is known to be frequently modified.

D124244 wants to add a check for stores to immutable regions.
It would basically turn all stores to system globals into an error even
though we have no reason to believe that those mutable sys globals
should be treated as if they were immutable. And this leads to
false-positives if we apply D124244.

In this patch, I'm proposing to treat mutable sys globals actually
mutable, hence allocate them into the `GlobalSystemSpaceRegion`, UNLESS
they were declared as `const` (and a primitive arithmetic type), in
which case, we should use `GlobalImmutableSpaceRegion`.

In any other cases, I'm using the `GlobalInternalSpaceRegion`, which is
no different than the previous behavior.

---

In the tests I added, only the last `expected-warning` was different, compared to the baseline.
Which is this:
```lang=C++
void test_my_mutable_system_global_constraint() {
  assert(my_mutable_system_global > 2);
  clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{TRUE}}
  invalidate_globals();
  clang_analyzer_eval(my_mutable_system_global > 2); // expected-warning {{UNKNOWN}} It was previously TRUE.
}
void test_my_mutable_system_global_assign(int x) {
  my_mutable_system_global = x;
  clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{TRUE}}
  invalidate_globals();
  clang_analyzer_eval(my_mutable_system_global == x); // expected-warning {{UNKNOWN}} It was previously TRUE.
}
```

---

Unfortunately, the taint checker will be also affected.
The `stdin` global variable is a pointer, which is assumed to be a taint
source, and the rest of the taint propagation rules will propagate from
it.
However, since mutable variables are no longer treated immutable, they
also get invalidated, when an opaque function call happens, such as the
first `scanf(stdin, ...)`. This would effectively remove taint from the
pointer, consequently disable all the rest of the taint propagations
down the line from the `stdin` variable.

All that said, I decided to look through `DerivedSymbol`s as well, to
acquire the memregion in that case as well. This should preserve the
previously existing taint reports.

Reviewed By: martong

Differential Revision: https://reviews.llvm.org/D127306


  Commit: 929e60b6bd2f463321b6e98db85a3d9c89236996
      https://github.com/llvm/llvm-project/commit/929e60b6bd2f463321b6e98db85a3d9c89236996
  Author: Balazs Benics <balazs.benics at sigmatechnology.se>
  Date:   2022-06-15 (Wed, 15 Jun 2022)

  Changed paths:
    M clang/lib/StaticAnalyzer/Core/MemRegion.cpp

  Log Message:
  -----------
  [analyzer] Relax constraints on const qualified regions

The arithmetic restriction seems to be artificial.
The comment below seems to be stale.
Thus, we remove both.

Depends on D127306.

Reviewed By: martong

Differential Revision: https://reviews.llvm.org/D127763


Compare: https://github.com/llvm/llvm-project/compare/e1c5afa47d37...929e60b6bd2f