Skip to content

Commit e5f9f00

Browse files
chrchr-githubweb-flow
chrchr-github
and
web-flow
authored
Fix #11765 FN: minsize not checked for string literal, buffer access out of bounds not found (#8944)
Co-authored-by: chrchr-github <noreply@github.com>
1 parent b8b27a9 commit e5f9f00

10 files changed

Lines changed: 108 additions & 18 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 & 1 deletion
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">
@@ -4200,7 +4204,6 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
42004204
<not-null/>
42014205
<not-uninit/>
42024206
<strz/>
4203-
<minsize type="argvalue" arg="3"/>
42044207
</arg>
42054208
<arg nr="3" direction="in">
42064209
<not-uninit/>
@@ -5605,6 +5608,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
56055608
<arg nr="1" direction="in">
56065609
<not-null/>
56075610
<not-uninit/>
5611+
<strz/>
56085612
<minsize type="argvalue" arg="2"/>
56095613
</arg>
56105614
<arg nr="2" direction="in">
@@ -5781,6 +5785,7 @@ The function 'mktemp' is considered to be dangerous due to race conditions and s
57815785
<arg nr="2" direction="in">
57825786
<not-null/>
57835787
<not-uninit/>
5788+
<strz/>
57845789
<minsize type="argvalue" arg="3"/>
57855790
</arg>
57865791
<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: 6 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">
@@ -4584,6 +4588,7 @@ HFONT CreateFont(
45844588
<arg nr="3" direction="in">
45854589
<not-null/>
45864590
<not-uninit/>
4591+
<strz/>
45874592
<minsize type="argvalue" arg="4"/>
45884593
</arg>
45894594
<arg nr="4" direction="in">
@@ -4632,6 +4637,7 @@ HFONT CreateFont(
46324637
<arg nr="3" direction="in">
46334638
<not-null/>
46344639
<not-uninit/>
4640+
<strz/>
46354641
<minsize type="argvalue" arg="4"/>
46364642
</arg>
46374643
<arg nr="4" 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+
const size_t charSize = bufTok->valueType()->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointee);
609+
if (charSize > 0) {
610+
ValueFlow::Value strSizeVal;
611+
strSizeVal.setKnown();
612+
strSizeVal.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE;
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: 13 additions & 0 deletions
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

‎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: 48 additions & 12 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);
@@ -3492,15 +3493,7 @@ class TestBufferOverrun : public TestFixture {
34923493
" (void)strxfrm(dest,src,2);\n"
34933494
" (void)strxfrm(dest,src,3);\n" // <<
34943495
"}\n");
3495-
ASSERT_EQUALS("[test.cpp:6:24]: (error) Buffer is accessed out of bounds: src [bufferAccessOutOfBounds]\n", errout_str());
3496-
// source size is too small
3497-
check("void f(void) {\n"
3498-
" const char src[1] = \"a\";\n"
3499-
" char dest[3] = \"abc\";\n"
3500-
" (void)strxfrm(dest,src,1);\n"
3501-
" (void)strxfrm(dest,src,2);\n" // <<
3502-
"}\n");
3503-
ASSERT_EQUALS("[test.cpp:5:24]: (error) Buffer is accessed out of bounds: src [bufferAccessOutOfBounds]\n", errout_str());
3496+
ASSERT_EQUALS("", errout_str());
35043497
}
35053498

35063499
void buffer_overrun_33() { // #2019
@@ -3638,6 +3631,50 @@ class TestBufferOverrun : public TestFixture {
36383631
ASSERT_EQUALS("", errout_str());
36393632
}
36403633

3634+
void buffer_overrun_39() { // #11765
3635+
check("void f() {\n"
3636+
" char buf[10];\n"
3637+
" memcpy(buf, \"abc\", 4);\n"
3638+
"}\n");
3639+
ASSERT_EQUALS("", errout_str());
3640+
3641+
check("void f() {\n"
3642+
" char buf[128];\n"
3643+
" memcpy(buf, \"abc\", 5);\n"
3644+
"}\n");
3645+
ASSERT_EQUALS("[test.cpp:3:17]: (error) Buffer is accessed out of bounds: \"abc\" [bufferAccessOutOfBounds]\n",
3646+
errout_str());
3647+
3648+
check("void f() {\n"
3649+
" char buf[128];\n"
3650+
" memcpy(buf, L\"abc\", 4 * sizeof(wchar_t));\n"
3651+
"}\n");
3652+
ASSERT_EQUALS("", errout_str());
3653+
3654+
check("void f() {\n"
3655+
" char buf[128];\n"
3656+
" memcpy(buf, L\"abc\", 4 * sizeof(wchar_t) + 1);\n"
3657+
"}\n");
3658+
ASSERT_EQUALS("[test.cpp:3:17]: (error) Buffer is accessed out of bounds: L\"abc\" [bufferAccessOutOfBounds]\n",
3659+
errout_str());
3660+
3661+
check("int f(char* s) {\n"
3662+
" return memcmp(s, \"\\0\\0\\0\", 3);\n"
3663+
"}\n");
3664+
ASSERT_EQUALS("", errout_str());
3665+
3666+
check("int f(const char16_t* buf) {\n"
3667+
" return memcmp(buf, u\"abc\", 4 * sizeof(char16_t));\n"
3668+
"}\n");
3669+
ASSERT_EQUALS("", errout_str());
3670+
3671+
check("int f(const char* buf) {\n"
3672+
" return memcmp(buf, &\"abc\"[2], 4 * sizeof(char));\n"
3673+
"}\n");
3674+
ASSERT_EQUALS("[test.cpp:2:24]: (error) Buffer is accessed out of bounds: &\"abc\"[2] [bufferAccessOutOfBounds]\n",
3675+
errout_str());
3676+
}
3677+
36413678
void buffer_overrun_errorpath() {
36423679
setMultiline();
36433680
Settings s = settings0;
@@ -4618,9 +4655,8 @@ class TestBufferOverrun : public TestFixture {
46184655
check("void f() {\n"
46194656
" mymemset(\"abc\", 0, 20);\n"
46204657
"}\n", settings);
4621-
TODO_ASSERT_EQUALS("[test.cpp:2]: (error) Buffer is accessed out of bounds.\n",
4622-
"",
4623-
errout_str());
4658+
ASSERT_EQUALS("[test.cpp:2:12]: (error) Buffer is accessed out of bounds: \"abc\" [bufferAccessOutOfBounds]\n",
4659+
errout_str());
46244660

46254661
check("void f() {\n"
46264662
" mymemset(temp, \"abc\", 4);\n"

0 commit comments

Comments
 (0)