Skip to content

Commit 295153d

Browse files
rikardfalkeborndanmar
rikardfalkeborn
authored and
danmar
committed
Checkstring fixes (danmar#1783)
* teststring.cpp: Fix ternary syntax in tests * stringLiteralWrite: Add tests wide character and utf16 strings * suspiciousStringCompare: Add test with wide character string * strPlusChar: Handle wide characters * incorrectStringCompare: Add test with wide string * Suspicious string compare: suggest wcscmp for wide strings * deadStrcmp: Extend to handle wide strings * sprintfOverlappingData: Print name of strcmp function * Conversion of char literal to boolean, add wide character tests * Conversion of char literal to boolean, fix ternary
1 parent 16ebb90 commit 295153d

3 files changed

Lines changed: 110 additions & 25 deletions

File tree

‎lib/checkstring.cpp‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -213,18 +213,19 @@ void CheckString::checkSuspiciousStringCompare()
213213
const bool ischar(litTok->tokType() == Token::eChar);
214214
if (litTok->tokType() == Token::eString) {
215215
if (mTokenizer->isC() || (var && var->isArrayOrPointer()))
216-
suspiciousStringCompareError(tok, varname);
216+
suspiciousStringCompareError(tok, varname, litTok->isLong());
217217
} else if (ischar && var && var->isPointer()) {
218218
suspiciousStringCompareError_char(tok, varname);
219219
}
220220
}
221221
}
222222
}
223223

224-
void CheckString::suspiciousStringCompareError(const Token* tok, const std::string& var)
224+
void CheckString::suspiciousStringCompareError(const Token* tok, const std::string& var, bool isLong)
225225
{
226+
const std::string cmpFunc = isLong ? "wcscmp" : "strcmp";
226227
reportError(tok, Severity::warning, "literalWithCharPtrCompare",
227-
"$symbol:" + var + "\nString literal compared with variable '$symbol'. Did you intend to use strcmp() instead?", CWE595, false);
228+
"$symbol:" + var + "\nString literal compared with variable '$symbol'. Did you intend to use " + cmpFunc + "() instead?", CWE595, false);
228229
}
229230

230231
void CheckString::suspiciousStringCompareError_char(const Token* tok, const std::string& var)
@@ -240,7 +241,7 @@ void CheckString::suspiciousStringCompareError_char(const Token* tok, const std:
240241

241242
static bool isChar(const Variable* var)
242243
{
243-
return (var && !var->isPointer() && !var->isArray() && var->typeStartToken()->str() == "char");
244+
return (var && !var->isPointer() && !var->isArray() && (var->typeStartToken()->str() == "char" || var->typeStartToken()->str() == "wchar_t"));
244245
}
245246

246247
void CheckString::strPlusChar()
@@ -260,7 +261,12 @@ void CheckString::strPlusChar()
260261

261262
void CheckString::strPlusCharError(const Token *tok)
262263
{
263-
reportError(tok, Severity::error, "strPlusChar", "Unusual pointer arithmetic. A value of type 'char' is added to a string literal.", CWE665, false);
264+
std::string charType = "char";
265+
if (tok && tok->astOperand2() && tok->astOperand2()->variable())
266+
charType = tok->astOperand2()->variable()->typeStartToken()->str();
267+
else if (tok && tok->astOperand2() && tok->astOperand2()->tokType() == Token::eChar && tok->astOperand2()->isLong())
268+
charType = "wchar_t";
269+
reportError(tok, Severity::error, "strPlusChar", "Unusual pointer arithmetic. A value of type '" + charType +"' is added to a string literal.", CWE665, false);
264270
}
265271

266272
//---------------------------------------------------------------------------
@@ -309,7 +315,8 @@ void CheckString::checkIncorrectStringCompare()
309315
incorrectStringBooleanError(tok->next(), tok->strAt(1));
310316
} else if (Token::Match(tok, "if|while ( %str%|%char% )") && !tok->tokAt(2)->getValue(0)) {
311317
incorrectStringBooleanError(tok->tokAt(2), tok->strAt(2));
312-
}
318+
} else if (tok->str() == "?" && Token::Match(tok->astOperand1(), "%str%|%char%"))
319+
incorrectStringBooleanError(tok->astOperand1(), tok->astOperand1()->str());
313320
}
314321
}
315322
}
@@ -380,9 +387,9 @@ void CheckString::overlappingStrcmp()
380387

381388
for (const Token *eq0 : equals0) {
382389
for (const Token * ne0 : notEquals0) {
383-
if (!Token::simpleMatch(eq0->previous(), "strcmp ("))
390+
if (!Token::Match(eq0->previous(), "strcmp|wcscmp ("))
384391
continue;
385-
if (!Token::simpleMatch(ne0->previous(), "strcmp ("))
392+
if (!Token::Match(ne0->previous(), "strcmp|wcscmp ("))
386393
continue;
387394
const std::vector<const Token *> args1 = getArguments(eq0->previous());
388395
const std::vector<const Token *> args2 = getArguments(ne0->previous());
@@ -445,20 +452,22 @@ void CheckString::sprintfOverlappingData()
445452
true,
446453
false);
447454
if (same) {
448-
sprintfOverlappingDataError(args[argnr], args[argnr]->expressionString());
455+
sprintfOverlappingDataError(tok, args[argnr], args[argnr]->expressionString());
449456
}
450457
}
451458
}
452459
}
453460
}
454461

455-
void CheckString::sprintfOverlappingDataError(const Token *tok, const std::string &varname)
462+
void CheckString::sprintfOverlappingDataError(const Token *funcTok, const Token *tok, const std::string &varname)
456463
{
464+
const std::string func = funcTok ? funcTok->str() : "s[n]printf";
465+
457466
reportError(tok, Severity::error, "sprintfOverlappingData",
458467
"$symbol:" + varname + "\n"
459-
"Undefined behavior: Variable '$symbol' is used as parameter and destination in s[n]printf().\n"
460-
"The variable '$symbol' is used both as a parameter and as destination in "
461-
"s[n]printf(). The origin and destination buffers overlap. Quote from glibc (C-library) "
468+
"Undefined behavior: Variable '$symbol' is used as parameter and destination in " + func + "().\n" +
469+
"The variable '$symbol' is used both as a parameter and as destination in " +
470+
func + "(). The origin and destination buffers overlap. Quote from glibc (C-library) "
462471
"documentation (http://www.gnu.org/software/libc/manual/html_mono/libc.html#Formatted-Output-Functions): "
463472
"\"If copying takes place between objects that overlap as a result of a call "
464473
"to sprintf() or snprintf(), the results are undefined.\"", CWE628, false);

‎lib/checkstring.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,24 +86,24 @@ class CPPCHECKLIB CheckString : public Check {
8686

8787
private:
8888
void stringLiteralWriteError(const Token *tok, const Token *strValue);
89-
void sprintfOverlappingDataError(const Token *tok, const std::string &varname);
89+
void sprintfOverlappingDataError(const Token *funcTok, const Token *tok, const std::string &varname);
9090
void strPlusCharError(const Token *tok);
9191
void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string);
9292
void incorrectStringBooleanError(const Token *tok, const std::string& string);
9393
void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2);
9494
void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2);
95-
void suspiciousStringCompareError(const Token* tok, const std::string& var);
95+
void suspiciousStringCompareError(const Token* tok, const std::string& var, bool isLong);
9696
void suspiciousStringCompareError_char(const Token* tok, const std::string& var);
9797
void overlappingStrcmpError(const Token* eq0, const Token *ne0);
9898

9999
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
100100
CheckString c(nullptr, settings, errorLogger);
101101

102102
c.stringLiteralWriteError(nullptr, nullptr);
103-
c.sprintfOverlappingDataError(nullptr, "varname");
103+
c.sprintfOverlappingDataError(nullptr, nullptr, "varname");
104104
c.strPlusCharError(nullptr);
105105
c.incorrectStringCompareError(nullptr, "substr", "\"Hello World\"");
106-
c.suspiciousStringCompareError(nullptr, "foo");
106+
c.suspiciousStringCompareError(nullptr, "foo", false);
107107
c.suspiciousStringCompareError_char(nullptr, "foo");
108108
c.incorrectStringBooleanError(nullptr, "\"Hello World\"");
109109
c.incorrectStringBooleanError(nullptr, "\'x\'");

‎test/teststring.cpp‎

Lines changed: 84 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,14 @@ class TestString : public TestFixture {
4444
TEST_CASE(strPlusChar1); // "/usr" + '/'
4545
TEST_CASE(strPlusChar2); // "/usr" + ch
4646
TEST_CASE(strPlusChar3); // ok: path + "/sub" + '/'
47+
TEST_CASE(strPlusChar4); // L"/usr" + L'/'
4748

49+
TEST_CASE(snprintf1); // Dangerous usage of snprintf
4850
TEST_CASE(sprintf1); // Dangerous usage of sprintf
4951
TEST_CASE(sprintf2);
5052
TEST_CASE(sprintf3);
5153
TEST_CASE(sprintf4); // struct member
54+
TEST_CASE(wsprintf1); // Dangerous usage of wsprintf
5255

5356
TEST_CASE(incorrectStringCompare);
5457

@@ -105,6 +108,18 @@ class TestString : public TestFixture {
105108
" foo_FP1(s);\n"
106109
"}");
107110
ASSERT_EQUALS("", errout.str());
111+
112+
check("void f() {\n"
113+
" wchar_t *abc = L\"abc\";\n"
114+
" abc[0] = u'a';\n"
115+
"}");
116+
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (error) Modifying string literal \"abc\" directly or indirectly is undefined behaviour.\n", errout.str());
117+
118+
check("void f() {\n"
119+
" char16_t *abc = u\"abc\";\n"
120+
" abc[0] = 'a';\n"
121+
"}");
122+
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (error) Modifying string literal \"abc\" directly or indirectly is undefined behaviour.\n", errout.str());
108123
}
109124

110125
void alwaysTrueFalseStringCompare() {
@@ -263,6 +278,11 @@ class TestString : public TestFixture {
263278
"}");
264279
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use strcmp() instead?\n", errout.str());
265280

281+
check("bool foo(wchar_t* c) {\n"
282+
" return c == L\"x\";\n"
283+
"}");
284+
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal compared with variable 'c'. Did you intend to use wcscmp() instead?\n", errout.str());
285+
266286
check("bool foo(const char* c) {\n"
267287
" return \"x\" == c;\n"
268288
"}");
@@ -370,6 +390,11 @@ class TestString : public TestFixture {
370390
"}");
371391
ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str());
372392

393+
check("bool foo(wchar_t* c) {\n"
394+
" return c == L'x';\n"
395+
"}");
396+
ASSERT_EQUALS("[test.cpp:2]: (warning) Char literal compared with pointer 'c'. Did you intend to dereference it?\n", errout.str());
397+
373398
check("bool foo(char* c) {\n"
374399
" return '\\0' != c;\n"
375400
"}");
@@ -419,13 +444,22 @@ class TestString : public TestFixture {
419444
}
420445

421446

447+
void snprintf1() {
448+
check("void foo()\n"
449+
"{\n"
450+
" char buf[100];\n"
451+
" snprintf(buf,100,\"%s\",buf);\n"
452+
"}");
453+
ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in snprintf().\n", errout.str());
454+
}
455+
422456
void sprintf1() {
423457
check("void foo()\n"
424458
"{\n"
425459
" char buf[100];\n"
426460
" sprintf(buf,\"%s\",buf);\n"
427461
"}");
428-
ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in s[n]printf().\n", errout.str());
462+
ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in sprintf().\n", errout.str());
429463
}
430464

431465
void sprintf2() {
@@ -462,6 +496,15 @@ class TestString : public TestFixture {
462496
ASSERT_EQUALS("", errout.str());
463497
}
464498

499+
void wsprintf1() {
500+
check("void foo()\n"
501+
"{\n"
502+
" wchar_t buf[100];\n"
503+
" swprintf(buf,10, \"%s\",buf);\n"
504+
"}");
505+
ASSERT_EQUALS("[test.cpp:4]: (error) Undefined behavior: Variable 'buf' is used as parameter and destination in swprintf().\n", errout.str());
506+
}
507+
465508
void strPlusChar1() {
466509
// Strange looking pointer arithmetic..
467510
check("void foo()\n"
@@ -500,30 +543,49 @@ class TestString : public TestFixture {
500543
ASSERT_EQUALS("", errout.str());
501544
}
502545

546+
void strPlusChar4() {
547+
// Strange looking pointer arithmetic, wide char..
548+
check("void foo()\n"
549+
"{\n"
550+
" const wchar_t *p = L\"/usr\" + L'/';\n"
551+
"}");
552+
ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'wchar_t' is added to a string literal.\n", errout.str());
553+
554+
check("void foo(wchar_t c)\n"
555+
"{\n"
556+
" const wchar_t *p = L\"/usr\" + c;\n"
557+
"}");
558+
ASSERT_EQUALS("[test.cpp:3]: (error) Unusual pointer arithmetic. A value of type 'wchar_t' is added to a string literal.\n", errout.str());
559+
}
503560

504561
void incorrectStringCompare() {
505562
check("int f() {\n"
506-
" return test.substr( 0 , 4 ) == \"Hello\" ? : 0 : 1 ;\n"
563+
" return test.substr( 0 , 4 ) == \"Hello\" ? 0 : 1 ;\n"
507564
"}");
508565
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str());
509566

510567
check("int f() {\n"
511-
" return test.substr( 0 , 5 ) == \"Hello\" ? : 0 : 1 ;\n"
568+
" return test.substr( 0 , 4 ) == L\"Hello\" ? 0 : 1 ;\n"
569+
"}");
570+
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str());
571+
572+
check("int f() {\n"
573+
" return test.substr( 0 , 5 ) == \"Hello\" ? 0 : 1 ;\n"
512574
"}");
513575
ASSERT_EQUALS("", errout.str());
514576

515577
check("int f() {\n"
516-
" return \"Hello\" == test.substr( 0 , 4 ) ? : 0 : 1 ;\n"
578+
" return \"Hello\" == test.substr( 0 , 4 ) ? 0 : 1 ;\n"
517579
"}");
518580
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str());
519581

520582
check("int f() {\n"
521-
" return \"Hello\" == foo.bar<int>().z[1].substr(i+j*4, 4) ? : 0 : 1 ;\n"
583+
" return \"Hello\" == foo.bar<int>().z[1].substr(i+j*4, 4) ? 0 : 1 ;\n"
522584
"}");
523585
ASSERT_EQUALS("[test.cpp:2]: (warning) String literal \"Hello\" doesn't match length argument for substr().\n", errout.str());
524586

525587
check("int f() {\n"
526-
" return \"Hello\" == test.substr( 0 , 5 ) ? : 0 : 1 ;\n"
588+
" return \"Hello\" == test.substr( 0 , 5 ) ? 0 : 1 ;\n"
527589
"}");
528590
ASSERT_EQUALS("", errout.str());
529591

@@ -547,6 +609,11 @@ class TestString : public TestFixture {
547609
"}");
548610
ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str());
549611

612+
check("int f() {\n"
613+
" return \"Hello\" ? 1 : 2;\n"
614+
"}");
615+
ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of string literal \"Hello\" to bool always evaluates to true.\n", errout.str());
616+
550617
check("int f() {\n"
551618
" assert (test || \"Hello\");\n"
552619
"}");
@@ -582,22 +649,26 @@ class TestString : public TestFixture {
582649
" if('a'){}\n"
583650
" if(L'b'){}\n"
584651
" if(1 && 'c'){}\n"
585-
" int x = 'd' ? 1 : 2;\n" // <- TODO
652+
" int x = 'd' ? 1 : 2;\n"
586653
"}");
587654
ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal 'a' to bool always evaluates to true.\n"
588655
"[test.cpp:3]: (warning) Conversion of char literal 'b' to bool always evaluates to true.\n"
589656
"[test.cpp:4]: (warning) Conversion of char literal 'c' to bool always evaluates to true.\n"
657+
"[test.cpp:5]: (warning) Conversion of char literal 'd' to bool always evaluates to true.\n"
590658
, errout.str());
591659

592660
check("void f() {\n"
593661
" if('\\0'){}\n"
662+
" if(L'\\0'){}\n"
594663
"}");
595664
ASSERT_EQUALS("", errout.str());
596665

597666
check("void f() {\n"
598667
" if('\\0' || cond){}\n"
668+
" if(L'\\0' || cond){}\n"
599669
"}");
600-
ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n", errout.str());
670+
ASSERT_EQUALS("[test.cpp:2]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n"
671+
"[test.cpp:3]: (warning) Conversion of char literal '\\0' to bool always evaluates to false.\n", errout.str());
601672
}
602673

603674
void deadStrcmp() {
@@ -606,6 +677,11 @@ class TestString : public TestFixture {
606677
"}");
607678
ASSERT_EQUALS("[test.cpp:2]: (warning) The expression 'strcmp(str,\"def\") != 0' is suspicious. It overlaps 'strcmp(str,\"abc\") == 0'.\n", errout.str());
608679

680+
check("void f(const wchar_t *str) {\n"
681+
" if (wcscmp(str, L\"abc\") == 0 || wcscmp(str, L\"def\")) {}\n"
682+
"}");
683+
ASSERT_EQUALS("[test.cpp:2]: (warning) The expression 'wcscmp(str,L\"def\") != 0' is suspicious. It overlaps 'wcscmp(str,L\"abc\") == 0'.\n", errout.str());
684+
609685
check("struct X {\n"
610686
" char *str;\n"
611687
"};\n"

0 commit comments

Comments
 (0)