[clang] Don't use Optional::getValue (NFC)

Don't use Optional::hasValue (NFC)

[clang] Use value_or instead of getValueOr (NFC)

[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.

D1

[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

show more ...

[analyzer][NFC] Prefer using isa<> instead getAs<> in conditions

Depends on D125709

Reviewed By: martong

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

[analyzer][NFC] Remove dead code and modernize surroundings

Thanks @kazu for helping me clean these parts in D127799.

I'm leaving the dump methods, along with the unused visitor handlers and
the fo

[analyzer][NFC] Remove dead code and modernize surroundings

Thanks @kazu for helping me clean these parts in D127799.

I'm leaving the dump methods, along with the unused visitor handlers and
the forwarding methods.

The dead parts actually helped to uncover two bugs, to which I'm going
to post separate patches.

Reviewed By: martong

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

show more ...

[analyzer] Expose Taint.h to plugins

Reviewed By: NoQ, xazax.hun, steakhal

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

[analyzer] Add more propagations to Taint analysis

Add more functions as taint propators to GenericTaintChecker.

Reviewed By: steakhal

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

[analyzer] Add more sources to Taint analysis

Add more functions as taint sources to GenericTaintChecker.

Reviewed By: steakhal

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

[analyzer] Just use default capture after 7fd60ee6e0a87957a718297a4a42d9881fc561e3

[analyzer] Fix -Wunused-lambda-capture in -DLLVM_ENABLE_ASSERTIONS=off builds

Revert "Revert "[analyzer] Fix taint rule of fgets and setproctitle_init""

This reverts commit 2acead35c1289d2b3593a992b0639ca6427e481f.

Let's try `REQUIRES: asserts`.

Revert "Revert "[analyzer] Fix taint propagation by remembering to the location context""

This reverts commit d16c5f4192c30d53468a472c6820163a81192825.

Let's try `REQUIRES: asserts`.

Revert "Revert "[analyzer] Add failing test case demonstrating buggy taint propagation""

This reverts commit b8ae323cca61dc1edcd36e9ae18c7e4c3d76d52e.

Let's try `REQUIRES: asserts`.

Revert "[analyzer] Add failing test case demonstrating buggy taint propagation"

This reverts commit 744745ae195f0997e5bfd5aa2de47b9ea156b6a6.

I'm reverting this since this patch caused a build brea

Revert "[analyzer] Add failing test case demonstrating buggy taint propagation"

This reverts commit 744745ae195f0997e5bfd5aa2de47b9ea156b6a6.

I'm reverting this since this patch caused a build breakage.

https://lab.llvm.org/buildbot/#/builders/91/builds/3818

show more ...

Revert "[analyzer] Fix taint propagation by remembering to the location context"

This reverts commit b099e1e562555fbc67e2e0abbc15074e14a85ff1.

I'm reverting this since the head of the patch stack c

Revert "[analyzer] Fix taint propagation by remembering to the location context"

This reverts commit b099e1e562555fbc67e2e0abbc15074e14a85ff1.

I'm reverting this since the head of the patch stack caused a build
breakage.

https://lab.llvm.org/buildbot/#/builders/91/builds/3818

show more ...

Revert "[analyzer] Fix taint rule of fgets and setproctitle_init"

This reverts commit bf5963bf19670ea58facdf57492e147c13bb650f.

I'm reverting this since the head of the patch stack caused a build
b

Revert "[analyzer] Fix taint rule of fgets and setproctitle_init"

This reverts commit bf5963bf19670ea58facdf57492e147c13bb650f.

I'm reverting this since the head of the patch stack caused a build
breakage.

https://lab.llvm.org/buildbot/#/builders/91/builds/3818

show more ...

[analyzer] Fix taint rule of fgets and setproctitle_init

There was a typo in the rule.
`{{0}, ReturnValueIndex}` meant that the discrete index is `0` and the
variadic index is `-1`.
What we wanted i

[analyzer] Fix taint rule of fgets and setproctitle_init

There was a typo in the rule.
`{{0}, ReturnValueIndex}` meant that the discrete index is `0` and the
variadic index is `-1`.
What we wanted instead is that both `0` and `-1` are in the discrete index
list.

Instead of this, we wanted to express that both `0` and the
`ReturnValueIndex` is in the discrete arg list.

The manual inspection revealed that `setproctitle_init` also suffered a
probably incomplete propagation rule.

Reviewed By: Szelethus, gamesh411

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

show more ...

[analyzer] Fix taint propagation by remembering to the location context

Fixes the issue D118987 by mapping the propagation to the callsite's
LocationContext.
This way we can keep track of the in-fli

[analyzer] Fix taint propagation by remembering to the location context

Fixes the issue D118987 by mapping the propagation to the callsite's
LocationContext.
This way we can keep track of the in-flight propagations.

Note that empty propagation sets won't be inserted.

Reviewed By: NoQ, Szelethus

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

show more ...

[analyzer] Add failing test case demonstrating buggy taint propagation

Recently we uncovered a serious bug in the `GenericTaintChecker`.
It was already flawed before D116025, but that was the patch

[analyzer] Add failing test case demonstrating buggy taint propagation

Recently we uncovered a serious bug in the `GenericTaintChecker`.
It was already flawed before D116025, but that was the patch that turned
this silent bug into a crash.

It happens if the `GenericTaintChecker` has a rule for a function, which
also has a definition.

char *fgets(char *s, int n, FILE *fp) {
nested_call(); // no parameters!
return (char *)0;
}

// Within some function:
fgets(..., tainted_fd);

When the engine inlines the definition and finds a function call within
that, the `PostCall` event for the call will get triggered sooner than the
`PostCall` for the original function.
This mismatch violates the assumption of the `GenericTaintChecker` which
wants to propagate taint information from the `PreCall` event to the
`PostCall` event, where it can actually bind taint to the return value
**of the same call**.

Let's get back to the example and go through step-by-step.
The `GenericTaintChecker` will see the `PreCall<fgets(..., tainted_fd)>`
event, so it would 'remember' that it needs to taint the return value
and the buffer, from the `PostCall` handler, where it has access to the
return value symbol.
However, the engine will inline fgets and the `nested_call()` gets
evaluated subsequently, which produces an unimportant
`PreCall<nested_call()>`, then a `PostCall<nested_call()>` event, which is
observed by the `GenericTaintChecker`, which will unconditionally mark
tainted the 'remembered' arg indexes, trying to access a non-existing
argument, resulting in a crash.
If it doesn't crash, it will behave completely unintuitively, by marking
completely unrelated memory regions tainted, which is even worse.

The resulting assertion is something like this:
Expr.h: const Expr *CallExpr::getArg(unsigned int) const: Assertion
`Arg < getNumArgs() && "Arg access out of range!"' failed.

The gist of the backtrace:
CallExpr::getArg(unsigned int) const
SimpleFunctionCall::getArgExpr(unsigned int)
CallEvent::getArgSVal(unsigned int) const
GenericTaintChecker::checkPostCall(const CallEvent &, CheckerContext&) const

Prior to D116025, there was a check for the argument count before it
applied taint, however, it still suffered from the same underlying
issue/bug regarding propagation.

This path does not intend to fix the bug, rather start a discussion on
how to fix this.

---

Let me elaborate on how I see this problem.

This pre-call, post-call juggling is just a workaround.
The engine should by itself propagate taint where necessary right where
it invalidates regions.
For the tracked values, which potentially escape, we need to erase the
information we know about them; and this is exactly what is done by
invalidation.
However, in the case of taint, we basically want to approximate from the
opposite side of the spectrum.
We want to preserve taint in most cases, rather than cleansing them.

Now, we basically sanitize all escaping tainted regions implicitly,
since invalidation binds a fresh conjured symbol for the given region,
and that has not been associated with taint.

IMO this is a bad default behavior, we should be more aggressive about
preserving taint if not further spreading taint to the reachable
regions.

We have a couple of options for dealing with it (let's call it //tainting
policy//):
1) Taint only the parameters which were tainted prior to the call.
2) Taint the return value of the call, since it likely depends on the
tainted input - if any arguments were tainted.
3) Taint all escaped regions - (maybe transitively using the cluster
algorithm) - if any arguments were tainted.
4) Not taint anything - this is what we do right now :D

The `ExprEngine` should not deal with taint on its own. It should be done
by a checker, such as the `GenericTaintChecker`.
However, the `Pre`-`PostCall` checker callbacks are not designed for this.
`RegionChanges` would be a much better fit for modeling taint propagation.
What we would need in the `RegionChanges` callback is the `State` prior
invalidation, the `State` after the invalidation, and a `CheckerContext` in
which the checker can create transitions, where it would place `NoteTags`
for the modeled taint propagations and report errors if a taint sink
rule gets violated.
In this callback, we could query from the prior State, if the given
value was tainted; then act and taint if necessary according to the
checker's tainting policy.

By using RegionChanges for this, we would 'fix' the mentioned
propagation bug 'by-design'.

Reviewed By: Szelethus

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

show more ...

Fix pair construction with an implicit constructor inside.

[analyzer][NFC] Refactor GenericTaintChecker to use CallDescriptionMap

GenericTaintChecker now uses CallDescriptionMap to describe the possible
operation in code which trigger the introduction (sour

[analyzer][NFC] Refactor GenericTaintChecker to use CallDescriptionMap

GenericTaintChecker now uses CallDescriptionMap to describe the possible
operation in code which trigger the introduction (sources), the removal
(filters), the passing along (propagations) and detection (sinks) of
tainted values.

Reviewed By: steakhal, NoQ

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

show more ...

[analyzer] sprintf is a taint propagator not a source

Due to a typo, `sprintf()` was recognized as a taint source instead of a
taint propagator. It was because an empty taint source list - which is

[analyzer] sprintf is a taint propagator not a source

Due to a typo, `sprintf()` was recognized as a taint source instead of a
taint propagator. It was because an empty taint source list - which is
the first parameter of the `TaintPropagationRule` - encoded the
unconditional taint sources.
This typo effectively turned the `sprintf()` into an unconditional taint
source.

This patch fixes that typo and demonstrated the correct behavior with
tests.

Reviewed By: martong

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

show more ...

[Sema, StaticAnalyzer] Use StringRef::contains (NFC)

[clang] Use llvm::is_contained (NFC)