Skip to content

Fix #901 Improve out-of-bounds check to detect error with sprintf() #8473

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

Open
chrchr-github wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #901 Improve out-of-bounds check to detect error with sprintf() #8473

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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ $(libcppdir)/checkautovariables.o: lib/checkautovariables.cpp lib/addoninfo.h li
$(libcppdir)/checkbool.o: lib/checkbool.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkbool.h lib/checkers.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/regex.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbool.cpp

$(libcppdir)/checkbufferoverrun.o: lib/checkbufferoverrun.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkbufferoverrun.h lib/checkers.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/regex.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h lib/xml.h
$(libcppdir)/checkbufferoverrun.o: lib/checkbufferoverrun.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkbufferoverrun.h lib/checkers.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/regex.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vf_common.h lib/vfvalue.h lib/xml.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbufferoverrun.cpp

$(libcppdir)/checkclass.o: lib/checkclass.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/checkclass.h lib/checkers.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/regex.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h lib/xml.h
Expand Down
9 changes: 5 additions & 4 deletions lib/checkbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "utils.h"
#include "valueflow.h"
#include "vfvalue.h"
#include "vf_common.h"

#include <algorithm>
#include <cstdlib>
Expand Down Expand Up @@ -83,7 +84,7 @@
return (op->valueType() && op->valueType()->pointer) ? op : tok;
}

static int getMinFormatStringOutputLength(const std::vector<const Token*> &parameters, nonneg int formatStringArgNr)
static int getMinFormatStringOutputLength(const std::vector<const Token*> &parameters, nonneg int formatStringArgNr, const Settings& settings)
{
if (formatStringArgNr <= 0 || formatStringArgNr > parameters.size())
return 0;
Expand Down Expand Up @@ -138,8 +139,8 @@
break;
case 's':
parameterLength = 0;
if (inputArgNr < parameters.size() && parameters[inputArgNr]->tokType() == Token::eString)
parameterLength = Token::getStrLength(parameters[inputArgNr]);
if (inputArgNr < parameters.size())
parameterLength = ValueFlow::valueFlowGetStrLength(parameters[inputArgNr], settings);

Check failure

Code scanning / Cppcheck Premium

Guarantee that container indices and iterators are within the valid range Critical

Guarantee that container indices and iterators are within the valid range
Comment thread
chrchr-github marked this conversation as resolved.
Dismissed
Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Apr 21, 2026

Choose a reason for hiding this comment

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

Looks like a false positive.

Copy link
Copy Markdown
Collaborator

@danmar danmar Apr 25, 2026

Choose a reason for hiding this comment

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

I agree. And it might be fixed in latest cppcheck premium I fail to reproduce..


handleNextParameter = true;
break;
Expand Down Expand Up @@ -602,7 +603,7 @@
switch (minsize.type) {
case Library::ArgumentChecks::MinSize::Type::STRLEN:
if (settings.library.isargformatstr(ftok, minsize.arg)) {
return getMinFormatStringOutputLength(args, minsize.arg) < bufferSize;
return getMinFormatStringOutputLength(args, minsize.arg, settings) < bufferSize;
} else if (arg) {
const Token *strtoken = arg->getValueTokenMaxStrLength();
if (strtoken)
Expand Down
6 changes: 3 additions & 3 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1843,9 +1843,9 @@
const bool isCpp = (src && src->isCpp()) || (dst && dst->isCpp());
if (isNotEqual(decl, parentdecl) && !(isCpp && (Token::simpleMatch(decl.first, "auto") || Token::simpleMatch(parentdecl.first, "auto"))))
return true;
if (isNotEqual(decl, dst->valueType(), isCpp, settings))

Check failure on line 1846 in lib/valueflow.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Called C++ object pointer is null 


See more on https://sonarcloud.io/project/issues?id=danmar_cppcheck&issues=AZ2xOEHPEKMbOcCSm09r&open=AZ2xOEHPEKMbOcCSm09r&pullRequest=8473
return true;
if (isNotEqual(parentdecl, src->valueType(), isCpp, settings))

Check failure on line 1848 in lib/valueflow.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Called C++ object pointer is null 


See more on https://sonarcloud.io/project/issues?id=danmar_cppcheck&issues=AZ2xOEHPEKMbOcCSm09s&open=AZ2xOEHPEKMbOcCSm09s&pullRequest=8473
return true;
}
return false;
Expand Down Expand Up @@ -6803,17 +6803,17 @@
} else if (tok->str() == "+=" && astIsContainer(tok->astOperand1())) {
const Token* containerTok = tok->astOperand1();
const Token* valueTok = tok->astOperand2();
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok);
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok, settings);
forwardMinimumContainerSize(size, tok, containerTok);

} else if (tok->str() == "=" && Token::simpleMatch(tok->astOperand2(), "+") && astIsContainerString(tok)) {
const Token* tok2 = tok->astOperand2();
MathLib::bigint size = 0;
while (Token::simpleMatch(tok2, "+") && tok2->astOperand2()) {
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2());
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2(), settings);
tok2 = tok2->astOperand1();
}
size += ValueFlow::valueFlowGetStrLength(tok2);
size += ValueFlow::valueFlowGetStrLength(tok2, settings);
forwardMinimumContainerSize(size, tok, tok->astOperand1());
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/vf_analyzers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1547,7 +1547,7 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
case Library::Container::Action::APPEND: {
std::vector<const Token*> args = getArguments(tok->astParent()->tokAt(2));
if (args.size() == 1) // TODO: handle overloads
n = ValueFlow::valueFlowGetStrLength(tok->astParent()->tokAt(3));
n = ValueFlow::valueFlowGetStrLength(tok->astParent()->tokAt(3), settings);
if (n == 0) // TODO: handle known empty append
val->setPossible();
break;
Expand Down
6 changes: 4 additions & 2 deletions lib/vf_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ namespace ValueFlow
v.debugPath.emplace_back(tok, std::move(s));
}

MathLib::bigint valueFlowGetStrLength(const Token* tok)
MathLib::bigint valueFlowGetStrLength(const Token* tok, const Settings& settings)
{
if (tok->tokType() == Token::eString)
return Token::getStrLength(tok);
Expand All @@ -394,8 +394,10 @@ namespace ValueFlow
return v->intvalue;
if (const Value* v = tok->getKnownValue(Value::ValueType::TOK)) {
if (v->tokvalue != tok)
return valueFlowGetStrLength(v->tokvalue);
return valueFlowGetStrLength(v->tokvalue, settings);
}
if (const Token* cont = settings.library.getContainerFromYield(tok, Library::Container::Yield::BUFFER_NT))
return valueFlowGetStrLength(cont, settings);
return 0;
}
}
2 changes: 1 addition & 1 deletion lib/vf_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace ValueFlow
const Token* tok,
SourceLocation local = SourceLocation::current());

MathLib::bigint valueFlowGetStrLength(const Token* tok);
MathLib::bigint valueFlowGetStrLength(const Token* tok, const Settings& settings);
}

#endif // vfCommonH
2 changes: 1 addition & 1 deletion oss-fuzz/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ $(libcppdir)/checkautovariables.o: ../lib/checkautovariables.cpp ../lib/addoninf
$(libcppdir)/checkbool.o: ../lib/checkbool.cpp ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkbool.h ../lib/checkers.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/regex.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbool.cpp

$(libcppdir)/checkbufferoverrun.o: ../lib/checkbufferoverrun.cpp ../externals/tinyxml2/tinyxml2.h ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkbufferoverrun.h ../lib/checkers.h ../lib/config.h ../lib/ctu.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/path.h ../lib/platform.h ../lib/regex.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h ../lib/xml.h
$(libcppdir)/checkbufferoverrun.o: ../lib/checkbufferoverrun.cpp ../externals/tinyxml2/tinyxml2.h ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkbufferoverrun.h ../lib/checkers.h ../lib/config.h ../lib/ctu.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/path.h ../lib/platform.h ../lib/regex.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vf_common.h ../lib/vfvalue.h ../lib/xml.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbufferoverrun.cpp

$(libcppdir)/checkclass.o: ../lib/checkclass.cpp ../externals/tinyxml2/tinyxml2.h ../lib/addoninfo.h ../lib/astutils.h ../lib/check.h ../lib/checkclass.h ../lib/checkers.h ../lib/config.h ../lib/errorlogger.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/path.h ../lib/platform.h ../lib/regex.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h ../lib/xml.h
Expand Down
20 changes: 20 additions & 0 deletions test/testbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4610,6 +4610,26 @@ class TestBufferOverrun : public TestFixture {
" mysprintf(a, \"abcd\");\n"
"}", settings);
ASSERT_EQUALS("", errout_str());

check("void f() {\n" // #901
Copy link
Copy Markdown
Collaborator

@danmar danmar Apr 25, 2026

Choose a reason for hiding this comment

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

Are there some related test to ensure there is not false positives. I.e. if a size is 2 instead.

" const char b[] = \"b\";\n"
" char a[1];\n"
" sprintf(a, \"%s\", b);\n"
"}\n"
"void g() {\n"
" const char* b = \"b\";\n"
" char a[1];\n"
" sprintf(a, \"%s\", b);\n"
"}\n"
"void h() {\n"
" const std::string b = \"b\";\n"
" char a[1];\n"
" sprintf(a, \"%s\", b.c_str());\n"
"}", settings0);
ASSERT_EQUALS("[test.cpp:4:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
"[test.cpp:9:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
"[test.cpp:14:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n",
errout_str());
}

void minsize_mul() {
Expand Down
Loading