Fixes#123459.
This changes checking of the returned expr to also look for memory
regions whose stack frame context was a child of the current stack frame
context, e.g., for cases like this given in #123459:
```
struct S { int *p; };
S f() {
S s;
{
int a = 1;
s.p = &a;
}
return s;
}
```
Reapplying changes from https://github.com/llvm/llvm-project/pull/125638
after buildbot failures.
Buildbot failures fixed in 029e7e98dc9956086adc6c1dfb0c655a273fbee6,
latest commit on this PR. It was a problem with a declared class member
with same name as its type. Sorry!
Fixes https://github.com/llvm/llvm-project/issues/123459.
Previously, when the StackAddrEscapeChecker checked return values, it
did not scan into the structure of the return SVal. Now it does, and we
can catch some more false negatives that were already mocked out in the
tests in addition to those mentioned in
https://github.com/llvm/llvm-project/issues/123459.
The warning message at the moment for these newly caught leaks is not
great. I think they would be better if they had a better trace of why
and how the region leaks. If y'all are happy with these changes, I would
try to improve these warnings and work on normalizing this SVal checking
on the `checkEndFunction` side of the checker also.
Two of the stack address leak test cases now have two warnings, one
warning from return expression checking and another from`
checkEndFunction` `iterBindings` checking. For these two cases, I prefer
the warnings from the return expression checking, but I couldn't figure
out a way to drop the `checkEndFunction` without breaking other
`checkEndFunction` warnings that we do want. Thoughts here?
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---------
This is the last of the 3 stacked PRs, it must not be merged before
https://github.com/llvm/llvm-project/pull/105652 and
https://github.com/llvm/llvm-project/pull/105653
This commit ensures that the `CallDescription`s in `MallocChecker` are
matched with the mode `CDM::CLibrary`, so:
- they don't match methods or functions within user-defined namespaces;
- they also match builtin variants of these functions (if any), so the
checker can model `__builtin_alloca()` like `alloca()`.
This change fixes https://github.com/llvm/llvm-project/issues/81597. New
tests were added to verify that `std::malloc` and `std::free` (from
`<cstdlib>`) are modeled, but a method that's named e.g. `free` isn't
confused with the memory release function.
The responsibility for modeling `__builtin_alloca` and
`__builtin_alloca_with_align` was moved from `BuiltinFunctionChecker` to
`MallocChecker`, to avoid buggy interactions between the checkers and
ensure that the builtin and non-builtin variants are handled by exactly
the same logic.
This change might be a step backwards for the users who don't have
`unix.Malloc` enabled; but I suspect that `__builtin_alloca()` is so
rare that it would be a waste of time to implement backwards
compatibility for them.
There were several test files that relied on `__builtin_alloca()` calls
to get an `AllocaRegion`, these were modified to enable `unix.Malloc`.
One of these files (cxx-uninitialized-object-ptr-ref.cpp) had some tests
that relied on the fact that `malloc()` was treated as a "black box" in
them, these were updated to use `calloc()` (to get initialized memory)
and `free()` (to avoid memory leak reports).
While I was developing this change, I found a very suspicious assert in
`MallocChecker`. As it isn't blocking the goals of this commit, I just
marked it with a FIXME, but I'll try to investigate and fix it in a
follow-up change.
without prototypes. This patch converts the function signatures to have
a prototype for the situations where the test is not specific to K&R C
declarations. e.g.,
void func();
becomes
void func(void);
This is the ninth batch of tests being updated (there are a
significant number of other tests left to be updated).
Summary:
Leaking a stack address via a static variable refers to it in the diagnostic as a 'global'. This patch corrects the diagnostic for static variables.
Patch by Phil Camp, SN Systems
Reviewers: dcoughlin, zaks.anna
Subscribers: xazax.hun, cfe-commits
Differential Revision: http://reviews.llvm.org/D19866
Patch by Phil Camp
llvm-svn: 270849
null comparison when the pointer is known to be non-null.
This catches the array to pointer decay, function to pointer decay and
address of variables. This does not catch address of function since this
has been previously used to silence a warning.
Pointer to bool conversion is under -Wbool-conversion.
Pointer to null comparison is under -Wtautological-pointer-compare, a sub-group
of -Wtautological-compare.
void foo() {
int arr[5];
int x;
// warn on these conditionals
if (foo);
if (arr);
if (&x);
if (foo == null);
if (arr == null);
if (&x == null);
if (&foo); // no warning
}
llvm-svn: 202216
This doesn't appear to be the cause of the slowdown. I'll have to try a
manual bisect to see if there's really anything there, or if it's just
the bot itself taking on additional load. Meanwhile, this change helps
with correctness.
This changes an assertion and adds a test case, then re-applies r180638,
which was reverted in r180714.
<rdar://problem/13296133> and PR15863
llvm-svn: 180864
This seems to be causing quite a slowdown on our internal analyzer bot,
and I'm not sure why. Needs further investigation.
This reverts r180638 / 9e161ea981f22ae017b6af09d660bfc3ddf16a09.
llvm-svn: 180714
Casts to bool (and _Bool) are equivalent to checks against zero,
not truncations to 1 bit or 8 bits.
This improved reasoning does cause a change in the behavior of the alpha
BoolAssignment checker. Previously, this checker complained about statements
like "bool x = y" if 'y' was known not to be 0 or 1. Now it does not, since
that conversion is well-defined. It's hard to say what the "best" behavior
here is: this conversion is safe, but might be better written as an explicit
comparison against zero.
More usefully, besides improving our model of booleans, this fixes spurious
warnings when returning the address of a local variable cast to bool.
<rdar://problem/13296133>
llvm-svn: 180638
This fixes a few cases where we'd emit path notes like this:
+---+
1| v
p = malloc(len);
^ |2
+---+
In general this should make path notes more consistent and more correct,
especially in cases where the leak happens on the false branch of an if
that jumps directly to the end of the function. There are a couple places
where the leak is reported farther away from the cause; these are usually
cases where there are several levels of nested braces before the end of
the function. This still matches our current behavior for when there /is/
a statement after all the braces, though.
llvm-svn: 168070
StackAddrLeakChecker
ObjCAtSyncChecker
UnixAPIChecker
MacOSXAPIChecker
The rest have/create implicit dependencies between checkers and need to be handled differently.
llvm-svn: 125559
by inspecting the Store bindings instead of iterating over all the global variables
in a translation unit. By looking at the store directly, we avoid cases where we cannot
directly load from the global variable, such as an array (which can result in an assertion failure)
and it also catches cases where we store stack addresses to non-scalar globals.
Also, but not iterating over all the globals in the translation unit, we maintain cache
locality, and the complexity of the checker becomes restricted to the complexity of the
analyzed function, and doesn't scale with the size of the translation unit.
This fixes PR 7383.
llvm-svn: 106184
