Skip to content

Commit

Permalink
[analyzer] Use explicit call description mode in more checkers (llvm#…
Browse files Browse the repository at this point in the history
…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: e2f1cba
- MallocChecker: d6d84b5
- iterator checkers: 06eedff
- InvalidPtr checker: 024281d

... 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.
  • Loading branch information
NagyDonat authored May 7, 2024
1 parent dcc7ef3 commit 6d64f8e
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 67 deletions.
40 changes: 20 additions & 20 deletions clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,27 +148,28 @@ using MutexDescriptor =
class BlockInCriticalSectionChecker : public Checker<check::PostCall> {
private:
const std::array<MutexDescriptor, 8> MutexDescriptors{
MemberMutexDescriptor(
CallDescription(/*QualifiedName=*/{"std", "mutex", "lock"},
/*RequiredArgs=*/0),
CallDescription({"std", "mutex", "unlock"}, 0)),
FirstArgMutexDescriptor(CallDescription({"pthread_mutex_lock"}, 1),
CallDescription({"pthread_mutex_unlock"}, 1)),
FirstArgMutexDescriptor(CallDescription({"mtx_lock"}, 1),
CallDescription({"mtx_unlock"}, 1)),
FirstArgMutexDescriptor(CallDescription({"pthread_mutex_trylock"}, 1),
CallDescription({"pthread_mutex_unlock"}, 1)),
FirstArgMutexDescriptor(CallDescription({"mtx_trylock"}, 1),
CallDescription({"mtx_unlock"}, 1)),
FirstArgMutexDescriptor(CallDescription({"mtx_timedlock"}, 1),
CallDescription({"mtx_unlock"}, 1)),
MemberMutexDescriptor({/*MatchAs=*/CDM::CXXMethod,
/*QualifiedName=*/{"std", "mutex", "lock"},
/*RequiredArgs=*/0},
{CDM::CXXMethod, {"std", "mutex", "unlock"}, 0}),
FirstArgMutexDescriptor({CDM::CLibrary, {"pthread_mutex_lock"}, 1},
{CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
FirstArgMutexDescriptor({CDM::CLibrary, {"mtx_lock"}, 1},
{CDM::CLibrary, {"mtx_unlock"}, 1}),
FirstArgMutexDescriptor({CDM::CLibrary, {"pthread_mutex_trylock"}, 1},
{CDM::CLibrary, {"pthread_mutex_unlock"}, 1}),
FirstArgMutexDescriptor({CDM::CLibrary, {"mtx_trylock"}, 1},
{CDM::CLibrary, {"mtx_unlock"}, 1}),
FirstArgMutexDescriptor({CDM::CLibrary, {"mtx_timedlock"}, 1},
{CDM::CLibrary, {"mtx_unlock"}, 1}),
RAIIMutexDescriptor("lock_guard"),
RAIIMutexDescriptor("unique_lock")};

const std::array<CallDescription, 5> BlockingFunctions{
ArrayRef{StringRef{"sleep"}}, ArrayRef{StringRef{"getc"}},
ArrayRef{StringRef{"fgets"}}, ArrayRef{StringRef{"read"}},
ArrayRef{StringRef{"recv"}}};
const CallDescriptionSet BlockingFunctions{{CDM::CLibrary, {"sleep"}},
{CDM::CLibrary, {"getc"}},
{CDM::CLibrary, {"fgets"}},
{CDM::CLibrary, {"read"}},
{CDM::CLibrary, {"recv"}}};

const BugType BlockInCritSectionBugType{
this, "Call to blocking function in critical section", "Blocking Error"};
Expand Down Expand Up @@ -291,8 +292,7 @@ void BlockInCriticalSectionChecker::handleUnlock(

bool BlockInCriticalSectionChecker::isBlockingInCritSection(
const CallEvent &Call, CheckerContext &C) const {
return llvm::any_of(BlockingFunctions,
[&Call](auto &&Fn) { return Fn.matches(Call); }) &&
return BlockingFunctions.contains(Call) &&
!C.getState()->get<ActiveCritSections>().isEmpty();
}

Expand Down
4 changes: 2 additions & 2 deletions clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ class CStringChecker : public Checker< eval::Call,
};

// These require a bit of special handling.
CallDescription StdCopy{{"std", "copy"}, 3},
StdCopyBackward{{"std", "copy_backward"}, 3};
CallDescription StdCopy{CDM::SimpleFunc, {"std", "copy"}, 3},
StdCopyBackward{CDM::SimpleFunc, {"std", "copy_backward"}, 3};

FnCheck identifyCall(const CallEvent &Call, CheckerContext &C) const;
void evalMemcpy(CheckerContext &C, const CallEvent &Call, CharKind CK) const;
Expand Down
58 changes: 25 additions & 33 deletions clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,28 @@ namespace {
class InnerPointerChecker
: public Checker<check::DeadSymbols, check::PostCall> {

CallDescription AppendFn, AssignFn, AddressofFn, AddressofFn_, ClearFn,
CStrFn, DataFn, DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn,
ReplaceFn, ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
CallDescriptionSet InvalidatingMemberFunctions{
CallDescription(CDM::CXXMethod, {"std", "basic_string", "append"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "assign"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "clear"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "erase"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "insert"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "pop_back"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "push_back"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "replace"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "reserve"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "resize"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "shrink_to_fit"}),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "swap"})};

CallDescriptionSet AddressofFunctions{
CallDescription(CDM::SimpleFunc, {"std", "addressof"}),
CallDescription(CDM::SimpleFunc, {"std", "__addressof"})};

CallDescriptionSet InnerPointerAccessFunctions{
CallDescription(CDM::CXXMethod, {"std", "basic_string", "c_str"}),
CallDescription(CDM::SimpleFunc, {"std", "data"}, 1),
CallDescription(CDM::CXXMethod, {"std", "basic_string", "data"})};

public:
class InnerPointerBRVisitor : public BugReporterVisitor {
Expand Down Expand Up @@ -71,30 +90,10 @@ class InnerPointerChecker
}
};

InnerPointerChecker()
: AppendFn({"std", "basic_string", "append"}),
AssignFn({"std", "basic_string", "assign"}),
AddressofFn({"std", "addressof"}), AddressofFn_({"std", "__addressof"}),
ClearFn({"std", "basic_string", "clear"}),
CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}, 1),
DataMemberFn({"std", "basic_string", "data"}),
EraseFn({"std", "basic_string", "erase"}),
InsertFn({"std", "basic_string", "insert"}),
PopBackFn({"std", "basic_string", "pop_back"}),
PushBackFn({"std", "basic_string", "push_back"}),
ReplaceFn({"std", "basic_string", "replace"}),
ReserveFn({"std", "basic_string", "reserve"}),
ResizeFn({"std", "basic_string", "resize"}),
ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
SwapFn({"std", "basic_string", "swap"}) {}

/// Check whether the called member function potentially invalidates
/// pointers referring to the container object's inner buffer.
bool isInvalidatingMemberFunction(const CallEvent &Call) const;

/// Check whether the called function returns a raw inner pointer.
bool isInnerPointerAccessFunction(const CallEvent &Call) const;

/// Mark pointer symbols associated with the given memory region released
/// in the program state.
void markPtrSymbolsReleased(const CallEvent &Call, ProgramStateRef State,
Expand Down Expand Up @@ -127,14 +126,7 @@ bool InnerPointerChecker::isInvalidatingMemberFunction(
return false;
}
return isa<CXXDestructorCall>(Call) ||
matchesAny(Call, AppendFn, AssignFn, ClearFn, EraseFn, InsertFn,
PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
ShrinkToFitFn, SwapFn);
}

bool InnerPointerChecker::isInnerPointerAccessFunction(
const CallEvent &Call) const {
return matchesAny(Call, CStrFn, DataFn, DataMemberFn);
InvalidatingMemberFunctions.contains(Call);
}

void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent &Call,
Expand Down Expand Up @@ -181,7 +173,7 @@ void InnerPointerChecker::checkFunctionArguments(const CallEvent &Call,

// std::addressof functions accepts a non-const reference as an argument,
// but doesn't modify it.
if (matchesAny(Call, AddressofFn, AddressofFn_))
if (AddressofFunctions.contains(Call))
continue;

markPtrSymbolsReleased(Call, State, ArgRegion, C);
Expand Down Expand Up @@ -221,7 +213,7 @@ void InnerPointerChecker::checkPostCall(const CallEvent &Call,
}
}

if (isInnerPointerAccessFunction(Call)) {
if (InnerPointerAccessFunctions.contains(Call)) {

if (isa<SimpleFunctionCall>(Call)) {
// NOTE: As of now, we only have one free access function: std::data.
Expand Down
18 changes: 9 additions & 9 deletions clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ class SmartPtrModeling
using SmartPtrMethodHandlerFn =
void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
{{{"reset"}}, &SmartPtrModeling::handleReset},
{{{"release"}}, &SmartPtrModeling::handleRelease},
{{{"swap"}, 1}, &SmartPtrModeling::handleSwapMethod},
{{{"get"}}, &SmartPtrModeling::handleGet}};
const CallDescription StdSwapCall{{"std", "swap"}, 2};
const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
const CallDescription StdMakeUniqueForOverwriteCall{
{"std", "make_unique_for_overwrite"}};
{{CDM::CXXMethod, {"reset"}}, &SmartPtrModeling::handleReset},
{{CDM::CXXMethod, {"release"}}, &SmartPtrModeling::handleRelease},
{{CDM::CXXMethod, {"swap"}, 1}, &SmartPtrModeling::handleSwapMethod},
{{CDM::CXXMethod, {"get"}}, &SmartPtrModeling::handleGet}};
const CallDescription StdSwapCall{CDM::SimpleFunc, {"std", "swap"}, 2};
const CallDescriptionSet MakeUniqueVariants{
{CDM::SimpleFunc, {"std", "make_unique"}},
{CDM::SimpleFunc, {"std", "make_unique_for_overwrite"}}};
};
} // end of anonymous namespace

Expand Down Expand Up @@ -296,7 +296,7 @@ bool SmartPtrModeling::evalCall(const CallEvent &Call,
return handleSwap(State, Call.getArgSVal(0), Call.getArgSVal(1), C);
}

if (matchesAny(Call, StdMakeUniqueCall, StdMakeUniqueForOverwriteCall)) {
if (MakeUniqueVariants.contains(Call)) {
if (!ModelSmartPtrDereference)
return false;

Expand Down
8 changes: 5 additions & 3 deletions clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,17 +388,19 @@ class StreamChecker : public Checker<check::PreCall, eval::Call,
};

CallDescriptionMap<FnDescription> FnTestDescriptions = {
{{{"StreamTesterChecker_make_feof_stream"}, 1},
{{CDM::SimpleFunc, {"StreamTesterChecker_make_feof_stream"}, 1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4, ErrorFEof,
false),
0}},
{{{"StreamTesterChecker_make_ferror_stream"}, 1},
{{CDM::SimpleFunc, {"StreamTesterChecker_make_ferror_stream"}, 1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
ErrorFError, false),
0}},
{{{"StreamTesterChecker_make_ferror_indeterminate_stream"}, 1},
{{CDM::SimpleFunc,
{"StreamTesterChecker_make_ferror_indeterminate_stream"},
1},
{nullptr,
std::bind(&StreamChecker::evalSetFeofFerror, _1, _2, _3, _4,
ErrorFError, true),
Expand Down

0 comments on commit 6d64f8e

Please sign in to comment.