Skip to content

[clang][analyzer] Model allocation behavior or getdelim/geline #83138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H

#include "ProgramState_Fwd.h"
#include "SVals.h"

#include "clang/AST/OperationKinds.h"
#include "clang/AST/Stmt.h"
#include "clang/Basic/OperatorKinds.h"
Expand Down Expand Up @@ -110,6 +113,9 @@ class OperatorKind {
OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
bool IsBinary);

std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
ProgramStateRef State);

} // namespace ento

} // namespace clang
Expand Down
77 changes: 71 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ class MallocChecker
CHECK_FN(checkGMemdup)
CHECK_FN(checkGMallocN)
CHECK_FN(checkGMallocN0)
CHECK_FN(preGetdelim)
CHECK_FN(checkGetdelim)
CHECK_FN(checkReallocN)
CHECK_FN(checkOwnershipAttr)

Expand All @@ -391,6 +393,11 @@ class MallocChecker
using CheckFn = std::function<void(const MallocChecker *,
const CallEvent &Call, CheckerContext &C)>;

const CallDescriptionMap<CheckFn> PreFnMap{
{{{"getline"}, 3}, &MallocChecker::preGetdelim},
{{{"getdelim"}, 4}, &MallocChecker::preGetdelim},
};

const CallDescriptionMap<CheckFn> FreeingMemFnMap{
{{{"free"}, 1}, &MallocChecker::checkFree},
{{{"if_freenameindex"}, 1}, &MallocChecker::checkIfFreeNameIndex},
Expand Down Expand Up @@ -439,6 +446,8 @@ class MallocChecker
std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)},
{{{"g_realloc_n"}, 3}, &MallocChecker::checkReallocN},
{{{"g_try_realloc_n"}, 3}, &MallocChecker::checkReallocN},
{{{"getline"}, 3}, &MallocChecker::checkGetdelim},
{{{"getdelim"}, 4}, &MallocChecker::checkGetdelim},
};

bool isMemCall(const CallEvent &Call) const;
Expand Down Expand Up @@ -588,11 +597,14 @@ class MallocChecker
/// }
/// \param [in] ReturnsNullOnFailure Whether the memory deallocation function
/// we're modeling returns with Null on failure.
/// \param [in] ArgValOpt Optional value to use for the argument instead of
/// the one obtained from ArgExpr.
/// \returns The ProgramState right after deallocation.
[[nodiscard]] ProgramStateRef
FreeMemAux(CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
AllocationFamily Family, bool ReturnsNullOnFailure = false) const;
AllocationFamily Family, bool ReturnsNullOnFailure = false,
std::optional<SVal> ArgValOpt = {}) const;

// TODO: Needs some refactoring, as all other deallocation modeling
// functions are suffering from out parameters and messy code due to how
Expand Down Expand Up @@ -1423,6 +1435,50 @@ void MallocChecker::checkGMallocN0(const CallEvent &Call,
C.addTransition(State);
}

void MallocChecker::preGetdelim(const CallEvent &Call,
CheckerContext &C) const {
if (!Call.isGlobalCFunction())
return;
Comment on lines +1440 to +1441
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in the middle of refactoring CallDescription::matches() and after that there will be a cleaner alternative instead of this kind of late check. (No action needed from you, I'll update this code along with all the other checkers.)


ProgramStateRef State = C.getState();
const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
if (!LinePtr)
return;

// FreeMemAux takes IsKnownToBeAllocated as an output parameter, and it will
// be true after the call if the symbol was registered by this checker.
// We do not need this value here, as FreeMemAux will take care
// of reporting any violation of the preconditions.
bool IsKnownToBeAllocated = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it correctly, you declare this bool because FreeMemAux needs a reference that'll be use as an out parameter; but you don't actually use the value that's returned in it.

Consider adding a comment that explains this, because out-parameters are not that common and the fact that this acts as an out-parameter is not marked (unlike the older style when out-parameters are implemented with pointers and when &TargetVariable is passed to a function you can guess that it's an out-parameter). Originally I thought that this is an in-parameter and you put it into a named variable to explain its purpose (as an alternative to the comment /*IsKnownToBeAllocated=*/).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment to clarify the intent of this variable.

State = FreeMemAux(C, Call.getArgExpr(0), Call, State, false,
IsKnownToBeAllocated, AF_Malloc, false, LinePtr);
if (State)
C.addTransition(State);
}

void MallocChecker::checkGetdelim(const CallEvent &Call,
CheckerContext &C) const {
if (!Call.isGlobalCFunction())
return;

ProgramStateRef State = C.getState();
// Handle the post-conditions of getline and getdelim:
// Register the new conjured value as an allocated buffer.
const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
if (!CE)
return;

SValBuilder &SVB = C.getSValBuilder();

const auto LinePtr = getPointeeDefVal(Call.getArgSVal(0), State);
const auto Size = getPointeeDefVal(Call.getArgSVal(1), State);
if (!LinePtr || !Size || !LinePtr->getAsRegion())
return;

State = setDynamicExtent(State, LinePtr->getAsRegion(), *Size, SVB);
C.addTransition(MallocUpdateRefState(C, CE, State, AF_Malloc, *LinePtr));
}

void MallocChecker::checkReallocN(const CallEvent &Call,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
Expand Down Expand Up @@ -1895,15 +1951,17 @@ static void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) {
}
}

ProgramStateRef MallocChecker::FreeMemAux(
CheckerContext &C, const Expr *ArgExpr, const CallEvent &Call,
ProgramStateRef State, bool Hold, bool &IsKnownToBeAllocated,
AllocationFamily Family, bool ReturnsNullOnFailure) const {
ProgramStateRef
MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
const CallEvent &Call, ProgramStateRef State,
bool Hold, bool &IsKnownToBeAllocated,
AllocationFamily Family, bool ReturnsNullOnFailure,
std::optional<SVal> ArgValOpt) const {

if (!State)
return nullptr;

SVal ArgVal = C.getSVal(ArgExpr);
SVal ArgVal = ArgValOpt.value_or(C.getSVal(ArgExpr));
if (!isa<DefinedOrUnknownSVal>(ArgVal))
return nullptr;
DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>();
Expand Down Expand Up @@ -2881,6 +2939,13 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
return;
}

// We need to handle getline pre-conditions here before the pointed region
// gets invalidated by StreamChecker
if (const auto *PreFN = PreFnMap.lookup(Call)) {
(*PreFN)(this, Call, C);
return;
}

// We will check for double free in the post visit.
if (const AnyFunctionCall *FC = dyn_cast<AnyFunctionCall>(&Call)) {
const FunctionDecl *FD = FC->getDecl();
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include <optional>

namespace clang {
Expand Down Expand Up @@ -182,5 +183,13 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
}
}

std::optional<DefinedSVal> getPointeeDefVal(SVal PtrSVal,
ProgramStateRef State) {
if (const auto *Ptr = PtrSVal.getAsRegion()) {
return State->getSVal(Ptr).getAs<DefinedSVal>();
}
return std::nullopt;
}

} // namespace ento
} // namespace clang
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ typedef __typeof(sizeof(int)) size_t;
void *malloc(size_t);
void *calloc(size_t, size_t);
void free(void *);
void *alloca(size_t);


#if __OBJC__
Expand Down
95 changes: 95 additions & 0 deletions clang/test/Analysis/getline-alloc.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify %s

// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.unix,debug.ExprInspection -verify %s

#include "Inputs/system-header-simulator.h"
#include "Inputs/system-header-simulator-for-malloc.h"

void test_getline_null_buffer() {
FILE *F1 = tmpfile();
if (!F1)
return;
char *buffer = NULL;
size_t n = 0;
if (getline(&buffer, &n, F1) > 0) {
char c = buffer[0]; // ok
}
free(buffer);
fclose(F1);
}

void test_getline_malloc_buffer() {
FILE *F1 = tmpfile();
if (!F1)
return;

size_t n = 10;
char *buffer = malloc(n);
char *ptr = buffer;

ssize_t r = getdelim(&buffer, &n, '\r', F1);
// ptr may be dangling
free(ptr); // expected-warning {{Attempt to free released memory}}
free(buffer); // ok
fclose(F1);
}

void test_getline_alloca() {
FILE *F1 = tmpfile();
if (!F1)
return;
size_t n = 10;
char *buffer = alloca(n);
getline(&buffer, &n, F1); // expected-warning {{Memory allocated by alloca() should not be deallocated}}
fclose(F1);
}

void test_getline_invalid_ptr() {
FILE *F1 = tmpfile();
if (!F1)
return;
size_t n = 10;
char *buffer = (char*)test_getline_invalid_ptr;
getline(&buffer, &n, F1); // expected-warning {{Argument to getline() is the address of the function 'test_getline_invalid_ptr', which is not memory allocated by malloc()}}
fclose(F1);
}

void test_getline_leak() {
FILE *F1 = tmpfile();
if (!F1)
return;

char *buffer = NULL;
size_t n = 0;
ssize_t read;

while ((read = getline(&buffer, &n, F1)) != -1) {
printf("%s\n", buffer);
}

fclose(F1); // expected-warning {{Potential memory leak}}
}

void test_getline_stack() {
size_t n = 10;
char buffer[10];
char *ptr = buffer;

FILE *F1 = tmpfile();
if (!F1)
return;

getline(&ptr, &n, F1); // expected-warning {{Argument to getline() is the address of the local variable 'buffer', which is not memory allocated by malloc()}}
}

void test_getline_static() {
static size_t n = 10;
static char buffer[10];
char *ptr = buffer;

FILE *F1 = tmpfile();
if (!F1)
return;

getline(&ptr, &n, F1); // expected-warning {{Argument to getline() is the address of the static variable 'buffer', which is not memory allocated by malloc()}}
}