Skip to content

Commit 9305216

Browse files
chrchr-githubweb-flow
chrchr-github
and
web-flow
authored
Fix #901 Improve out-of-bounds check to detect error with sprintf() (#8473)
Co-authored-by: chrchr-github <noreply@github.com>
1 parent c3df665 commit 9305216

8 files changed

Lines changed: 51 additions & 13 deletions

File tree

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ $(libcppdir)/checkautovariables.o: lib/checkautovariables.cpp lib/addoninfo.h li
515515
$(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
516516
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbool.cpp
517517

518-
$(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
518+
$(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
519519
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbufferoverrun.cpp
520520

521521
$(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

‎lib/checkbufferoverrun.cpp‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "utils.h"
3737
#include "valueflow.h"
3838
#include "vfvalue.h"
39+
#include "vf_common.h"
3940

4041
#include <algorithm>
4142
#include <cstdlib>
@@ -83,7 +84,7 @@ static const Token* getRealBufferTok(const Token* tok) {
8384
return (op->valueType() && op->valueType()->pointer) ? op : tok;
8485
}
8586

86-
static int getMinFormatStringOutputLength(const std::vector<const Token*> &parameters, nonneg int formatStringArgNr)
87+
static int getMinFormatStringOutputLength(const std::vector<const Token*> &parameters, nonneg int formatStringArgNr, const Settings& settings)
8788
{
8889
if (formatStringArgNr <= 0 || formatStringArgNr > parameters.size())
8990
return 0;
@@ -138,8 +139,8 @@ static int getMinFormatStringOutputLength(const std::vector<const Token*> &param
138139
break;
139140
case 's':
140141
parameterLength = 0;
141-
if (inputArgNr < parameters.size() && parameters[inputArgNr]->tokType() == Token::eString)
142-
parameterLength = Token::getStrLength(parameters[inputArgNr]);
142+
if (inputArgNr < parameters.size())
143+
parameterLength = ValueFlow::valueFlowGetStrLength(parameters[inputArgNr], settings);
143144

144145
handleNextParameter = true;
145146
break;
@@ -602,7 +603,7 @@ static bool checkBufferSize(const Token *ftok, const Library::ArgumentChecks::Mi
602603
switch (minsize.type) {
603604
case Library::ArgumentChecks::MinSize::Type::STRLEN:
604605
if (settings.library.isargformatstr(ftok, minsize.arg)) {
605-
return getMinFormatStringOutputLength(args, minsize.arg) < bufferSize;
606+
return getMinFormatStringOutputLength(args, minsize.arg, settings) < bufferSize;
606607
} else if (arg) {
607608
const Token *strtoken = arg->getValueTokenMaxStrLength();
608609
if (strtoken)

‎lib/valueflow.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6801,17 +6801,17 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
68016801
} else if (tok->str() == "+=" && astIsContainer(tok->astOperand1())) {
68026802
const Token* containerTok = tok->astOperand1();
68036803
const Token* valueTok = tok->astOperand2();
6804-
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok);
6804+
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok, settings);
68056805
forwardMinimumContainerSize(size, tok, containerTok);
68066806

68076807
} else if (tok->str() == "=" && Token::simpleMatch(tok->astOperand2(), "+") && astIsContainerString(tok)) {
68086808
const Token* tok2 = tok->astOperand2();
68096809
MathLib::bigint size = 0;
68106810
while (Token::simpleMatch(tok2, "+") && tok2->astOperand2()) {
6811-
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2());
6811+
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2(), settings);
68126812
tok2 = tok2->astOperand1();
68136813
}
6814-
size += ValueFlow::valueFlowGetStrLength(tok2);
6814+
size += ValueFlow::valueFlowGetStrLength(tok2, settings);
68156815
forwardMinimumContainerSize(size, tok, tok->astOperand1());
68166816
}
68176817
}

‎lib/vf_analyzers.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1547,7 +1547,7 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
15471547
case Library::Container::Action::APPEND: {
15481548
std::vector<const Token*> args = getArguments(tok->astParent()->tokAt(2));
15491549
if (args.size() == 1) // TODO: handle overloads
1550-
n = ValueFlow::valueFlowGetStrLength(tok->astParent()->tokAt(3));
1550+
n = ValueFlow::valueFlowGetStrLength(tok->astParent()->tokAt(3), settings);
15511551
if (n == 0) // TODO: handle known empty append
15521552
val->setPossible();
15531553
break;

‎lib/vf_common.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,7 @@ namespace ValueFlow
389389
v.debugPath.emplace_back(tok, std::move(s));
390390
}
391391

392-
MathLib::bigint valueFlowGetStrLength(const Token* tok)
392+
MathLib::bigint valueFlowGetStrLength(const Token* tok, const Settings& settings)
393393
{
394394
if (tok->tokType() == Token::eString)
395395
return Token::getStrLength(tok);
@@ -399,8 +399,10 @@ namespace ValueFlow
399399
return v->intvalue;
400400
if (const Value* v = tok->getKnownValue(Value::ValueType::TOK)) {
401401
if (v->tokvalue != tok)
402-
return valueFlowGetStrLength(v->tokvalue);
402+
return valueFlowGetStrLength(v->tokvalue, settings);
403403
}
404+
if (const Token* cont = settings.library.getContainerFromYield(tok, Library::Container::Yield::BUFFER_NT))
405+
return valueFlowGetStrLength(cont, settings);
404406
return 0;
405407
}
406408
}

‎lib/vf_common.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ namespace ValueFlow
5353
const Token* tok,
5454
SourceLocation local = SourceLocation::current());
5555

56-
MathLib::bigint valueFlowGetStrLength(const Token* tok);
56+
MathLib::bigint valueFlowGetStrLength(const Token* tok, const Settings& settings);
5757
}
5858

5959
#endif // vfCommonH

‎oss-fuzz/Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ $(libcppdir)/checkautovariables.o: ../lib/checkautovariables.cpp ../lib/addoninf
185185
$(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
186186
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbool.cpp
187187

188-
$(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
188+
$(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
189189
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkbufferoverrun.cpp
190190

191191
$(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

‎test/testbufferoverrun.cpp‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4620,6 +4620,41 @@ class TestBufferOverrun : public TestFixture {
46204620
" mysprintf(a, \"abcd\");\n"
46214621
"}", settings);
46224622
ASSERT_EQUALS("", errout_str());
4623+
4624+
check("void f() {\n" // #901
4625+
" const char b[] = \"b\";\n"
4626+
" char a[1];\n"
4627+
" sprintf(a, \"%s\", b);\n"
4628+
"}\n"
4629+
"void g() {\n"
4630+
" const char* b = \"b\";\n"
4631+
" char a[1];\n"
4632+
" sprintf(a, \"%s\", b);\n"
4633+
"}\n"
4634+
"void h() {\n"
4635+
" const std::string b = \"b\";\n"
4636+
" char a[1];\n"
4637+
" sprintf(a, \"%s\", b.c_str());\n"
4638+
"}\n"
4639+
"void i() {\n"
4640+
" const char b[] = \"b\";\n"
4641+
" char a[2];\n"
4642+
" sprintf(a, \"%s\", b);\n"
4643+
"}\n"
4644+
"void j() {\n"
4645+
" const char* b = \"b\";\n"
4646+
" char a[2];\n"
4647+
" sprintf(a, \"%s\", b);\n"
4648+
"}\n"
4649+
"void k() {\n"
4650+
" const std::string b = \"b\";\n"
4651+
" char a[2];\n"
4652+
" sprintf(a, \"%s\", b.c_str());\n"
4653+
"}\n", settings0);
4654+
ASSERT_EQUALS("[test.cpp:4:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
4655+
"[test.cpp:9:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
4656+
"[test.cpp:14:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n",
4657+
errout_str());
46234658
}
46244659

46254660
void minsize_mul() {

0 commit comments

Comments
 (0)