[analyzer][NFC] Introduce APSIntPtr, a safe wrapper of APSInt (1/4) (#120435)

One could create dangling APSInt references in various ways in the past, that were sometimes assumed to be persisted in

[analyzer][NFC] Introduce APSIntPtr, a safe wrapper of APSInt (1/4) (#120435)

One could create dangling APSInt references in various ways in the past, that were sometimes assumed to be persisted in the BasicValueFactor.

One should always use BasicValueFactory to create persistent APSInts, that could be used by ConcreteInts or SymIntExprs and similar long-living objects.
If one used a temporary or local variables for this, these would dangle.
To enforce the contract of the analyzer BasicValueFactory and the uses of APSInts, let's have a dedicated strong-type for this.

The idea is that APSIntPtr is always owned by the BasicValueFactory, and that is the only component that can construct it.

These PRs are all NFC - besides fixing dangling APSInt references.

show more ...

[analyzer] Explicitly register NoStoreFuncVisitor from alpha.unix.cst… (#108373)

…ring.UninitRead

This is a drastic simplification of #106982. If you read that patch,
this is the same thing with

[analyzer] Explicitly register NoStoreFuncVisitor from alpha.unix.cst… (#108373)

…ring.UninitRead

This is a drastic simplification of #106982. If you read that patch,
this is the same thing with all BugReporterVisitors.cpp and
SValBuilder.cpp changes removed! (since all replies came regarding
changed to those files, I felt the new PR was justified)

The patch was inspired by a pretty poor bug report on FFMpeg:


In this bug report, block is uninitialized, hence the bug report that it
should not have been passed to memcpy. The confusing part is in line 93,
where block was passed as a non-const pointer to seq_unpack_rle_block,
which was obviously meant to initialize block. As developers, we know
that clang likely didn't skip this function and found a path of
execution on which this initialization failed, but NoStoreFuncVisitor
failed to attach the usual "returning without writing to block" message.

I fixed this by instead of tracking the entire array, I tracked the
actual element which was found to be uninitialized (Remember, we
heuristically only check if the first and last-to-access element is
initialized, not the entire array). This is how the bug report looks
now, with 'seq_unpack_rle_block' having notes describing the path of
execution and lack of a value change:



Since NoStoreFuncVisitor was a TU-local class, I moved it back to
BugReporterVisitors.h, and registered it manually in CStringChecker.cpp.
This was done because we don't have a good trackRegionValue() function,
only a trackExpressionValue() function. We have an expression for the
array, but not for its first (or last-to-access) element, so I only had
a MemRegion on hand.

show more ...

[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] Use explicit call description mode in more checkers (#90974)

This commit explicitly specifies the matching mode (C library function,
any non-method function, or C++ method) for the `Call

[analyzer] Use explicit call description mode in more checkers (#90974)

This commit explicitly specifies the matching mode (C library function,
any non-method function, or C++ method) for the `CallDescription`s
constructed in various checkers.

Some code was simplified to use `CallDescriptionSet`s instead of
individual `CallDescription`s.

This change won't cause major functional changes, but isn't NFC because
it ensures that e.g. call descriptions for a non-method function won't
accidentally match a method that has the same name.

Separate commits have already performed this change in other checkers:
- easy cases: e2f1cbae45f81f3cd9a4d3c2bcf69a094eb060fa
- MallocChecker: d6d84b5d1448e4f2e24b467a0abcf42fe9d543e9
- iterator checkers: 06eedffe0d2782922e63cc25cb927f4acdaf7b30
- InvalidPtr checker: 024281d4d26344f9613b9115ea1fcbdbdba23235

... and follow-up commits will handle the remaining checkers.

My goal is to ensure that the call description mode is always explicitly
specified and eliminate (or strongly restrict) the vague "may be either
a method or a simple function" mode that's the current default.

show more ...

[analyzer] Remove untrusted buffer size warning in the TaintPropagation checker (#68607)

Before this commit the the checker alpha.security.taint.TaintPropagation always reported warnings when the si

[analyzer] Remove untrusted buffer size warning in the TaintPropagation checker (#68607)

Before this commit the the checker alpha.security.taint.TaintPropagation always reported warnings when the size argument of a memcpy-like or malloc-like function was tainted. However, this produced false positive reports in situations where the size was tainted, but correctly performed bound checks guaranteed the safety of the call.

This commit removes the rough "always warn if the size argument is tainted" heuristic; but it would be good to add a more refined "warns if the size argument is tainted and can be too large" heuristic in follow-up commits. That logic would belong to CStringChecker and MallocChecker, because those are the checkers responsible for the more detailed modeling of memcpy-like and malloc-like functions. To mark this plan, TODO comments are added in those two checkers.

There were several test cases that used these sinks to test generic properties of taint tracking; those were adapted to use different logic.

As a minor unrelated change, this commit ensures that strcat (and its wide variant, wcsncat) propagates taint from the first argument to the first argument, i.e. a tainted string remains tainted if we concatenate it with another string. This change was required because the adapted variant of multipleTaintedArgs is relying on strncat to compose a value that combines taint from two different sources.

show more ...

[analyzer] Make recognition of hardened __FOO_chk functions explicit (#86536)

In builds that use source hardening (-D_FORTIFY_SOURCE), many standard
functions are implemented as macros that expand

[analyzer] Make recognition of hardened __FOO_chk functions explicit (#86536)

In builds that use source hardening (-D_FORTIFY_SOURCE), many standard
functions are implemented as macros that expand to calls of hardened
functions that take one additional argument compared to the "usual"
variant and perform additional input validation. For example, a `memcpy`
call may expand to `__memcpy_chk()` or `__builtin___memcpy_chk()`.

Before this commit, `CallDescription`s created with the matching mode
`CDM::CLibrary` automatically matched these hardened variants (in a
addition to the "usual" function) with a fairly lenient heuristic.

Unfortunately this heuristic meant that the `CLibrary` matching mode was
only usable by checkers that were prepared to handle matches with an
unusual number of arguments.

This commit limits the recognition of the hardened functions to a
separate matching mode `CDM::CLibraryMaybeHardened` and applies this
mode for functions that have hardened variants and were previously
recognized with `CDM::CLibrary`.

This way checkers that are prepared to handle the hardened variants will
be able to detect them easily; while other checkers can simply use
`CDM::CLibrary` for matching C library functions (and they won't
encounter surprising argument counts).

The initial motivation for refactoring this area was that previously
`CDM::CLibrary` accepted calls with more arguments/parameters than the
expected number, so I wasn't able to use it for `malloc` without
accidentally matching calls to the 3-argument BSD kernel malloc.

After this commit this "may have more args/params" logic will only
activate when we're actually matching a hardened variant function (in
`CDM::CLibraryMaybeHardened` mode). The recognition of "sprintf()" and
"snprintf()" in CStringChecker was refactored, because previously it was
abusing the behavior that extra arguments are accepted even if the
matched function is not a hardened variant.

This commit also fixes the oversight that the old code would've
recognized e.g. `__wmemcpy_chk` as a hardened variant of `memcpy`.

After this commit I'm planning to create several follow-up commits that
ensure that checkers looking for C library functions use `CDM::CLibrary`
as a "sane default" matching mode.

This commit is not truly NFC (it eliminates some buggy corner cases),
but it does not intentionally modify the behavior of CSA on real-world
non-crazy code.

As a minor unrelated change I'm eliminating the argument/variable
"IsBuiltin" from the evalSprintf function family in CStringChecker,
because it was completely unused.

---------

Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>

show more ...

[analyzer] Refactor CallDescription match mode (NFC) (#83432)

The class `CallDescription` is used to define patterns that are used for
matching `CallEvent`s. For example, a
`CallDescription{{"std"

[analyzer] Refactor CallDescription match mode (NFC) (#83432)

The class `CallDescription` is used to define patterns that are used for
matching `CallEvent`s. For example, a
`CallDescription{{"std", "find_if"}, 3}`
matches a call to `std::find_if` with 3 arguments.

However, these patterns are somewhat fuzzy, so this pattern could also
match something like `std::__1::find_if` (with an additional namespace
layer), or, unfortunately, a `CallDescription` for the well-known
function `free()` can match a C++ method named `free()`:
https://github.com/llvm/llvm-project/issues/81597

To prevent this kind of ambiguity this commit introduces the enum
`CallDescription::Mode` which can limit the pattern matching to
non-method function calls (or method calls etc.). After this NFC change,
one or more follow-up commits will apply the right pattern matching
modes in the ~30 checkers that use `CallDescription`s.

Note that `CallDescription` previously had a `Flags` field which had
only two supported values:
- `CDF_None` was the default "match anything" mode,
- `CDF_MaybeBuiltin` was a "match only C library functions and accept
some inexact matches" mode.
This commit preserves `CDF_MaybeBuiltin` under the more descriptive
name `CallDescription::Mode::CLibrary` (or `CDM::CLibrary`).

Instead of this "Flags" model I'm switching to a plain enumeration
becasue I don't think that there is a natural usecase to combine the
different matching modes. (Except for the default "match anything" mode,
which is currently kept for compatibility, but will be phased out in the
follow-up commits.)

show more ...

[analyzer] Fix "sprintf" parameter modeling in CStringChecker

`CE->getCalleeDecl()` returns `VarDecl` if the callee is actually a
function pointer variable. Consequently, calling `getAsFunction()` w

[analyzer] Fix "sprintf" parameter modeling in CStringChecker

`CE->getCalleeDecl()` returns `VarDecl` if the callee is actually a
function pointer variable. Consequently, calling `getAsFunction()` will
return null.

To workaround the case, we should use the `CallEvent::parameters()`,
which will internally recover the function being called and do the right
thing.

Fixes #74269
Depends on "[analyzer][NFC] Prefer CallEvent over CallExpr in APIs"

show more ...

[analyzer][NFC] Prefer CallEvent over CallExpr in APIs

This change only uplifts existing APIs, without any semantic changes.
This is the continuation of 44820630dfa45bc47748a5abda7d4a9cb86da2c1.

Be

[analyzer][NFC] Prefer CallEvent over CallExpr in APIs

This change only uplifts existing APIs, without any semantic changes.
This is the continuation of 44820630dfa45bc47748a5abda7d4a9cb86da2c1.

Benefits of using CallEvents over CallExprs:
The callee decl is traced through function pointers if possible.
This will be important to fix #74269 in a follow-up patch.

show more ...

[clang][Analyzer][NFC] Use condition type for comparison in several checkers (#72358)

[analyzer] Compute length of string literal initializers (#66990) (#68368)

Fix issue https://github.com/llvm/llvm-project/issues/66990

[analyzer] CStringChecker buffer access checks should check the first bytes

By not checking if the first byte of the buffer is accessible,
we missed some reports in the Juliet benchmark.

(Juliet CW

[analyzer] CStringChecker buffer access checks should check the first bytes

By not checking if the first byte of the buffer is accessible,
we missed some reports in the Juliet benchmark.

(Juliet CWE-124 Buffer Underwrite: memcpy, memmove)
https://discourse.llvm.org/t/patches-inspired-by-the-juliet-benchmark/73106

Depends on D159108

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

show more ...

[analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy

By not checking if the first byte of the destination of strcpy and
strncpy is writable, we missed some rep

[analyzer] CStringChecker should check the first byte of the destination of strcpy, strncpy

By not checking if the first byte of the destination of strcpy and
strncpy is writable, we missed some reports in the Juliet benchmark.

(Juliet CWE-124 Buffer Underwrite: strcpy, strncpy)
https://discourse.llvm.org/t/patches-inspired-by-the-juliet-benchmark/73106

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

show more ...

[analyzer][NFC] Remove useless class BuiltinBug

...because it provides no useful functionality compared to its base
class `BugType`.

A long time ago there were substantial differences between `BugT

[analyzer][NFC] Remove useless class BuiltinBug

...because it provides no useful functionality compared to its base
class `BugType`.

A long time ago there were substantial differences between `BugType` and
`BuiltinBug`, but they were eliminated by commit 1bd58233 in 2009 (!).
Since then the only functionality provided by `BuiltinBug` was that it
specified `categories::LogicError` as the bug category and it stored an
extra data member `desc`.

This commit sets `categories::LogicError` as the default value of the
third argument (bug category) in the constructors of BugType and
replaces use of the `desc` field with simpler logic.

Note that `BugType` has a data member `Description` and a non-virtual
method `BugType::getDescription()` which queries it; these are distinct
from the member `desc` of `BuiltinBug` and the identically named method
`BuiltinBug::getDescription()` which queries it. This confusing name
collision was a major motivation for the elimination of `BuiltinBug`.

As this commit touches many files, I avoided functional changes and left
behind FIXME notes to mark minor issues that should be fixed later.

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

show more ...

[clang] Satisfy clang v12

Older versions of clang (for example, v12) throw an error when compiling
CStringChecker.cpp that the initializers for `SourceArgExpr`,
`DestinationArgExpr`, and `SizeArgExp

[clang] Satisfy clang v12

Older versions of clang (for example, v12) throw an error when compiling
CStringChecker.cpp that the initializers for `SourceArgExpr`,
`DestinationArgExpr`, and `SizeArgExpr` are missing braces around
initialization of subobject. Newer clang versions don't throw this
error. This patch adds the initialization braces to satisfy clang.

Reviewed By: steakhal

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

show more ...

[NFC] Initialize pointer fields and remove needless null check.

Reviewed here: https://reviews.llvm.org/D153589

[analyzer][NFC] Simplify CStringChecker strong types

[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][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

Fixing GitHub issue: https://github.com/llvm/llvm-project/issues/55019
F

[analyzer][CStringChecker] Adjust the invalidation operation on the super region of the destination buffer during string copy

Fixing GitHub issue: https://github.com/llvm/llvm-project/issues/55019
Following the previous fix https://reviews.llvm.org/D12571 on issue https://github.com/llvm/llvm-project/issues/23328

The two issues report false memory leaks after calling string-copy APIs with a buffer field in an object as the destination.
The buffer invalidation incorrectly drops the assignment to a heap memory block when no overflow problems happen.
And the pointer of the dropped assignment is declared in the same object of the destination buffer.

The previous fix only considers the `memcpy` functions whose copy length is available from arguments.
In this issue, the copy length is inferable from the buffer declaration and string literals being copied.
Therefore, I have adjusted the previous fix to reuse the copy length computed before.

Besides, for APIs that never overflow (strsep) or we never know whether they can overflow (std::copy),
new invalidation operations have been introduced to inform CStringChecker::InvalidateBuffer whether or not to
invalidate the super region that encompasses the destination buffer.

Reviewed By: steakhal

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

show more ...

Implement BufferOverlap check for sprint/snprintf

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

[ADT] Make StringRef::compare like std::string_view::compare

string_view has a slightly weaker contract, which only specifies whether
the value is bigger or smaller than 0. Adapt users accordingly a

[ADT] Make StringRef::compare like std::string_view::compare

string_view has a slightly weaker contract, which only specifies whether
the value is bigger or smaller than 0. Adapt users accordingly and just
forward to the standard function (that also compiles down to memcmp)

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

[clang] Use a StringRef instead of a raw char pointer to store builtin and call information

This avoids recomputing string length that is already known at compile time.

It has a slight impact on pr

[clang] Use a StringRef instead of a raw char pointer to store builtin and call information

This avoids recomputing string length that is already known at compile time.

It has a slight impact on preprocessing / compile time, see

https://llvm-compile-time-tracker.com/compare.php?from=3f36d2d579d8b0e8824d9dd99bfa79f456858f88&to=e49640c507ddc6615b5e503144301c8e41f8f434&stat=instructions:u

This a recommit of e953ae5bbc313fd0cc980ce021d487e5b5199ea4 and the subsequent fixes caa713559bd38f337d7d35de35686775e8fb5175 and 06b90e2e9c991e211fecc97948e533320a825470.

The above patchset caused some version of GCC to take eons to compile clang/lib/Basic/Targets/AArch64.cpp, as spotted in aa171833ab0017d9732e82b8682c9848ab25ff9e.
The fix is to make BuiltinInfo tables a compilation unit static variable, instead of a private static variable.

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

show more ...

Revert "[clang] Use a StringRef instead of a raw char pointer to store builtin and call information"
Revert "Fix lldb option handling since e953ae5bbc313fd0cc980ce021d487e5b5199ea4 (part 2)"
Revert "

Revert "[clang] Use a StringRef instead of a raw char pointer to store builtin and call information"
Revert "Fix lldb option handling since e953ae5bbc313fd0cc980ce021d487e5b5199ea4 (part 2)"
Revert "Fix lldb option handling since e953ae5bbc313fd0cc980ce021d487e5b5199ea4"

GCC build hangs on this bot https://lab.llvm.org/buildbot/#/builders/37/builds/19104
compiling CMakeFiles/obj.clangBasic.dir/Targets/AArch64.cpp.d

The bot uses GNU 11.3.0, but I can reproduce locally with gcc (Debian 12.2.0-3) 12.2.0.

This reverts commit caa713559bd38f337d7d35de35686775e8fb5175.
This reverts commit 06b90e2e9c991e211fecc97948e533320a825470.
This reverts commit e953ae5bbc313fd0cc980ce021d487e5b5199ea4.

show more ...