Skip to content

avoid more unchecked pointer dereferences #6293

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.

Sign up for GitHub

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
chrchr-github merged 1 commit into from
Apr 15, 2024
Merged

avoid more unchecked pointer dereferences #6293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3286,7 +3286,7 @@ bool isConstVarExpression(const Token *tok, const std::function<bool(const Token
return false;
}

static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings)
static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings& settings)
{
const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&");

Expand Down Expand Up @@ -3326,11 +3326,11 @@ static ExprUsage getFunctionUsage(const Token* tok, int indirect, const Settings
if (ftok->variable()->isStlType() || (ftok->variable()->valueType() && ftok->variable()->valueType()->container)) // STL types or containers don't initialize external variables
return ExprUsage::Used;
} else {
const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1);
const bool isnullbad = settings.library.isnullargbad(ftok, argnr + 1);
if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad)
return ExprUsage::Used;
bool hasIndirect = false;
const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
const bool isuninitbad = settings.library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect);
if (isuninitbad && (!addressOf || isnullbad))
return ExprUsage::Used;
}
Expand All @@ -3349,7 +3349,7 @@ bool isLeafDot(const Token* tok)
return isLeafDot(parent);
}

ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings)
ExprUsage getExprUsage(const Token* tok, int indirect, const Settings& settings)
{
const Token* parent = tok->astParent();
if (indirect > 0 && parent) {
Expand All @@ -3365,7 +3365,7 @@ ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings)
return ExprUsage::NotUsed;
if (Token::simpleMatch(parent, ":") && Token::simpleMatch(parent->astParent(), "?"))
return getExprUsage(parent->astParent(), indirect, settings);
if (isUsedAsBool(tok, settings))
if (isUsedAsBool(tok, &settings))
return ExprUsage::NotUsed;
}
if (indirect == 0) {
Expand Down
2 changes: 1 addition & 1 deletion lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ bool isLeafDot(const Token* tok);

enum class ExprUsage { None, NotUsed, PassedByReference, Used, Inconclusive };

ExprUsage getExprUsage(const Token* tok, int indirect, const Settings* settings);
ExprUsage getExprUsage(const Token* tok, int indirect, const Settings& settings);

const Variable *getLHSVariable(const Token *tok);

Expand Down
4 changes: 1 addition & 3 deletions lib/check.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,7 @@ class CPPCHECKLIB Check {
}
};

virtual FileInfo * getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const {
(void)tokenizer;
(void)settings;
virtual FileInfo * getFileInfo(const Tokenizer& /*tokenizer*/, const Settings& /*settings*/) const {
return nullptr;
}

Expand Down
12 changes: 6 additions & 6 deletions lib/checkbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,11 +915,11 @@ namespace
};
}

bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings *settings, const Token *argtok, MathLib::bigint *offset, int type)
bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings &settings, const Token *argtok, MathLib::bigint *offset, int type)
{
if (!offset)
return false;
if (!argtok->valueType() || argtok->valueType()->typeSize(settings->platform) == 0)
if (!argtok->valueType() || argtok->valueType()->typeSize(settings.platform) == 0)
return false;
const Token *indexTok = nullptr;
if (type == 1 && Token::Match(argtok, "%name% [") && argtok->astParent() == argtok->next() && !Token::simpleMatch(argtok->linkAt(1), "] ["))
Expand All @@ -932,22 +932,22 @@ bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings *settings, const
return false;
if (!indexTok->hasKnownIntValue())
return false;
*offset = indexTok->getKnownIntValue() * argtok->valueType()->typeSize(settings->platform);
*offset = indexTok->getKnownIntValue() * argtok->valueType()->typeSize(settings.platform);
return true;
}

bool CheckBufferOverrun::isCtuUnsafeArrayIndex(const Settings *settings, const Token *argtok, MathLib::bigint *offset)
bool CheckBufferOverrun::isCtuUnsafeArrayIndex(const Settings &settings, const Token *argtok, MathLib::bigint *offset)
{
return isCtuUnsafeBufferUsage(settings, argtok, offset, 1);
}

bool CheckBufferOverrun::isCtuUnsafePointerArith(const Settings *settings, const Token *argtok, MathLib::bigint *offset)
bool CheckBufferOverrun::isCtuUnsafePointerArith(const Settings &settings, const Token *argtok, MathLib::bigint *offset)
{
return isCtuUnsafeBufferUsage(settings, argtok, offset, 2);
}

/** @brief Parse current TU and extract file info */
Check::FileInfo *CheckBufferOverrun::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
Check::FileInfo *CheckBufferOverrun::getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const
{
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeArrayIndex = CTU::getUnsafeUsage(tokenizer, settings, isCtuUnsafeArrayIndex);
const std::list<CTU::FileInfo::UnsafeUsage> &unsafePointerArith = CTU::getUnsafeUsage(tokenizer, settings, isCtuUnsafePointerArith);
Expand Down
8 changes: 4 additions & 4 deletions lib/checkbufferoverrun.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
}

/** @brief Parse current TU and extract file info */
Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const override;
Check::FileInfo *getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const override;

/** @brief Analyse all file infos for all TU */
bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list<Check::FileInfo*> &fileInfo, const Settings& settings, ErrorLogger &errorLogger) override;
Expand Down Expand Up @@ -130,9 +130,9 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
ValueFlow::Value getBufferSize(const Token *bufTok) const;

// CTU
static bool isCtuUnsafeBufferUsage(const Settings *settings, const Token *argtok, MathLib::bigint *offset, int type);
static bool isCtuUnsafeArrayIndex(const Settings *settings, const Token *argtok, MathLib::bigint *offset);
static bool isCtuUnsafePointerArith(const Settings *settings, const Token *argtok, MathLib::bigint *offset);
static bool isCtuUnsafeBufferUsage(const Settings &settings, const Token *argtok, MathLib::bigint *offset, int type);
static bool isCtuUnsafeArrayIndex(const Settings &settings, const Token *argtok, MathLib::bigint *offset);
static bool isCtuUnsafePointerArith(const Settings &settings, const Token *argtok, MathLib::bigint *offset);

Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const override;
static bool analyseWholeProgram1(const std::map<std::string, std::list<const CTU::FileInfo::CallBase *>> &callsMap, const CTU::FileInfo::UnsafeUsage &unsafeUsage, int type, ErrorLogger &errorLogger);
Expand Down
10 changes: 5 additions & 5 deletions lib/checkclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3533,14 +3533,14 @@ namespace
};
}

Check::FileInfo *CheckClass::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
Check::FileInfo *CheckClass::getFileInfo(const Tokenizer &tokenizer, const Settings& /*settings*/) const
{
if (!tokenizer->isCPP())
if (!tokenizer.isCPP())
return nullptr;
(void)settings;

// One definition rule
std::vector<MyFileInfo::NameLoc> classDefinitions;
for (const Scope * classScope : tokenizer->getSymbolDatabase()->classAndStructScopes) {
for (const Scope * classScope : tokenizer.getSymbolDatabase()->classAndStructScopes) {
if (classScope->isAnonymous())
continue;

Expand Down Expand Up @@ -3575,7 +3575,7 @@ Check::FileInfo *CheckClass::getFileInfo(const Tokenizer *tokenizer, const Setti

MyFileInfo::NameLoc nameLoc;
nameLoc.className = std::move(name);
nameLoc.fileName = tokenizer->list.file(classScope->classDef);
nameLoc.fileName = tokenizer.list.file(classScope->classDef);
nameLoc.lineNumber = classScope->classDef->linenr();
nameLoc.column = classScope->classDef->column();

Expand Down
2 changes: 1 addition & 1 deletion lib/checkclass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class CPPCHECKLIB CheckClass : public Check {
void checkUnsafeClassRefMember();

/** @brief Parse current TU and extract file info */
Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const override;
Check::FileInfo *getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const override;

Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const override;

Expand Down
2 changes: 1 addition & 1 deletion lib/checkleakautovar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ const Token * CheckLeakAutoVar::checkTokenInsideExpression(const Token * const t
const std::map<int, VarInfo::AllocInfo>::const_iterator var = varInfo.alloctype.find(tok->varId());
if (var != varInfo.alloctype.end()) {
bool unknown = false;
if (var->second.status == VarInfo::DEALLOC && CheckNullPointer::isPointerDeRef(tok, unknown, mSettings) && !unknown) {
if (var->second.status == VarInfo::DEALLOC && CheckNullPointer::isPointerDeRef(tok, unknown, *mSettings) && !unknown) {
Copy link
Collaborator Author

@firewave firewave Apr 15, 2024

Choose a reason for hiding this comment

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

Dereferencing those pointers in the check implementations is safe since we know they are never nullptr (and already is done all over the place). This will go away after #5323 is finished.

deallocUseError(tok, tok->str());
} else if (Token::simpleMatch(tok->tokAt(-2), "= &")) {
varInfo.erase(tok->varId());
Expand Down
46 changes: 22 additions & 24 deletions lib/checknullpointer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,28 +62,26 @@ static bool checkNullpointerFunctionCallPlausibility(const Function* func, unsig
* @param var variables that the function read / write.
* @param library --library files data
*/
void CheckNullPointer::parseFunctionCall(const Token &tok, std::list<const Token *> &var, const Library *library)
void CheckNullPointer::parseFunctionCall(const Token &tok, std::list<const Token *> &var, const Library &library)
{
if (Token::Match(&tok, "%name% ( )") || !tok.tokAt(2))
return;

const std::vector<const Token *> args = getArguments(&tok);

if (library || tok.function() != nullptr) {
for (int argnr = 1; argnr <= args.size(); ++argnr) {
const Token *param = args[argnr - 1];
if (library && library->isnullargbad(&tok, argnr) && checkNullpointerFunctionCallPlausibility(tok.function(), argnr))
for (int argnr = 1; argnr <= args.size(); ++argnr) {
const Token *param = args[argnr - 1];
if (library.isnullargbad(&tok, argnr) && checkNullpointerFunctionCallPlausibility(tok.function(), argnr))
var.push_back(param);
else if (tok.function()) {
const Variable* argVar = tok.function()->getArgumentVar(argnr-1);
if (argVar && argVar->isStlStringType() && !argVar->isArrayOrPointer())
var.push_back(param);
else if (tok.function()) {
const Variable* argVar = tok.function()->getArgumentVar(argnr-1);
if (argVar && argVar->isStlStringType() && !argVar->isArrayOrPointer())
var.push_back(param);
}
}
}

if (library && library->formatstr_function(&tok)) {
const int formatStringArgNr = library->formatstr_argno(&tok);
if (library.formatstr_function(&tok)) {
const int formatStringArgNr = library.formatstr_argno(&tok);
if (formatStringArgNr < 0 || formatStringArgNr >= args.size())
return;

Expand All @@ -95,7 +93,7 @@ void CheckNullPointer::parseFunctionCall(const Token &tok, std::list<const Token
return;
const std::string &formatString = args[formatStringArgNr]->strValue();
int argnr = formatStringArgNr + 1;
const bool scan = library->formatstr_scan(&tok);
const bool scan = library.formatstr_scan(&tok);

bool percent = false;
for (std::string::const_iterator i = formatString.cbegin(); i != formatString.cend(); ++i) {
Expand Down Expand Up @@ -148,15 +146,15 @@ namespace {
*/
bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown) const
{
return isPointerDeRef(tok, unknown, mSettings);
return isPointerDeRef(tok, unknown, *mSettings);
}

bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Settings *settings)
bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Settings &settings)
{
unknown = false;

// Is pointer used as function parameter?
if (Token::Match(tok->previous(), "[(,] %name% [,)]") && settings) {
if (Token::Match(tok->previous(), "[(,] %name% [,)]")) {
const Token *ftok = tok->previous();
while (ftok && ftok->str() != "(") {
if (ftok->str() == ")")
Expand All @@ -165,7 +163,7 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
}
if (ftok && ftok->previous()) {
std::list<const Token *> varlist;
parseFunctionCall(*ftok->previous(), varlist, &settings->library);
parseFunctionCall(*ftok->previous(), varlist, settings.library);
if (std::find(varlist.cbegin(), varlist.cend(), tok) != varlist.cend()) {
return true;
}
Expand Down Expand Up @@ -263,7 +261,7 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
}


static bool isNullablePointer(const Token* tok, const Settings* settings)
static bool isNullablePointer(const Token* tok)
{
if (!tok)
return false;
Expand All @@ -274,7 +272,7 @@ static bool isNullablePointer(const Token* tok, const Settings* settings)
if (astIsSmartPointer(tok))
return true;
if (Token::simpleMatch(tok, "."))
return isNullablePointer(tok->astOperand2(), settings);
return isNullablePointer(tok->astOperand2());
if (const Variable* var = tok->variable()) {
return (var->isPointer() || var->isSmartPointer());
}
Expand All @@ -294,8 +292,8 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
if (Token::Match(tok, "%num%|%char%|%str%"))
continue;

if (!isNullablePointer(tok, mSettings) ||
(tok->str() == "." && isNullablePointer(tok->astOperand2(), mSettings) && tok->astOperand2()->getValue(0))) // avoid duplicate warning
if (!isNullablePointer(tok) ||
(tok->str() == "." && isNullablePointer(tok->astOperand2()) && tok->astOperand2()->getValue(0))) // avoid duplicate warning
continue;

// Can pointer be NULL?
Expand Down Expand Up @@ -367,7 +365,7 @@ void CheckNullPointer::nullConstantDereference()
nullPointerError(tok);
} else { // function call
std::list<const Token *> var;
parseFunctionCall(*tok, var, &mSettings->library);
parseFunctionCall(*tok, var, mSettings->library);

// is one of the var items a NULL pointer?
for (const Token *vartok : var) {
Expand Down Expand Up @@ -551,7 +549,7 @@ void CheckNullPointer::redundantConditionWarning(const Token* tok, const ValueFl
}

// NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature
static bool isUnsafeUsage(const Settings *settings, const Token *vartok, MathLib::bigint *value)
static bool isUnsafeUsage(const Settings &settings, const Token *vartok, MathLib::bigint *value)
{
(void)value;
bool unknown = false;
Expand Down Expand Up @@ -580,7 +578,7 @@ namespace
};
}

Check::FileInfo *CheckNullPointer::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
Check::FileInfo *CheckNullPointer::getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const
{
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeUsage = CTU::getUnsafeUsage(tokenizer, settings, isUnsafeUsage);
if (unsafeUsage.empty())
Expand Down
6 changes: 3 additions & 3 deletions lib/checknullpointer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class CPPCHECKLIB CheckNullPointer : public Check {
*/
bool isPointerDeRef(const Token *tok, bool &unknown) const;

static bool isPointerDeRef(const Token *tok, bool &unknown, const Settings *settings);
static bool isPointerDeRef(const Token *tok, bool &unknown, const Settings &settings);

private:
/**
Expand All @@ -78,7 +78,7 @@ class CPPCHECKLIB CheckNullPointer : public Check {
*/
static void parseFunctionCall(const Token &tok,
std::list<const Token *> &var,
const Library *library);
const Library &library);

/** @brief This constructor is used when running checks. */
CheckNullPointer(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
Expand Down Expand Up @@ -106,7 +106,7 @@ class CPPCHECKLIB CheckNullPointer : public Check {
void nullPointerError(const Token *tok, const std::string &varname, const ValueFlow::Value* value, bool inconclusive);

/** @brief Parse current TU and extract file info */
Check::FileInfo *getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const override;
Check::FileInfo *getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const override;

Check::FileInfo * loadFileInfoFromXml(const tinyxml2::XMLElement *xmlElement) const override;

Expand Down
2 changes: 1 addition & 1 deletion lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3363,7 +3363,7 @@ void CheckOther::checkAccessOfMovedVariable()
else
inconclusive = true;
} else {
const ExprUsage usage = getExprUsage(tok, 0, mSettings);
const ExprUsage usage = getExprUsage(tok, 0, *mSettings);
if (usage == ExprUsage::Used)
accessOfMoved = true;
if (usage == ExprUsage::PassedByReference)
Expand Down
2 changes: 1 addition & 1 deletion lib/checkstl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2479,7 +2479,7 @@ void CheckStl::checkDereferenceInvalidIterator2()
emptyAdvance = tok->astParent();
}
}
if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings) && !isInvalidIterator && !emptyAdvance) {
if (!CheckNullPointer::isPointerDeRef(tok, unknown, *mSettings) && !isInvalidIterator && !emptyAdvance) {
if (!unknown)
continue;
inconclusive = true;
Expand Down
10 changes: 5 additions & 5 deletions lib/checkuninitvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1641,13 +1641,13 @@ void CheckUninitVar::valueFlowUninit()
const bool isarray = tok->variable()->isArray();
if (isarray && tok->variable()->isMember())
continue; // Todo: this is a bailout
const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, mSettings);
const bool deref = CheckNullPointer::isPointerDeRef(tok, unknown, *mSettings);
uninitderef = deref && v->indirect == 0;
const bool isleaf = isLeafDot(tok) || uninitderef;
if (!isleaf && Token::Match(tok->astParent(), ". %name%") && (tok->astParent()->next()->varId() || tok->astParent()->next()->isEnumerator()))
continue;
}
const ExprUsage usage = getExprUsage(tok, v->indirect, mSettings);
const ExprUsage usage = getExprUsage(tok, v->indirect, *mSettings);
if (usage == ExprUsage::NotUsed || usage == ExprUsage::Inconclusive)
continue;
if (!v->subexpressions.empty() && usage == ExprUsage::PassedByReference)
Expand All @@ -1670,10 +1670,10 @@ void CheckUninitVar::valueFlowUninit()
}

// NOLINTNEXTLINE(readability-non-const-parameter) - used as callback so we need to preserve the signature
static bool isVariableUsage(const Settings *settings, const Token *vartok, MathLib::bigint *value)
static bool isVariableUsage(const Settings &settings, const Token *vartok, MathLib::bigint *value)
{
(void)value;
return CheckUninitVar::isVariableUsage(vartok, settings->library, true, CheckUninitVar::Alloc::ARRAY);
return CheckUninitVar::isVariableUsage(vartok, settings.library, true, CheckUninitVar::Alloc::ARRAY);
}

// a Clang-built executable will crash when using the anonymous MyFileInfo later on - so put it in a unique namespace for now
Expand All @@ -1698,7 +1698,7 @@ namespace
};
}

Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer *tokenizer, const Settings *settings) const
Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const
{
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeUsage = CTU::getUnsafeUsage(tokenizer, settings, ::isVariableUsage);
if (unsafeUsage.empty())
Expand Down
Loading