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 2 commits 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
2 commits
Select commit Hold shift + click to select a range
473c3fd
Fix #901 Improve out-of-bounds check to detect error with sprintf()
web-flow Apr 21, 2026
6a29bf8
Update testbufferoverrun.cpp
chrchr-github Apr 27, 2026
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 @@ static const Token* getRealBufferTok(const Token* tok) {
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 @@ static int getMinFormatStringOutputLength(const std::vector<const Token*> &param
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);
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
chrchr-github marked this conversation as resolved.

handleNextParameter = true;
break;
Expand Down Expand Up @@ -602,7 +603,7 @@ static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::Mi
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 @@ -6803,17 +6803,17 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
} 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)
Copy link
Copy Markdown
Collaborator

@firewave firewave Apr 27, 2026

Choose a reason for hiding this comment

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

We could just pass Library instead of Settings.

Copy link
Copy Markdown
Collaborator Author

@chrchr-github chrchr-github Apr 27, 2026

Choose a reason for hiding this comment

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

Yeah, but we are doing that all over the place. And in the end, it just means slightly more code at the call site, slightly less code in the called function.

Copy link
Copy Markdown
Collaborator

@firewave firewave Apr 27, 2026

Choose a reason for hiding this comment

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

It's actually not that bad - see #8500 (I actually made those changes a while before I spotted it in this PR).

{
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
35 changes: 35 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,41 @@ class TestBufferOverrun : public TestFixture {
" mysprintf(a, \"abcd\");\n"
"}", settings);
ASSERT_EQUALS("", errout_str());

check("void f() {\n" // #901
Comment thread
chrchr-github marked this conversation as resolved.
" 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"
"}\n"
"void i() {\n"
" const char b[] = \"b\";\n"
" char a[2];\n"
" sprintf(a, \"%s\", b);\n"
"}\n"
"void j() {\n"
" const char* b = \"b\";\n"
" char a[2];\n"
" sprintf(a, \"%s\", b);\n"
"}\n"
"void k() {\n"
" const std::string b = \"b\";\n"
" char a[2];\n"
" sprintf(a, \"%s\", b.c_str());\n"
"}\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