Skip to content

Commit 18d5f15

Browse files
web-flowweb-flow
committed
Fix #11765 FN: minsize not checked for string literal, buffer access out of bounds not found - #8944 
1 parent b2d8361 commit 18d5f15

10 files changed

Lines changed: 106 additions & 9 deletions

File tree

‎cfg/gtk.cfg‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5386,11 +5386,13 @@
53865386
<arg nr="1" direction="in">
53875387
<not-null/>
53885388
<not-uninit/>
5389+
<strz/>
53895390
<minsize type="argvalue" arg="3"/>
53905391
</arg>
53915392
<arg nr="2" direction="in">
53925393
<not-null/>
53935394
<not-uninit/>
5395+
<strz/>
53945396
<minsize type="argvalue" arg="3"/>
53955397
</arg>
53965398
<arg nr="3" direction="in">

‎cfg/posix.cfg‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2123,11 +2123,13 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
21232123
<arg nr="1" direction="in">
21242124
<not-null/>
21252125
<not-uninit/>
2126+
<strz/>
21262127
<minsize type="argvalue" arg="3"/>
21272128
</arg>
21282129
<arg nr="2" direction="in">
21292130
<not-null/>
21302131
<not-uninit/>
2132+
<strz/>
21312133
<minsize type="argvalue" arg="3"/>
21322134
</arg>
21332135
<arg nr="3" direction="in">
@@ -2146,11 +2148,13 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
21462148
<arg nr="1" direction="in">
21472149
<not-null/>
21482150
<not-uninit/>
2151+
<strz/>
21492152
<minsize type="argvalue" arg="3"/>
21502153
</arg>
21512154
<arg nr="2" direction="in">
21522155
<not-null/>
21532156
<not-uninit/>
2157+
<strz/>
21542158
<minsize type="argvalue" arg="3"/>
21552159
</arg>
21562160
<arg nr="3" direction="in">
@@ -5606,6 +5610,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
56065610
<arg nr="1" direction="in">
56075611
<not-null/>
56085612
<not-uninit/>
5613+
<strz/>
56095614
<minsize type="argvalue" arg="2"/>
56105615
</arg>
56115616
<arg nr="2" direction="in">
@@ -5782,6 +5787,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
57825787
<arg nr="2" direction="in">
57835788
<not-null/>
57845789
<not-uninit/>
5790+
<strz/>
57855791
<minsize type="argvalue" arg="3"/>
57865792
</arg>
57875793
<arg nr="3" direction="in">

‎cfg/std.cfg‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5233,7 +5233,6 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
52335233
<not-null/>
52345234
<not-uninit/>
52355235
<strz/>
5236-
<minsize type="argvalue" arg="3"/>
52375236
</arg>
52385237
<arg nr="3" direction="in">
52395238
<not-uninit/>

‎cfg/windows.cfg‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4293,11 +4293,13 @@ HFONT CreateFont(
42934293
<arg nr="1" direction="in">
42944294
<not-null/>
42954295
<not-uninit/>
4296+
<strz/>
42964297
<minsize type="argvalue" arg="3"/>
42974298
</arg>
42984299
<arg nr="2" direction="in">
42994300
<not-null/>
43004301
<not-uninit/>
4302+
<strz/>
43014303
<minsize type="argvalue" arg="3"/>
43024304
</arg>
43034305
<arg nr="3" direction="in">
@@ -4317,11 +4319,13 @@ HFONT CreateFont(
43174319
<arg nr="1" direction="in">
43184320
<not-null/>
43194321
<not-uninit/>
4322+
<strz/>
43204323
<minsize type="argvalue" arg="3"/>
43214324
</arg>
43224325
<arg nr="2" direction="in">
43234326
<not-null/>
43244327
<not-uninit/>
4328+
<strz/>
43254329
<minsize type="argvalue" arg="3"/>
43264330
</arg>
43274331
<arg nr="3" direction="in">

‎lib/checkbufferoverrun.cpp‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -558,7 +558,7 @@ void CheckBufferOverrunImpl::pointerArithmeticError(const Token *tok, const Toke
558558

559559
//---------------------------------------------------------------------------
560560

561-
ValueFlow::Value CheckBufferOverrunImpl::getBufferSize(const Token *bufTok, const Settings& settings) const
561+
ValueFlow::Value CheckBufferOverrunImpl::getBufferSize(const Token *bufTok, const Settings& settings, bool checkStringLiteral) const
562562
{
563563
if (!bufTok->valueType())
564564
return ValueFlow::Value(-1);
@@ -604,6 +604,15 @@ ValueFlow::Value CheckBufferOverrunImpl::getBufferSize(const Token *bufTok, cons
604604
return bufSizeVal;
605605
}
606606
}
607+
} else if (checkStringLiteral && bufTok->tokType() == Token::Type::eString && bufTok->valueType()) {
608+
ValueFlow::Value strSizeVal;
609+
strSizeVal.setKnown();
610+
strSizeVal.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE;
611+
const size_t charSize = bufTok->valueType()->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointee);
612+
if (charSize > 0) {
613+
strSizeVal.intvalue = (Token::getStrArraySize(bufTok) - index) * charSize;
614+
return strSizeVal;
615+
}
607616
}
608617
}
609618

@@ -715,7 +724,8 @@ void CheckBufferOverrunImpl::bufferOverflow()
715724
if (argtok->valueType() && argtok->valueType()->pointer == 0)
716725
continue;
717726
// TODO: strcpy(buf+10, "hello");
718-
ValueFlow::Value bufferSize = getBufferSize(argtok, mSettings);
727+
const bool strz = mSettings.library.isargstrz(tok, argnr + 1);
728+
ValueFlow::Value bufferSize = getBufferSize(argtok, mSettings, !strz);
719729
if (bufferSize.intvalue <= 0)
720730
continue;
721731
// buffer size == 1 => do not warn for dynamic memory

‎lib/checkbufferoverrun.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ class CPPCHECKLIB CheckBufferOverrunImpl : public CheckImpl
129129
void objectIndex();
130130
void objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known);
131131

132-
ValueFlow::Value getBufferSize(const Token *bufTok, const Settings& settings) const;
132+
ValueFlow::Value getBufferSize(const Token *bufTok, const Settings& settings, bool checkStringLiteral = false) const;
133133

134134
// CTU
135135
static bool isCtuUnsafeBufferUsage(const Settings &settings, const Token *argtok, CTU::FileInfo::Value *offset, int type);

‎lib/checkfunctions.cpp‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,18 @@ void CheckFunctionsImpl::functionCalledError(const Token* tok, Severity severity
102102
reportError(tok, severity, prefix + "Called", msg, CWE477, Certainty::normal);
103103
}
104104

105+
static bool hasArgMinReadSize(const Token *ftok, int argnr, const Library &library)
106+
{
107+
if (library.getArgDirection(ftok, argnr) == Library::ArgumentChecks::Direction::DIR_IN) {
108+
if (const std::vector<Library::ArgumentChecks::MinSize> *minsizes = library.argminsizes(ftok, argnr)) {
109+
return std::any_of(minsizes->begin(), minsizes->end(), [](const Library::ArgumentChecks::MinSize &ms) {
110+
return ms.type == Library::ArgumentChecks::MinSize::Type::ARGVALUE;
111+
});
112+
}
113+
}
114+
return false;
115+
}
116+
105117
//---------------------------------------------------------------------------
106118
// Check <valid>, <strz> and <not-bool>
107119
//---------------------------------------------------------------------------
@@ -137,7 +149,7 @@ void CheckFunctionsImpl::invalidFunctionUsage()
137149
invalidFunctionArgError(argtok, functionToken->str(), argnr, nullptr, mSettings.library.validarg(functionToken, argnr));
138150
}
139151
// check <strz>
140-
if (mSettings.library.isargstrz(functionToken, argnr)) {
152+
if (mSettings.library.isargstrz(functionToken, argnr) && !hasArgMinReadSize(functionToken, argnr, mSettings.library)) {
141153
if (Token::Match(argtok, "& %var% !![") && argtok->next() && argtok->next()->valueType()) {
142154
const ValueType * valueType = argtok->next()->valueType();
143155
const Variable * variable = argtok->next()->variable();

‎test/cfg/posix.c‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,12 @@ void invalidFunctionArgStr_mbsnrtowcs(void)
194194
if (mbsnrtowcs (wenough, &cp, 1, 10, &s) != 1 || wcscmp (wenough, L"AEF") != 0) {}
195195
}
196196

197+
int invalidFunctionArgStr_strncasecmp(const char* s)
198+
{
199+
const char buf[3] = {'a', 'b', 'c'};
200+
return strncasecmp(buf, s, 3);
201+
}
202+
197203
struct tm * ignoredReturnValue_localtime(const time_t *tp)
198204
{
199205
// cppcheck-suppress [ignoredReturnValue,localtimeCalled]
@@ -719,6 +725,13 @@ size_t bufferAccessOutOfBounds_strnlen(const char *s, size_t maxlen)
719725
size_t len = strnlen(buf,2);
720726
// cppcheck-suppress bufferAccessOutOfBounds
721727
len+=strnlen(buf,3);
728+
len+=strnlen("ab",5);
729+
return len;
730+
}
731+
732+
size_t bufferAccessOutOfBounds_strncasecmp(const char *s)
733+
{
734+
size_t len = strncasecmp(s, "abc", 5);
722735
return len;
723736
}
724737

@@ -1606,4 +1619,4 @@ void pthread_define_test()
16061619
(void)rwlock;
16071620
PTHREAD_SCOPE_PROCESS;
16081621
PTHREAD_SCOPE_SYSTEM;
1609-
}
1622+
}

‎test/cfg/std.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ void invalidFunctionArgStr_strncat(void)
7878
// No warning is expected for
7979
strncat (str1, &src, 1);
8080
puts (str1);
81+
char d[] = {'a','b','c'};
82+
// cppcheck-suppress invalidFunctionArgStr
83+
strncat(d, "def", 1);
8184
}
8285

8386
char * invalidFunctionArgStr_strpbrk( const char *p )
@@ -4913,6 +4916,10 @@ void invalidFunctionArgString(char c)
49134916
char d='d';
49144917
/* cppcheck-suppress invalidFunctionArgStr */
49154918
(void)strcat(str,&d);
4919+
4920+
char a[] = { 'a', 'b', 'c' };
4921+
/* cppcheck-suppress [bufferAccessOutOfBounds,invalidFunctionArgStr] */
4922+
(void)strcat(a, "def");
49164923
}
49174924

49184925
void ignoredReturnValue_abs(int i)

‎test/testbufferoverrun.cpp‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,7 @@ class TestBufferOverrun : public TestFixture {
229229
TEST_CASE(buffer_overrun_36);
230230
TEST_CASE(buffer_overrun_37);
231231
TEST_CASE(buffer_overrun_38);
232+
TEST_CASE(buffer_overrun_39);
232233
TEST_CASE(buffer_overrun_errorpath);
233234
TEST_CASE(buffer_overrun_bailoutIfSwitch); // ticket #2378 : bailoutIfSwitch
234235
TEST_CASE(buffer_overrun_function_array_argument);
@@ -3638,6 +3639,50 @@ class TestBufferOverrun : public TestFixture {
36383639
ASSERT_EQUALS("", errout_str());
36393640
}
36403641

3642+
void buffer_overrun_39() { // #11765
3643+
check("void f() {\n"
3644+
" char buf[10];\n"
3645+
" memcpy(buf, \"abc\", 4);\n"
3646+
"}\n");
3647+
ASSERT_EQUALS("", errout_str());
3648+
3649+
check("void f() {\n"
3650+
" char buf[128];\n"
3651+
" memcpy(buf, \"abc\", 5);\n"
3652+
"}\n");
3653+
ASSERT_EQUALS("[test.cpp:3:17]: (error) Buffer is accessed out of bounds: \"abc\" [bufferAccessOutOfBounds]\n",
3654+
errout_str());
3655+
3656+
check("void f() {\n"
3657+
" char buf[128];\n"
3658+
" memcpy(buf, L\"abc\", 4 * sizeof(wchar_t));\n"
3659+
"}\n");
3660+
ASSERT_EQUALS("", errout_str());
3661+
3662+
check("void f() {\n"
3663+
" char buf[128];\n"
3664+
" memcpy(buf, L\"abc\", 4 * sizeof(wchar_t) + 1);\n"
3665+
"}\n");
3666+
ASSERT_EQUALS("[test.cpp:3:17]: (error) Buffer is accessed out of bounds: L\"abc\" [bufferAccessOutOfBounds]\n",
3667+
errout_str());
3668+
3669+
check("int f(char* s) {\n"
3670+
" return memcmp(s, \"\\0\\0\\0\", 3);\n"
3671+
"}\n");
3672+
ASSERT_EQUALS("", errout_str());
3673+
3674+
check("int f(const char16_t* buf) {\n"
3675+
" return memcmp(buf, u\"abc\", 4 * sizeof(char16_t));\n"
3676+
"}\n");
3677+
ASSERT_EQUALS("", errout_str());
3678+
3679+
check("int f(const char* buf) {\n"
3680+
" return memcmp(buf, &\"abc\"[2], 4 * sizeof(char));\n"
3681+
"}\n");
3682+
ASSERT_EQUALS("[test.cpp:2:24]: (error) Buffer is accessed out of bounds: &\"abc\"[2] [bufferAccessOutOfBounds]\n",
3683+
errout_str());
3684+
}
3685+
36413686
void buffer_overrun_errorpath() {
36423687
setMultiline();
36433688
Settings s = settings0;
@@ -4618,9 +4663,8 @@ class TestBufferOverrun : public TestFixture {
46184663
check("void f() {\n"
46194664
" mymemset(\"abc\", 0, 20);\n"
46204665
"}\n", settings);
4621-
TODO_ASSERT_EQUALS("[test.cpp:2]: (error) Buffer is accessed out of bounds.\n",
4622-
"",
4623-
errout_str());
4666+
ASSERT_EQUALS("[test.cpp:2:12]: (error) Buffer is accessed out of bounds: \"abc\" [bufferAccessOutOfBounds]\n",
4667+
errout_str());
46244668

46254669
check("void f() {\n"
46264670
" mymemset(temp, \"abc\", 4);\n"

0 commit comments

Comments
 (0)