Skip to content

Commit 473c3fd

Browse files
web-flowweb-flow
committed
Fix #901 Improve out-of-bounds check to detect error with sprintf()
1 parent 1721485 commit 473c3fd

8 files changed

Lines changed: 36 additions & 13 deletions

File tree

‎Makefile‎

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

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

515515
$(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
@@ -6803,17 +6803,17 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
68036803
} else if (tok->str() == "+=" && astIsContainer(tok->astOperand1())) {
68046804
const Token* containerTok = tok->astOperand1();
68056805
const Token* valueTok = tok->astOperand2();
6806-
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok);
6806+
const MathLib::bigint size = ValueFlow::valueFlowGetStrLength(valueTok, settings);
68076807
forwardMinimumContainerSize(size, tok, containerTok);
68086808

68096809
} else if (tok->str() == "=" && Token::simpleMatch(tok->astOperand2(), "+") && astIsContainerString(tok)) {
68106810
const Token* tok2 = tok->astOperand2();
68116811
MathLib::bigint size = 0;
68126812
while (Token::simpleMatch(tok2, "+") && tok2->astOperand2()) {
6813-
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2());
6813+
size += ValueFlow::valueFlowGetStrLength(tok2->astOperand2(), settings);
68146814
tok2 = tok2->astOperand1();
68156815
}
6816-
size += ValueFlow::valueFlowGetStrLength(tok2);
6816+
size += ValueFlow::valueFlowGetStrLength(tok2, settings);
68176817
forwardMinimumContainerSize(size, tok, tok->astOperand1());
68186818
}
68196819
}

‎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
@@ -384,7 +384,7 @@ namespace ValueFlow
384384
v.debugPath.emplace_back(tok, std::move(s));
385385
}
386386

387-
MathLib::bigint valueFlowGetStrLength(const Token* tok)
387+
MathLib::bigint valueFlowGetStrLength(const Token* tok, const Settings& settings)
388388
{
389389
if (tok->tokType() == Token::eString)
390390
return Token::getStrLength(tok);
@@ -394,8 +394,10 @@ namespace ValueFlow
394394
return v->intvalue;
395395
if (const Value* v = tok->getKnownValue(Value::ValueType::TOK)) {
396396
if (v->tokvalue != tok)
397-
return valueFlowGetStrLength(v->tokvalue);
397+
return valueFlowGetStrLength(v->tokvalue, settings);
398398
}
399+
if (const Token* cont = settings.library.getContainerFromYield(tok, Library::Container::Yield::BUFFER_NT))
400+
return valueFlowGetStrLength(cont, settings);
399401
return 0;
400402
}
401403
}

‎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: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4610,6 +4610,26 @@ class TestBufferOverrun : public TestFixture {
46104610
" mysprintf(a, \"abcd\");\n"
46114611
"}", settings);
46124612
ASSERT_EQUALS("", errout_str());
4613+
4614+
check("void f() {\n" // #901
4615+
" const char b[] = \"b\";\n"
4616+
" char a[1];\n"
4617+
" sprintf(a, \"%s\", b);\n"
4618+
"}\n"
4619+
"void g() {\n"
4620+
" const char* b = \"b\";\n"
4621+
" char a[1];\n"
4622+
" sprintf(a, \"%s\", b);\n"
4623+
"}\n"
4624+
"void h() {\n"
4625+
" const std::string b = \"b\";\n"
4626+
" char a[1];\n"
4627+
" sprintf(a, \"%s\", b.c_str());\n"
4628+
"}", settings0);
4629+
ASSERT_EQUALS("[test.cpp:4:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
4630+
"[test.cpp:9:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n"
4631+
"[test.cpp:14:13]: (error) Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]\n",
4632+
errout_str());
46134633
}
46144634

46154635
void minsize_mul() {

0 commit comments

Comments
 (0)