Skip to content

avoid some unchecked pointer dereferences #4811

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
danmar merged 2 commits into from
Mar 2, 2023
Merged

avoid some unchecked pointer dereferences #4811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44fb9e9
avoid some unchecked pointer dereferences
firewave Feb 19, 2023
c004592
checkbufferoverrun.cpp: fixed `knownConditionTrueFalse` selfcheck war…
firewave Feb 20, 2023
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
14 changes: 7 additions & 7 deletions cli/cmdlineparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ static bool addFilesToList(const std::string& fileList, std::vector<std::string>
return true;
}

static bool addIncludePathsToList(const std::string& fileList, std::list<std::string>* pathNames)
static bool addIncludePathsToList(const std::string& fileList, std::list<std::string>& pathNames)
Copy link
Copy Markdown
Collaborator

@danmar danmar Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is my programming style.

If you call addIncludePathsToList(fileList, pathNames) .. both parameters by value (you can not see it's passed by reference looking at the call).. then that means the addIncludePathsToList will not modify pathNames.

Since addIncludePathsToList modifies pathNames I think it's more explicit to write addIncludePathsToList(fileList, &pathNames). You can see in the function call it might modify pathNames.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Feb 24, 2023

Choose a reason for hiding this comment

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

This is not a coding style decision. This is about safety and proper C++. If the parameter is not optional you might pass a nullptr (which we do but fortunately only is like three cases) and it will be dereferenced.

If this would be our style than we would have to convert every modifying reference to a pointer and also add checking of the pointer to each and every call. That would be a mess and would probably also hurt performance in a big way.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Feb 24, 2023

Choose a reason for hiding this comment

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

Thinking about it a bit more this style doesn't really makes sense since you will only see this on the very first call and not subsequent ones in the chain.

{
std::ifstream files(fileList);
if (files) {
Expand All @@ -93,20 +93,20 @@ static bool addIncludePathsToList(const std::string& fileList, std::list<std::st
if (!endsWith(pathName, '/'))
pathName += '/';

pathNames->emplace_back(std::move(pathName));
pathNames.emplace_back(std::move(pathName));
}
}
return true;
}
return false;
}

static bool addPathsToSet(const std::string& fileName, std::set<std::string>* set)
static bool addPathsToSet(const std::string& fileName, std::set<std::string>& set)
{
std::list<std::string> templist;
if (!addIncludePathsToList(fileName, &templist))
if (!addIncludePathsToList(fileName, templist))
return false;
set->insert(templist.cbegin(), templist.cend());
set.insert(templist.cbegin(), templist.cend());
return true;
}

Expand Down Expand Up @@ -276,7 +276,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
else if (std::strncmp(argv[i], "--config-excludes-file=", 23) == 0) {
// open this file and read every input file (1 file name per line)
const std::string cfgExcludesFile(23 + argv[i]);
if (!addPathsToSet(cfgExcludesFile, &mSettings->configExcludePaths)) {
if (!addPathsToSet(cfgExcludesFile, mSettings->configExcludePaths)) {
printError("unable to open config excludes file at '" + cfgExcludesFile + "'");
return false;
}
Expand Down Expand Up @@ -465,7 +465,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
else if (std::strncmp(argv[i], "--includes-file=", 16) == 0) {
// open this file and read every input file (1 file name per line)
const std::string includesFile(16 + argv[i]);
if (!addIncludePathsToList(includesFile, &mSettings->includePaths)) {
if (!addIncludePathsToList(includesFile, mSettings->includePaths)) {
printError("unable to open includes file at '" + includesFile + "'");
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions cli/cppcheckexecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,9 +573,9 @@ bool CppCheckExecutor::tryLoadLibrary(Library& destination, const std::string& b
*/
// cppcheck-suppress passedByValue - used as callback so we need to preserve the signature
// NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output_)
bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_)
{
output_->clear();
output_.clear();

std::string joinedArgs;
for (const std::string &arg : args) {
Expand All @@ -601,7 +601,7 @@ bool CppCheckExecutor::executeCommand(std::string exe, std::vector<std::string>
return false;
char buffer[1024];
while (fgets(buffer, sizeof(buffer), pipe.get()) != nullptr)
*output_ += buffer;
output_ += buffer;
return true;
}

2 changes: 1 addition & 1 deletion cli/cppcheckexecutor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class CppCheckExecutor : public ErrorLogger {
/**
* Execute a shell command and read the output from it. Returns true if command terminated successfully.
*/
static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output_);
static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output_);

static bool reportSuppressions(const Settings &settings, bool unusedFunctionCheckEnabled, const std::map<std::string, std::size_t> &files, ErrorLogger& errorLogger);

Expand Down
8 changes: 4 additions & 4 deletions gui/checkthread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@
#include <QSettings>

// NOLINTNEXTLINE(performance-unnecessary-value-param) - used as callback so we need to preserve the signature
static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string *output)
static bool executeCommand(std::string exe, std::vector<std::string> args, std::string redirect, std::string &output)
{
output->clear();
output.clear();

QStringList args2;
for (const std::string &arg: args)
Expand All @@ -60,9 +60,9 @@ static bool executeCommand(std::string exe, std::vector<std::string> args, std::
if (redirect == "2>&1") {
QString s1 = process.readAllStandardOutput();
QString s2 = process.readAllStandardError();
*output = (s1 + "\n" + s2).toStdString();
output = (s1 + "\n" + s2).toStdString();
} else
*output = process.readAllStandardOutput().toStdString();
output = process.readAllStandardOutput().toStdString();

if (redirect.compare(0,3,"2> ") == 0) {
std::ofstream fout(redirect.substr(3));
Expand Down
6 changes: 3 additions & 3 deletions lib/analyzerinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ void AnalyzerInformation::close()
}
}

static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash, std::list<ErrorMessage> *errors)
static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash, std::list<ErrorMessage> &errors)
{
tinyxml2::XMLDocument doc;
const tinyxml2::XMLError error = doc.LoadFile(analyzerInfoFile.c_str());
Expand All @@ -89,7 +89,7 @@ static bool skipAnalysis(const std::string &analyzerInfoFile, std::size_t hash,

for (const tinyxml2::XMLElement *e = rootNode->FirstChildElement(); e; e = e->NextSiblingElement()) {
if (std::strcmp(e->Name(), "error") == 0)
errors->emplace_back(e);
errors.emplace_back(e);
}

return true;
Expand Down Expand Up @@ -127,7 +127,7 @@ std::string AnalyzerInformation::getAnalyzerInfoFile(const std::string &buildDir
return Path::join(buildDir, filename) + ".analyzerinfo";
}

bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, std::size_t hash, std::list<ErrorMessage> *errors)
bool AnalyzerInformation::analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, std::size_t hash, std::list<ErrorMessage> &errors)
{
if (buildDir.empty() || sourcefile.empty())
return true;
Expand Down
2 changes: 1 addition & 1 deletion lib/analyzerinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class CPPCHECKLIB AnalyzerInformation {

/** Close current TU.analyzerinfo file */
void close();
bool analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, std::size_t hash, std::list<ErrorMessage> *errors);
bool analyzeFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg, std::size_t hash, std::list<ErrorMessage> &errors);
void reportErr(const ErrorMessage &msg);
void setFileInfo(const std::string &check, const std::string &fileInfo);
static std::string getAnalyzerInfoFile(const std::string &buildDir, const std::string &sourcefile, const std::string &cfg);
Expand Down
40 changes: 20 additions & 20 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ static int getArgumentPos(const Token* ftok, const Token* tokToFind){
}

template<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*> )>
static void astFlattenRecursive(T* tok, std::vector<T*>* result, const char* op, nonneg int depth = 0)
static void astFlattenRecursive(T* tok, std::vector<T*>& result, const char* op, nonneg int depth = 0)
{
++depth;
if (!tok || depth >= 100)
Expand All @@ -118,21 +118,21 @@ static void astFlattenRecursive(T* tok, std::vector<T*>* result, const char* op,
astFlattenRecursive(tok->astOperand1(), result, op, depth);
astFlattenRecursive(tok->astOperand2(), result, op, depth);
} else {
result->push_back(tok);
result.push_back(tok);
}
}

std::vector<const Token*> astFlatten(const Token* tok, const char* op)
{
std::vector<const Token*> result;
astFlattenRecursive(tok, &result, op);
astFlattenRecursive(tok, result, op);
return result;
}

std::vector<Token*> astFlatten(Token* tok, const char* op)
{
std::vector<Token*> result;
astFlattenRecursive(tok, &result, op);
astFlattenRecursive(tok, result, op);
return result;
}

Expand Down Expand Up @@ -865,12 +865,12 @@ const Token *findNextTokenFromBreak(const Token *breakToken)
}

bool extractForLoopValues(const Token *forToken,
nonneg int * const varid,
bool * const knownInitValue,
MathLib::bigint * const initValue,
bool * const partialCond,
MathLib::bigint * const stepValue,
MathLib::bigint * const lastValue)
nonneg int &varid,
bool &knownInitValue,
MathLib::bigint &initValue,
bool &partialCond,
MathLib::bigint &stepValue,
MathLib::bigint &lastValue)
{
if (!Token::simpleMatch(forToken, "for (") || !Token::simpleMatch(forToken->next()->astOperand2(), ";"))
return false;
Expand All @@ -880,28 +880,28 @@ bool extractForLoopValues(const Token *forToken,
if (!initExpr || !initExpr->isBinaryOp() || initExpr->str() != "=" || !Token::Match(initExpr->astOperand1(), "%var%"))
return false;
std::vector<MathLib::bigint> minInitValue = getMinValue(ValueFlow::makeIntegralInferModel(), initExpr->astOperand2()->values());
*varid = initExpr->astOperand1()->varId();
*knownInitValue = initExpr->astOperand2()->hasKnownIntValue();
*initValue = minInitValue.empty() ? 0 : minInitValue.front();
*partialCond = Token::Match(condExpr, "%oror%|&&");
varid = initExpr->astOperand1()->varId();
knownInitValue = initExpr->astOperand2()->hasKnownIntValue();
initValue = minInitValue.empty() ? 0 : minInitValue.front();
partialCond = Token::Match(condExpr, "%oror%|&&");
visitAstNodes(condExpr, [varid, &condExpr](const Token *tok) {
if (Token::Match(tok, "%oror%|&&"))
return ChildrenToVisit::op1_and_op2;
if (Token::Match(tok, "<|<=") && tok->isBinaryOp() && tok->astOperand1()->varId() == *varid && tok->astOperand2()->hasKnownIntValue()) {
if (Token::Match(tok, "<|<=") && tok->isBinaryOp() && tok->astOperand1()->varId() == varid && tok->astOperand2()->hasKnownIntValue()) {
if (Token::Match(condExpr, "%oror%|&&") || tok->astOperand2()->getKnownIntValue() < condExpr->astOperand2()->getKnownIntValue())
condExpr = tok;
}
return ChildrenToVisit::none;
});
if (!Token::Match(condExpr, "<|<=") || !condExpr->isBinaryOp() || condExpr->astOperand1()->varId() != *varid || !condExpr->astOperand2()->hasKnownIntValue())
if (!Token::Match(condExpr, "<|<=") || !condExpr->isBinaryOp() || condExpr->astOperand1()->varId() != varid || !condExpr->astOperand2()->hasKnownIntValue())
return false;
if (!incExpr || !incExpr->isUnaryOp("++") || incExpr->astOperand1()->varId() != *varid)
if (!incExpr || !incExpr->isUnaryOp("++") || incExpr->astOperand1()->varId() != varid)
return false;
*stepValue = 1;
stepValue = 1;
if (condExpr->str() == "<")
*lastValue = condExpr->astOperand2()->getKnownIntValue() - 1;
lastValue = condExpr->astOperand2()->getKnownIntValue() - 1;
else
*lastValue = condExpr->astOperand2()->getKnownIntValue();
lastValue = condExpr->astOperand2()->getKnownIntValue();
return true;
}

Expand Down
12 changes: 6 additions & 6 deletions lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,12 +216,12 @@ const Token *findNextTokenFromBreak(const Token *breakToken);
* Extract for loop values: loopvar varid, init value, step value, last value (inclusive)
*/
bool extractForLoopValues(const Token *forToken,
nonneg int * const varid,
bool * const knownInitValue,
long long * const initValue,
bool * const partialCond,
long long * const stepValue,
long long * const lastValue);
nonneg int &varid,
bool &knownInitValue,
long long &initValue,
bool &partialCond,
long long &stepValue,
long long &lastValue);

bool precedes(const Token * tok1, const Token * tok2);
bool succeeds(const Token* tok1, const Token* tok2);
Expand Down
29 changes: 14 additions & 15 deletions lib/checkbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,47 +183,46 @@ static int getMinFormatStringOutputLength(const std::vector<const Token*> &param

//---------------------------------------------------------------------------

static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector<Dimension> * const dimensions, ErrorPath * const errorPath, bool * const mightBeLarger, MathLib::bigint* path)
static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector<Dimension> &dimensions, ErrorPath &errorPath, bool &mightBeLarger, MathLib::bigint &path)
{
const Token *array = arrayToken;
while (Token::Match(array, ".|::"))
array = array->astOperand2();

if (array->variable() && array->variable()->isArray() && !array->variable()->dimensions().empty()) {
*dimensions = array->variable()->dimensions();
if (dimensions->size() >= 1 && ((*dimensions)[0].num <= 1 || !(*dimensions)[0].tok)) {
dimensions = array->variable()->dimensions();
if (dimensions[0].num <= 1 || !dimensions[0].tok) {
visitAstNodes(arrayToken,
[&](const Token *child) {
if (child->originalName() == "->") {
*mightBeLarger = true;
mightBeLarger = true;
return ChildrenToVisit::none;
}
return ChildrenToVisit::op1_and_op2;
});
}
} else if (const Token *stringLiteral = array->getValueTokenMinStrSize(settings, path)) {
} else if (const Token *stringLiteral = array->getValueTokenMinStrSize(settings, &path)) {
Dimension dim;
dim.tok = nullptr;
dim.num = Token::getStrArraySize(stringLiteral);
dim.known = array->hasKnownValue();
dimensions->emplace_back(dim);
dimensions.emplace_back(dim);
} else if (array->valueType() && array->valueType()->pointer >= 1 && (array->valueType()->isIntegral() || array->valueType()->isFloat())) {
const ValueFlow::Value *value = getBufferSizeValue(array);
if (!value)
return false;
if (path)
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Feb 20, 2023

Choose a reason for hiding this comment

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

I removed this check since it is always provided and all other output parameters were unchecked.

*path = value->path;
*errorPath = value->errorPath;
path = value->path;
errorPath = value->errorPath;
Dimension dim;
dim.known = value->isKnown();
dim.tok = nullptr;
const int typeSize = array->valueType()->typeSize(*settings, array->valueType()->pointer > 1);
if (typeSize == 0)
return false;
dim.num = value->intvalue / typeSize;
dimensions->emplace_back(dim);
dimensions.emplace_back(dim);
}
return !dimensions->empty();
return !dimensions.empty();
}

static ValueFlow::Value makeSizeValue(MathLib::bigint size, MathLib::bigint path)
Expand Down Expand Up @@ -314,7 +313,7 @@ void CheckBufferOverrun::arrayIndex()
ErrorPath errorPath;
bool mightBeLarger = false;
MathLib::bigint path = 0;
if (!getDimensionsEtc(tok->astOperand1(), mSettings, &dimensions, &errorPath, &mightBeLarger, &path))
if (!getDimensionsEtc(tok->astOperand1(), mSettings, dimensions, errorPath, mightBeLarger, path))
continue;

const Variable* const var = array->variable();
Expand Down Expand Up @@ -486,7 +485,7 @@ void CheckBufferOverrun::pointerArithmetic()
ErrorPath errorPath;
bool mightBeLarger = false;
MathLib::bigint path = 0;
if (!getDimensionsEtc(arrayToken, mSettings, &dimensions, &errorPath, &mightBeLarger, &path))
if (!getDimensionsEtc(arrayToken, mSettings, dimensions, errorPath, mightBeLarger, path))
continue;

if (tok->str() == "+") {
Expand Down Expand Up @@ -881,6 +880,8 @@ std::string CheckBufferOverrun::MyFileInfo::toString() const

bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *offset, int type)
{
if (!offset)
Copy link
Copy Markdown
Collaborator Author

@firewave firewave Feb 20, 2023

Choose a reason for hiding this comment

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

Moved this check here since it would do all the work and then bailout if no output parameter was provided.

return false;
const CheckBufferOverrun *c = dynamic_cast<const CheckBufferOverrun *>(check);
if (!c)
return false;
Expand All @@ -897,8 +898,6 @@ bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token
return false;
if (!indexTok->hasKnownIntValue())
return false;
if (!offset)
return false;
*offset = indexTok->getKnownIntValue() * argtok->valueType()->typeSize(*c->mSettings);
return true;
}
Expand Down
Loading