Skip to content

Commit

Permalink
Fix #13521 (Duplicate warnings nullPointerOutOfMemory and ctunullpoin…
Browse files Browse the repository at this point in the history 
…ter)
  • Loading branch information
@danmar
danmar committed Feb 6, 2025
1 parent cdf9d6d commit 74b0803
Show file tree
Hide file tree
Showing 8 changed files with 149 additions and 42 deletions.
18 changes: 9 additions & 9 deletions lib/checkbufferoverrun.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@ namespace
};
}

bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings &settings, const Token *argtok, MathLib::bigint *offset, int type)
bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *offset, int type)
{
if (!offset)
return false;
Expand All @@ -931,16 +931,16 @@ bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Settings &settings, const
return false;
if (!indexTok->hasKnownIntValue())
return false;
*offset = indexTok->getKnownIntValue() * argtok->valueType()->typeSize(settings.platform);
offset->value = 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, CTU::FileInfo::Value *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, CTU::FileInfo::Value* offset)
{
return isCtuUnsafeBufferUsage(settings, argtok, offset, 2);
}
Expand Down Expand Up @@ -1030,14 +1030,14 @@ bool CheckBufferOverrun::analyseWholeProgram1(const std::map<std::string, std::l

if (type == 1) {
errorId = "ctuArrayIndex";
if (unsafeUsage.value > 0)
errmsg = "Array index out of bounds; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue) + " and it is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".";
if (unsafeUsage.value.value > 0)
errmsg = "Array index out of bounds; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue.value) + " and it is accessed at offset " + MathLib::toString(unsafeUsage.value.value) + ".";
else
errmsg = "Array index out of bounds; buffer '" + unsafeUsage.myArgumentName + "' is accessed at offset " + MathLib::toString(unsafeUsage.value) + ".";
cwe = (unsafeUsage.value > 0) ? CWE_BUFFER_OVERRUN : CWE_BUFFER_UNDERRUN;
errmsg = "Array index out of bounds; buffer '" + unsafeUsage.myArgumentName + "' is accessed at offset " + MathLib::toString(unsafeUsage.value.value) + ".";
cwe = (unsafeUsage.value.value > 0) ? CWE_BUFFER_OVERRUN : CWE_BUFFER_UNDERRUN;
} else {
errorId = "ctuPointerArith";
errmsg = "Pointer arithmetic overflow; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue);
errmsg = "Pointer arithmetic overflow; '" + unsafeUsage.myArgumentName + "' buffer size is " + MathLib::toString(functionCall->callArgValue.value);
cwe = CWE_POINTER_ARITHMETIC_OVERFLOW;
}

Expand Down
6 changes: 3 additions & 3 deletions lib/checkbufferoverrun.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,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, CTU::FileInfo::Value *offset, int type);
static bool isCtuUnsafeArrayIndex(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *offset);
static bool isCtuUnsafePointerArith(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *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,
Expand Down
22 changes: 18 additions & 4 deletions lib/checknullpointer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,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, CTU::FileInfo::Value *value)
{
(void)value;
bool unknown = false;
Expand Down Expand Up @@ -659,22 +659,36 @@ bool CheckNullPointer::analyseWholeProgram(const CTU::FileInfo &ctu, const std::
if (warning == 1 && !settings.severity.isEnabled(Severity::warning))
break;

std::uint8_t unknownFunctionReturn = 0;
const std::list<ErrorMessage::FileLocation> &locationList =
CTU::FileInfo::getErrorPath(CTU::FileInfo::InvalidValueType::null,
unsafeUsage,
callsMap,
"Dereferencing argument ARG that is null",
nullptr,
warning,
settings.maxCtuDepth);
settings.maxCtuDepth,
&unknownFunctionReturn);
if (locationList.empty())
continue;

std::string id = "ctunullpointer";
std::string message = "Null pointer dereference: " + unsafeUsage.myArgumentName;
if (unknownFunctionReturn == (std::uint8_t)ValueFlow::Value::UnknownFunctionReturn::outOfMemory) {
id += "OutOfMemory";
warning = 1;
message = "If memory allocation fails, then there is a possible null pointer dereference: " + unsafeUsage.myArgumentName;
} else if (unknownFunctionReturn == (std::uint8_t)ValueFlow::Value::UnknownFunctionReturn::outOfResources) {
id += "OutOfResources";
warning = 1;
message = "If resource allocation fails, then there is a possible null pointer dereference: " + unsafeUsage.myArgumentName;
}

const ErrorMessage errmsg(locationList,
fi->file0,
warning ? Severity::warning : Severity::error,
"Null pointer dereference: " + unsafeUsage.myArgumentName,
"ctunullpointer",
message,
id,
CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
errorLogger.reportErr(errmsg);

Expand Down
4 changes: 4 additions & 0 deletions lib/checkuninitvar.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1722,6 +1722,10 @@ namespace
};
}

static bool isVariableUsage(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *value) {
return isVariableUsage(settings, argtok, &value->value);
}

Check::FileInfo *CheckUninitVar::getFileInfo(const Tokenizer &tokenizer, const Settings &settings) const
{
const std::list<CTU::FileInfo::UnsafeUsage> &unsafeUsage = CTU::getUnsafeUsage(tokenizer, settings, ::isVariableUsage);
Expand Down
55 changes: 34 additions & 21 deletions lib/ctu.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ static constexpr char ATTR_CALL_ARGNR[] = "call-argnr";
static constexpr char ATTR_CALL_ARGEXPR[] = "call-argexpr";
static constexpr char ATTR_CALL_ARGVALUETYPE[] = "call-argvaluetype";
static constexpr char ATTR_CALL_ARGVALUE[] = "call-argvalue";
static constexpr char ATTR_CALL_UNKNOWN_FUNCTION_RETURN[] = "call-argvalue-ufr";
static constexpr char ATTR_WARNING[] = "warning";
static constexpr char ATTR_LOC_FILENAME[] = "file";
static constexpr char ATTR_LOC_LINENR[] = "line";
Expand All @@ -54,6 +55,7 @@ static constexpr char ATTR_MY_ID[] = "my-id";
static constexpr char ATTR_MY_ARGNR[] = "my-argnr";
static constexpr char ATTR_MY_ARGNAME[] = "my-argname";
static constexpr char ATTR_VALUE[] = "value";
static constexpr char ATTR_UNKNOWN_FUNCTION_RETURN[] = "ufr";

std::string CTU::getFunctionId(const Tokenizer &tokenizer, const Function *function)
{
Expand Down Expand Up @@ -102,7 +104,8 @@ std::string CTU::FileInfo::FunctionCall::toXmlString() const
<< toBaseXmlString()
<< " " << ATTR_CALL_ARGEXPR << "=\"" << ErrorLogger::toxml(callArgumentExpression) << "\""
<< " " << ATTR_CALL_ARGVALUETYPE << "=\"" << static_cast<int>(callValueType) << "\""
<< " " << ATTR_CALL_ARGVALUE << "=\"" << callArgValue << "\"";
<< " " << ATTR_CALL_ARGVALUE << "=\"" << callArgValue.value << "\""
<< " " << ATTR_CALL_UNKNOWN_FUNCTION_RETURN << "=\"" << (int)callArgValue.unknownFunctionReturn << "\"";
if (warning)
out << " " << ATTR_WARNING << "=\"true\"";
if (callValuePath.empty())
Expand Down Expand Up @@ -141,7 +144,8 @@ std::string CTU::FileInfo::UnsafeUsage::toString() const
<< " " << ATTR_LOC_FILENAME << "=\"" << ErrorLogger::toxml(location.fileName) << '\"'
<< " " << ATTR_LOC_LINENR << "=\"" << location.lineNumber << '\"'
<< " " << ATTR_LOC_COLUMN << "=\"" << location.column << '\"'
<< " " << ATTR_VALUE << "=\"" << value << "\""
<< " " << ATTR_VALUE << "=\"" << value.value << "\""
<< " " << ATTR_UNKNOWN_FUNCTION_RETURN << "=\"" << (int)value.unknownFunctionReturn << "\""
<< "/>\n";
return out.str();
}
Expand Down Expand Up @@ -201,7 +205,8 @@ bool CTU::FileInfo::FunctionCall::loadFromXml(const tinyxml2::XMLElement *xmlEle
bool error=false;
callArgumentExpression = readAttrString(xmlElement, ATTR_CALL_ARGEXPR, &error);
callValueType = (ValueFlow::Value::ValueType)readAttrInt(xmlElement, ATTR_CALL_ARGVALUETYPE, &error);
callArgValue = readAttrInt(xmlElement, ATTR_CALL_ARGVALUE, &error);
callArgValue.value = readAttrInt(xmlElement, ATTR_CALL_ARGVALUE, &error);
callArgValue.unknownFunctionReturn = readAttrInt(xmlElement, ATTR_CALL_UNKNOWN_FUNCTION_RETURN, &error);
const char *w = xmlElement->Attribute(ATTR_WARNING);
warning = w && std::strcmp(w, "true") == 0;
for (const tinyxml2::XMLElement *e2 = xmlElement->FirstChildElement(); !error && e2; e2 = e2->NextSiblingElement()) {
Expand Down Expand Up @@ -267,7 +272,8 @@ std::list<CTU::FileInfo::UnsafeUsage> CTU::loadUnsafeUsageListFromXml(const tiny
unsafeUsage.location.fileName = readAttrString(e, ATTR_LOC_FILENAME, &error);
unsafeUsage.location.lineNumber = readAttrInt(e, ATTR_LOC_LINENR, &error);
unsafeUsage.location.column = readAttrInt(e, ATTR_LOC_COLUMN, &error);
unsafeUsage.value = readAttrInt(e, ATTR_VALUE, &error);
unsafeUsage.value.value = readAttrInt(e, ATTR_VALUE, &error);
unsafeUsage.value.unknownFunctionReturn = readAttrInt(e, ATTR_UNKNOWN_FUNCTION_RETURN, &error);

if (!error)
ret.push_back(std::move(unsafeUsage));
Expand Down Expand Up @@ -342,7 +348,7 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer &tokenizer)
functionCall.location = FileInfo::Location(tokenizer,tok);
functionCall.callArgNr = argnr + 1;
functionCall.callArgumentExpression = argtok->expressionString();
functionCall.callArgValue = value.intvalue;
functionCall.callArgValue = value;
functionCall.warning = !value.errorSeverity();
for (const ErrorPathItem &i : value.errorPath) {
const std::string& file = tokenizer.list.file(i.first);
Expand All @@ -364,7 +370,7 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer &tokenizer)
functionCall.callArgNr = argnr + 1;
functionCall.callArgumentExpression = argtok->expressionString();
const auto typeSize = argtok->valueType()->typeSize(tokenizer.getSettings().platform);
functionCall.callArgValue = typeSize > 0 ? argtok->variable()->dimension(0) * typeSize : -1;
functionCall.callArgValue.value = typeSize > 0 ? argtok->variable()->dimension(0) * typeSize : -1;
functionCall.warning = false;
fileInfo->functionCalls.push_back(std::move(functionCall));
}
Expand All @@ -377,7 +383,7 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer &tokenizer)
functionCall.location = FileInfo::Location(tokenizer, tok);
functionCall.callArgNr = argnr + 1;
functionCall.callArgumentExpression = argtok->expressionString();
functionCall.callArgValue = argtok->astOperand1()->valueType()->typeSize(tokenizer.getSettings().platform);
functionCall.callArgValue.value = argtok->astOperand1()->valueType()->typeSize(tokenizer.getSettings().platform);
functionCall.warning = false;
fileInfo->functionCalls.push_back(std::move(functionCall));
}
Expand Down Expand Up @@ -411,7 +417,8 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer &tokenizer)
functionCall.callFunctionName = tok->astOperand1()->expressionString();
functionCall.location = FileInfo::Location(tokenizer, tok);
functionCall.callArgNr = argnr + 1;
functionCall.callArgValue = 0;
functionCall.callArgValue = v;
functionCall.callArgValue.value = 0;
functionCall.callArgumentExpression = argtok->expressionString();
functionCall.warning = false;
fileInfo->functionCalls.push_back(std::move(functionCall));
Expand All @@ -436,9 +443,9 @@ CTU::FileInfo *CTU::getFileInfo(const Tokenizer &tokenizer)
return fileInfo;
}

static std::list<std::pair<const Token *, MathLib::bigint>> getUnsafeFunction(const Settings &settings, const Scope *scope, int argnr, bool (*isUnsafeUsage)(const Settings &settings, const Token *argtok, MathLib::bigint *value))
static std::vector<std::pair<const Token *, CTU::FileInfo::Value>> getUnsafeFunction(const Settings &settings, const Scope *scope, int argnr, bool (*isUnsafeUsage)(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *value))
{
std::list<std::pair<const Token *, MathLib::bigint>> ret;
std::vector<std::pair<const Token *, CTU::FileInfo::Value>> ret;
const Variable * const argvar = scope->function->getArgumentVar(argnr);
if (!argvar->isArrayOrPointer() && !argvar->isReference())
return ret;
Expand All @@ -459,7 +466,7 @@ static std::list<std::pair<const Token *, MathLib::bigint>> getUnsafeFunction(co
}
if (tok2->variable() != argvar)
continue;
MathLib::bigint value = 0;
CTU::FileInfo::Value value;
if (!isUnsafeUsage(settings, tok2, &value))
return ret; // TODO: Is this a read? then continue..
ret.emplace_back(tok2, value);
Expand All @@ -468,7 +475,7 @@ static std::list<std::pair<const Token *, MathLib::bigint>> getUnsafeFunction(co
return ret;
}

std::list<CTU::FileInfo::UnsafeUsage> CTU::getUnsafeUsage(const Tokenizer &tokenizer, const Settings &settings, bool (*isUnsafeUsage)(const Settings &settings, const Token *argtok, MathLib::bigint *value))
std::list<CTU::FileInfo::UnsafeUsage> CTU::getUnsafeUsage(const Tokenizer &tokenizer, const Settings &settings, bool (*isUnsafeUsage)(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *value))
{
std::list<CTU::FileInfo::UnsafeUsage> unsafeUsage;

Expand All @@ -482,9 +489,9 @@ std::list<CTU::FileInfo::UnsafeUsage> CTU::getUnsafeUsage(const Tokenizer &token

// "Unsafe" functions unconditionally reads data before it is written..
for (int argnr = 0; argnr < function->argCount(); ++argnr) {
for (const std::pair<const Token *, MathLib::bigint> &v : getUnsafeFunction(settings, &scope, argnr, isUnsafeUsage)) {
for (const std::pair<const Token *, CTU::FileInfo::Value> &v : getUnsafeFunction(settings, &scope, argnr, isUnsafeUsage)) {
const Token *tok = v.first;
const MathLib::bigint val = v.second;
const CTU::FileInfo::Value val = v.second;
unsafeUsage.emplace_back(CTU::getFunctionId(tokenizer, function), argnr+1, tok->str(), CTU::FileInfo::Location(tokenizer,tok), val);
}
}
Expand Down Expand Up @@ -520,7 +527,7 @@ static bool findPath(const std::string &callId,
continue;
switch (invalidValue) {
case CTU::FileInfo::InvalidValueType::null:
if (functionCall->callValueType != ValueFlow::Value::ValueType::INT || functionCall->callArgValue != 0)
if (functionCall->callValueType != ValueFlow::Value::ValueType::INT || functionCall->callArgValue.value != 0)
continue;
break;
case CTU::FileInfo::InvalidValueType::uninit:
Expand All @@ -530,7 +537,7 @@ static bool findPath(const std::string &callId,
case CTU::FileInfo::InvalidValueType::bufferOverflow:
if (functionCall->callValueType != ValueFlow::Value::ValueType::BUFFER_SIZE)
continue;
if (unsafeValue < 0 || (unsafeValue >= functionCall->callArgValue && functionCall->callArgValue >= 0))
if (unsafeValue < 0 || (unsafeValue >= functionCall->callArgValue.value && functionCall->callArgValue.value >= 0))
break;
continue;
}
Expand All @@ -557,14 +564,20 @@ std::list<ErrorMessage::FileLocation> CTU::FileInfo::getErrorPath(InvalidValueTy
const char info[],
const FunctionCall ** const functionCallPtr,
bool warning,
int maxCtuDepth)
int maxCtuDepth,
std::uint8_t *unknownFunctionReturn)
{
std::list<ErrorMessage::FileLocation> locationList;

const CTU::FileInfo::CallBase *path[10] = {nullptr};

if (!findPath(unsafeUsage.myId, unsafeUsage.myArgNr, unsafeUsage.value, invalidValue, callsMap, path, 0, warning, maxCtuDepth))
return locationList;
if (!findPath(unsafeUsage.myId, unsafeUsage.myArgNr, unsafeUsage.value.value, invalidValue, callsMap, path, 0, warning, maxCtuDepth))
return {};

if (unknownFunctionReturn && path[0] && dynamic_cast<const CTU::FileInfo::FunctionCall *>(path[0])) {
const auto* v = dynamic_cast<const CTU::FileInfo::FunctionCall *>(path[0]);
*unknownFunctionReturn = v->callArgValue.unknownFunctionReturn;
}

std::list<ErrorMessage::FileLocation> locationList;

const std::string value1 = (invalidValue == InvalidValueType::null) ? "null" : "uninitialized";

Expand Down
Loading

0 comments on commit 74b0803

Please sign in to comment.