[analyzer][NFC] Factor out SymbolManager::get<*> (#121781)

Replace the family of `SymbolManager::get*Symbol(...)` member functions
with a single generic `SymbolManager::get<*>` member function.

[analyzer][NFC] Migrate nonloc::ConcreteInt to use APSIntPtr (2/4) (#120436)

[StaticAnalyzer] Migrate away from PointerUnion::{is,get} (NFC) (#118421)

Note that PointerUnion::{is,get} have been soft deprecated in
PointerUnion.h:

// FIXME: Replace the uses of is(), get(

[StaticAnalyzer] Migrate away from PointerUnion::{is,get} (NFC) (#118421)

Note that PointerUnion::{is,get} have been soft deprecated in
PointerUnion.h:

// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>

I'm not touching PointerUnion::dyn_cast for now because it's a bit
complicated; we could blindly migrate it to dyn_cast_if_present, but
we should probably use dyn_cast when the operand is known to be
non-null.

show more ...

[analyzer] Never create Regions wrapping reference TypedValueRegions (NFCI) (#118096)

Like in the test case:
```c++
struct String {
String(const String &) {}
};

struct MatchComponent {
unsigned

[analyzer] Never create Regions wrapping reference TypedValueRegions (NFCI) (#118096)

Like in the test case:
```c++
struct String {
String(const String &) {}
};

struct MatchComponent {
unsigned numbers[2];
String prerelease;
MatchComponent(MatchComponent const &) = default;
};

MatchComponent get();
void consume(MatchComponent const &);

MatchComponent parseMatchComponent() {
MatchComponent component = get();
component.numbers[0] = 10;
component.numbers[1] = 20;
return component; // We should have no stack addr escape warning here.
}

void top() {
consume(parseMatchComponent());
}
```

When calling `consume(parseMatchComponent())` the
`parseMatchComponent()` would return a copy of a temporary of
`component`. That copy would invoke the
`MatchComponent::MatchComponent(const MatchComponent &)` ctor.

That ctor would have a (reference typed) ParamVarRegion, holding the
location (lvalue) of the object we are about to copy (&component). So
far so good, but just before evaluating the binding operation for
initializing the `numbers` field of the temporary, we evaluate the
ArrayInitLoopExpr representing the by-value elementwise copy of the
array `component.numbers`. This is represented by a LazyCompoundVal,
because we (usually) don't just copy large arrays and bind many
individual direct bindings. Rather, we take a snapshot by using a LCV.

However, notice that the LCV representing this copy would look like
this:
lazyCompoundVal{ParamVarRegion{"reference param of cctor"}.numbers}

Notice that it refers to the numbers field of a reference. It would be
much better to desugar the reference to the actual object, thus it
should be: `lazyCompoundVal{component.numbers}`

Actually, when binding the result of the ArrayInitLoopExpr to the
`temp_object.numbers` in the compiler-generated member initializer of
the cctor, we should have two individual direct bindings because this is
a "small array":
```
binding &Element{temp_object.numbers, 0} <- loadFrom(&Element{component.numbers, 0})
binding &Element{temp_object.numbers, 1} <- loadFrom(&Element{component.numbers, 1})
```
Where `loadFrom(...)` would be:
```
loadFrom(&Element{component.numbers, 0}): 10 U32b
loadFrom(&Element{component.numbers, 1}): 20 U32b
```
So the store should look like this, after PostInitializer of
`temp_object.numbers`:
```
temp_object at offset 0: 10 U32b
temp_object at offset 32: 20 U32b
```

The lesson is that it's okay to have TypedValueRegions of references as
long as we don't form subregions of those. If we ever want to refer to a
subregion of a "reference" we actually meant to "desugar" the reference
and slice a subregion of the pointee of the reference instead.

Once this canonicalization is in place, we can also drop the special
handling of references in `ProcessInitializer`, because now reference
TypedValueRegions are eagerly desugared into their referee region when
forming a subregion of it.

There should be no practical differences, but there are of course bugs
that this patch may surface.

show more ...

[analyzer] Improve FieldRegion descriptive name (#112313)

The current implementation of MemRegion::getDescriptiveName fails for
FieldRegions whose SuperRegion is an ElementRegion. As outlined below:

[analyzer] Improve FieldRegion descriptive name (#112313)

The current implementation of MemRegion::getDescriptiveName fails for
FieldRegions whose SuperRegion is an ElementRegion. As outlined below:
```Cpp
struct val_struct { int val; };
extern struct val_struct val_struct_array[3];

void func(){
// FieldRegion with ElementRegion as SuperRegion.
val_struct_array[0].val;
}
```

For this special case, the expression cannot be pretty printed and must
therefore be obtained separately.

show more ...

[clang] Use {} instead of std::nullopt to initialize empty ArrayRef (#109399)

Follow up to #109133.

[analyzer] Check the correct first and last elements in cstring.UninitializedRead (#95408)

I intend to fix this checker up so that we can move it out of alpha. I
made a bunch of analyses, and found

[analyzer] Check the correct first and last elements in cstring.UninitializedRead (#95408)

I intend to fix this checker up so that we can move it out of alpha. I
made a bunch of analyses, and found many similar false positives:

```c++
int t[] = {1,2,3};
memcpy(dst, t, sizeof(t) / sizeof(t[0])); // warn
```

The problem here is the way CStringChecker checks whether the
destination and source buffers are initialized: heuristically, it only
checks the first and last element. This is fine, however, it retrieves
these elements as characters, even if the underlaying object is not a
character array. Reading the last byte of an integer is undefined, so
the checker emits a bug here.

A quick search tells you the rationale: "Both objects are reinterpreted
as arrays of unsigned char.". But the static analyzer right now can't
check byte-by-byte if a memory region is _initialized_, it can only
check if its a well-defined character or not.

In this patch, I pry the original array out of the arguments to memcpy
(and similar functions), and retrieve the actual first and last elements
according to the array's actual element type.

Currently, my improvements reduced the number of reports to 29 on these
projects: memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres


https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acstring_uninit_upper_bound_patched&newcheck=%2acstring_uninit_upper_bounds_patched&diff-type=New&checker-name=alpha.unix.cstring.UninitializedRead&items-per-page=100

Before my patch, there were 87.


https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acstring_uninit_baseline&newcheck=%2acstring_uninit_upper_bounds_patched&diff-type=New&checker-name=alpha.unix.cstring.UninitializedRead&items-per-page=100

show more ...

[analyzer] Refine invalidation caused by `fread` (#93408)

This change enables more accurate modeling of the write effects of
`fread`. In particular, instead of invalidating the whole buffer, in a

[analyzer] Refine invalidation caused by `fread` (#93408)

This change enables more accurate modeling of the write effects of
`fread`. In particular, instead of invalidating the whole buffer, in a
best-effort basis, we would try to invalidate the actually accesses
elements of the buffer. This preserves the previous value of the buffer
of the unaffected slots. As a result, diagnose more uninitialized buffer
uses for example.

Currently, this refined invalidation only triggers for `fread` if and
only if the `count` parameter and the buffer pointer's index component
are concrete or perfectly-constrained symbols.
Additionally, if the `fread` would read more than 64 elements, the whole
buffer is invalidated as before. This is to have safeguards against
performance issues.

Refer to the comments of the assertions in the following example to see
the changes in the diagnostics:

```c++
void demo() {
FILE *fp = fopen("/home/test", "rb+");
if (!fp) return;
int buffer[10]; // uninitialized
int read_items = fread(buffer+1, sizeof(int), 5, fp);
if (5 == read_items) {
int v1 = buffer[1]; // Unknown value but not garbage.
clang_analyzer_isTainted(v1); // expected-warning {{YES}} <-- Would be "NO" without this patch.
clang_analyzer_dump(v1); // expected-warning {{conj_}} <-- Not a "derived" symbol, so it's directly invalidated now.
int v0 = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}} <-- Had no report here before.
(void)(v1 + v0);
} else {
// If 'fread' had an error.
int v0 = buffer[0]; // expected-warning {{Assigned value is garbage or undefined}} <-- Had no report here before.
(void)v0;
}
fclose(fp);
}
```

CPP-3247, CPP-3802

Co-authored by Marco Borgeaud (marco-antognini-sonarsource)

show more ...

[NFC] Refactor ConstantArrayType size storage (#85716)

In PR #79382, I need to add a new type that derives from
ConstantArrayType. This means that ConstantArrayType can no longer use
`llvm::Traili

[NFC] Refactor ConstantArrayType size storage (#85716)

In PR #79382, I need to add a new type that derives from
ConstantArrayType. This means that ConstantArrayType can no longer use
`llvm::TrailingObjects` to store the trailing optional Expr*.

This change refactors ConstantArrayType to store a 60-bit integer and
4-bits for the integer size in bytes. This replaces the APInt field
previously in the type but preserves enough information to recreate it
where needed.

To reduce the number of places where the APInt is re-constructed I've
also added some helper methods to the ConstantArrayType to allow some
common use cases that operate on either the stored small integer or the
APInt as appropriate.

Resolves #85124.

show more ...

Adapted MemRegion::getDescriptiveName to handle ElementRegions (#85104)

Fixes https://github.com/llvm/llvm-project/issues/84463

Changes:
- Adapted MemRegion::getDescriptiveName
- Added unittest

Adapted MemRegion::getDescriptiveName to handle ElementRegions (#85104)

Fixes https://github.com/llvm/llvm-project/issues/84463

Changes:
- Adapted MemRegion::getDescriptiveName
- Added unittest to check name for a given clang::ento::ElementRegion
- Some format changes due to clang-format

---------

Co-authored-by: Andreas Steinhausen <andreas.steinhausen@concenrio.io>
Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>

show more ...

[analyzer] NonParamVarRegion should prefer definition over canonical decl

When we construct a `NonParamVarRegion`, we canonicalize the decl to
always use the same entity for consistency.
At the mome

[analyzer] NonParamVarRegion should prefer definition over canonical decl

When we construct a `NonParamVarRegion`, we canonicalize the decl to
always use the same entity for consistency.
At the moment that is the canonical decl - which is the first decl in
the redecl chain.

However, this can cause problems with tentative declarations and extern
declarations if we declare an array with unknown bounds.

Consider this C example: https://godbolt.org/z/Kdvr11EqY
```lang=C
typedef typeof(sizeof(int)) size_t;
size_t clang_analyzer_getExtent(const void *p);
void clang_analyzer_dump(size_t n);

extern const unsigned char extern_redecl[];
const unsigned char extern_redecl[] = { 1,2,3,4 };
const unsigned char tentative_redecl[];
const unsigned char tentative_redecl[] = { 1,2,3,4 };

const unsigned char direct_decl[] = { 1,2,3,4 };

void test_redeclaration_extent(void) {
clang_analyzer_dump(clang_analyzer_getExtent(direct_decl)); // 4
clang_analyzer_dump(clang_analyzer_getExtent(extern_redecl)); // should be 4 instead of Unknown
clang_analyzer_dump(clang_analyzer_getExtent(tentative_redecl)); // should be 4 instead of Unknown
}
```

The `getType()` of the canonical decls for the forward declared globals,
will return `IncompleteArrayType`, unlike the
`getDefinition()->getType()`, which would have returned
`ConstantArrayType` of 4 elements.

This makes the `MemRegionManager::getStaticSize()` return `Unknown` as
the extent for the array variables, leading to FNs.

To resolve this, I think we should prefer the definition decl (if
present) over the canonical decl when constructing `NonParamVarRegion`s.

FYI The canonicalization of the decl was introduced by D57619 in 2019.

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

show more ...

[analyzer] Remove deprecated analyzer-config options

The `consider-single-element-arrays-as-flexible-array-members` analyzer
option was deprecated in clang-16, and now removed from clang-17 as
promi

[analyzer] Remove deprecated analyzer-config options

The `consider-single-element-arrays-as-flexible-array-members` analyzer
option was deprecated in clang-16, and now removed from clang-17 as
promised in
https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#static-analyzer

This shouldn't change observable behavior.

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

show more ...

[analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

I'm involved with the Static Analyzer for the most part.
I think we should embrace newer language standard features and gradu

[analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

I'm involved with the Static Analyzer for the most part.
I think we should embrace newer language standard features and gradually
move forward.

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

show more ...

[analyzer] Differentiate lifetime extended temporaries

This patch introduces a new `CXXLifetimeExtendedObjectRegion` as a representation
of the memory for the temporary object that is lifetime exten

[analyzer] Differentiate lifetime extended temporaries

This patch introduces a new `CXXLifetimeExtendedObjectRegion` as a representation
of the memory for the temporary object that is lifetime extended by the reference
to which they are bound.

This separation provides an ability to detect the use of dangling pointers
(either binding or dereference) in a robust manner.
For example, the `ref` is conditionally dangling in the following example:
```
template<typename T>
T const& select(bool cond, T const& t, T const& u) { return cond ? t : u; }

int const& le = Composite{}.x;
auto&& ref = select(cond, le, 10);
```
Before the change, regardless of the value of `cond`, the `select()` call would
have returned a `temp_object` region.
With the proposed change we would produce a (non-dangling) `lifetime_extended_object`
region with lifetime bound to `le` or a `temp_object` region for the dangling case.

We believe that such separation is desired, as such lifetime extended temporaries
are closer to the variables. For example, they may have a static storage duration
(this patch removes a static temporary region, which was an abomination).
We also think that alternative approaches are not viable.

While for some cases it may be possible to determine if the region is lifetime
extended by searching the parents of the initializer expr, this quickly becomes
complex in the presence of the conditions operators like this one:
```
Composite cc;
// Ternary produces prvalue 'int' which is extended, as branches differ in value category
auto&& x = cond ? Composite{}.x : cc.x;

// Ternary produces xvalue, and extends the Composite object
auto&& y = cond ? Composite{}.x : std::move(cc).x;
```

Finally, the lifetime of the `CXXLifetimeExtendedObjectRegion` is tied to the lifetime of
the corresponding variables, however, the "liveness" (or reachability) of the extending
variable does not imply the reachability of all symbols in the region.
In conclusion `CXXLifetimeExtendedObjectRegion`, in contrast to `VarRegions`, does not
need any special handling in `SymReaper`.

RFC: https://discourse.llvm.org/t/rfc-detecting-uses-of-dangling-references/70731

Reviewed By: xazax.hun

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

show more ...

[StaticAnalyzer] Remove unused function hasGlobalsOrParametersStorage

The last use was removed by:

commit e2e37b9afc0a0a66a1594377a88221e115d95348
Author: Ted Kremenek <kremenek@apple.com>
Da

[StaticAnalyzer] Remove unused function hasGlobalsOrParametersStorage

The last use was removed by:

commit e2e37b9afc0a0a66a1594377a88221e115d95348
Author: Ted Kremenek <kremenek@apple.com>
Date: Thu Jul 28 23:08:02 2011 +0000

show more ...

[clang][analyzer][NFC] Use the operator new directly with the `BumpPtrAllocator`

Reviewed By: xazax.hun

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

[clang][analyzer][NFC] Replace dyn_cast with cast in MemRegion::getMemorySpace

MemRegion::getMemorySpace() is annotated with
LLVM_ATTRIBUTE_RETURNS_NONNULL (which triggers instant UB if a null
point

[clang][analyzer][NFC] Replace dyn_cast with cast in MemRegion::getMemorySpace

MemRegion::getMemorySpace() is annotated with
LLVM_ATTRIBUTE_RETURNS_NONNULL (which triggers instant UB if a null
pointer is returned), and callers indeed don't check the return value
for null. Thus, even though llvm::dyn_cast is called, it can never
return null in this context. Therefore, we can safely call llvm::cast.

Reviewed By: steakhal

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

show more ...

[clang][analyzer][NFC] Move dyn_cast's into if statements for readability

Reviewed By: steakhal

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

[clang] Remove remaining uses of llvm::Optional (NFC)

This patch removes several "using" declarations and #include
"llvm/ADT/Optional.h".

This is part of an effort to migrate from llvm::Optional to

[clang] Remove remaining uses of llvm::Optional (NFC)

This patch removes several "using" declarations and #include
"llvm/ADT/Optional.h".

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

show more ...

[clang] Use std::optional instead of llvm::Optional (NFC)

This patch replaces (llvm::|)Optional< with std::optional<. I'll post
a separate patch to remove #include "llvm/ADT/Optional.h".

This is p

[clang] Use std::optional instead of llvm::Optional (NFC)

This patch replaces (llvm::|)Optional< with std::optional<. I'll post
a separate patch to remove #include "llvm/ADT/Optional.h".

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

show more ...

[clang] Add #include <optional> (NFC)

This patch adds #include <optional> to those files containing
llvm::Optional<...> or Optional<...>.

I'll post a separate patch to actually replace llvm::Option

[clang] Add #include <optional> (NFC)

This patch adds #include <optional> to those files containing
llvm::Optional<...> or Optional<...>.

I'll post a separate patch to actually replace llvm::Optional with
std::optional.

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

show more ...

[StaticAnalyzer] Use std::nullopt instead of None (NFC)

This patch mechanically replaces None with std::nullopt where the
compiler would warn if None were deprecated. The intent is to reduce
the am

[StaticAnalyzer] Use std::nullopt instead of None (NFC)

This patch mechanically replaces None with std::nullopt where the
compiler would warn if None were deprecated. The intent is to reduce
the amount of manual work required in migrating from Optional to
std::optional.

This is part of an effort to migrate from llvm::Optional to
std::optional:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

show more ...

[analyzer] Deprecate FAM analyzer-config, recommend -fstrict-flex-arrays instead

By default, clang assumes that all trailing array objects could be a
FAM. So, an array of undefined size, size 0, siz

[analyzer] Deprecate FAM analyzer-config, recommend -fstrict-flex-arrays instead

By default, clang assumes that all trailing array objects could be a
FAM. So, an array of undefined size, size 0, size 1, or even size 42 is
considered as FAMs for optimizations at least.

One needs to override the default behavior by supplying the
`-fstrict-flex-arrays=<N>` flag, with `N > 0` value to reduce the set of
FAM candidates. Value `3` is the most restrictive and `0` is the most
permissive on this scale.

0: all trailing arrays are FAMs
1: only incomplete, zero and one-element arrays are FAMs
2: only incomplete, zero-element arrays are FAMs
3: only incomplete arrays are FAMs

If the user is happy with consdering single-element arrays as FAMs, they
just need to remove the
`consider-single-element-arrays-as-flexible-array-members` from the
command line.
Otherwise, if they don't want to recognize such cases as FAMs, they
should specify `-fstrict-flex-arrays` anyway, which will be picked up by
CSA.

Any use of the deprecated analyzer-config value will trigger a warning
explaining what to use instead.
The `-analyzer-config-help` is updated accordingly.

Depends on D138657

Reviewed By: xazax.hun

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

show more ...

[clang] Implement -fstrict-flex-arrays=3

The -fstrict-flex-arrays=3 is the most restrictive type of flex arrays.
No number, including 0, is allowed in the FAM. In the cases where a "0"
is used, the

[clang] Implement -fstrict-flex-arrays=3

The -fstrict-flex-arrays=3 is the most restrictive type of flex arrays.
No number, including 0, is allowed in the FAM. In the cases where a "0"
is used, the resulting size is the same as if a zero-sized object were
substituted.

This is needed for proper _FORTIFY_SOURCE coverage in the Linux kernel,
among other reasons. So while the only reason for specifying a
zero-length array at the end of a structure is for specify a FAM,
treating it as such will cause _FORTIFY_SOURCE not to work correctly;
__builtin_object_size will report -1 instead of 0 for a destination
buffer size to keep any kernel internals from using the deprecated
members as fake FAMs.

For example:

struct broken {
int foo;
int fake_fam[0];
struct something oops;
};

There have been bugs where the above struct was created because "oops"
was added after "fake_fam" by someone not realizing. Under
__FORTIFY_SOURCE, doing:

memcpy(p->fake_fam, src, len);

raises no warnings when __builtin_object_size(p->fake_fam, 1) returns -1
and may stomp on "oops."

Omitting a warning when using the (invalid) zero-length array is how GCC
treats -fstrict-flex-arrays=3. A warning in that situation is likely an
irritant, because requesting this option level is explicitly requesting
this behavior.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836

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

show more ...

[clang][NFC] Use enum for -fstrict-flex-arrays

Use enums for the strict flex arrays flag so that it's more readable.

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