Repository navigation
Fix #11765 FN: minsize not checked for string literal, buffer access out of bounds not found - #8944
chrchr-github wants to merge 5 commits into
Conversation
|
Test results for commit cd753bf (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The PR adds new false positives. String literals now get a BUFFER_SIZE. As a result, every library minsize type="argvalue" on an input argument is checked against the literal's size. That is wrong for bounded string functions, which stop reading at the terminating NUL. Valid calls now get bufferAccessOutOfBounds warnings:
strncasecmp(s, "abc", 10)strnlen("abc", 100)_strnicmp(s, "abc", 10)strxfrm_l(d, "abc", 10, l)
The PR removes minsize from strxfrm, strncpy_s and strncat_s in std.cfg for exactly this reason. It leaves the same pattern in other places:
strxfrm_lin posix.cfg, which is the direct counterpart ofstrxfrmstrncasecmp,wcsncasecmp,strnlen/wcsnlenandwcswidthin posix.cfg_strnicmp*and_strncpy_s_l/_mbsncpy_sin windows.cfgsqlite3_stricmpin sqlite3.cfg
These configs need the same cleanup. The other option is to apply the literal size only for functions that are really memory-based (memcpy, memcmp, write, …). The new warnings on memory functions (memcmp(s, "abc", 10), write(fd, "abc", 10)) are correct. Wide, u8/U and concatenated literals give the expected sizes.
Testing
Built: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/. Both built fine.
Unit tests: ./testrunner TestBufferOverrun passed: 198 tests, 0 failures.
Library test files: I ran test/cfg/{std.c,std.cpp,posix.c,gnu.c,bsd.c,windows.cpp} through the cppcheck of both base and the PR with the runtests.sh options. The output is the same on both, so no existing cfg test covers these cases.
False positives found: scratch/a.c:
#include <string.h>
#include <strings.h>
#include <locale.h>
int f1(const char *s) { return strncasecmp(s, "abc", 10); }
size_t f3(void) { return strnlen("abc", 100); }
int f4(const char *s) { return _strnicmp(s, "abc", 10); }
size_t f5(char *d, locale_t l) { return strxfrm_l(d, "abc", 10, l); }Command: cppcheck -q --library=posix --library=windows a.c
PR output:
a.c:5:47: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (strncasecmp)
a.c:7:34: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (strnlen)
a.c:8:45: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (_strnicmp)
a.c:9:54: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (strxfrm_l)
Base: no warnings. All four calls are valid, because these functions stop at the NUL.
Checked and correct:
memcmp(s,"abc",10)andwrite(fd,"abc",10)are now flagged. Both are real overreads.memcpy(d, "ab" "cd", 6)is OK and5is OK; I did not test a size large enough to overread this literal.memcpy(d, U"ab", 12)is OK and13is flagged.wcsncasecmp(s, L"abc", 10)andwcswidth(L"ab", 10)are not flagged here only because 10 is within the literal's size in bytes. They would be flagged with a larger bound.
Automated review of cd753bf by Claude. The pull request was built and tested locally.
|
Test results for commit 9527653 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New false positives remain in library configs that the cleanup missed. The literal size is now checked against every input minsize type="argvalue". The latest commits removed it from the entries the earlier review listed, but three more bounded string functions still produce bufferAccessOutOfBounds on valid calls. Base does not warn on any of them:
_strnicmp_lin windows.cfg, args 1 and 2g_strncasecmpin gtk.cfg, args 1 and 2wcpncpyin posix.cfg, arg 2
The CI package run did not show these, but I reproduced them directly. Deleting the minsize entries one function at a time also has a cost. strnlen(buf, 3) on a char buf[2] that is not NUL-terminated was detected in base, and the PR turns that test in posix.c into a TODO; strncasecmp, strxfrm, strncpy_s and the others lose the same detection for real arrays. Skipping the literal size for <strz/> arguments in bufferOverflow() would fix the false positives without these false negatives. That is the alternative the earlier review proposed. Apart from this I found no new problems in the C++ change.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/. Both built fine.
Unit tests: ./testrunner TestBufferOverrun passed with 0 failures.
False positives that are still there
scratch/a.c:
#include <string.h>
#include <wchar.h>
#include <glib.h>
int f1(const char *s, _locale_t l) { return _strnicmp_l(s, "abc", 10, l); }
gint f2(const gchar *s) { return g_strncasecmp(s, "abc", 10); }
size_t f6(void) { const char buf[2]={'4','2'}; return strnlen(buf,3); }Command: cppcheck -q --library=posix --library=windows --library=gtk a.c
Base output:
a.c:9:63: error: Buffer is accessed out of bounds: buf [bufferAccessOutOfBounds] (strnlen on the non-terminated array)
PR output:
a.c:4:60: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (_strnicmp_l)
a.c:5:51: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds] (g_strncasecmp)
- The two calls the PR flags are valid, because these functions stop at the NUL.
- The PR no longer flags the real
strnlenoverread that base found. That is the TODO now in test/cfg/posix.c.
scratch/b.c:
#include <wchar.h>
wchar_t* f3(wchar_t *d) { return wcpncpy(d, L"ab", 20); }cppcheck -q --library=posix b.c reports b.c:2:45: warning: Buffer is accessed out of bounds: L"ab" with the PR and nothing with base. wcpncpy stops reading the source at the NUL, so this call is valid.
Also checked: memcpy(d, &"abc"[2], 4) and memcpy(d, "abc" + 2, 4) are not flagged. These are false negatives only, not false positives.
Script for the remaining entries: a short Python script lists every remaining minsize type="argvalue" on input or strz arguments in cfg/*.cfg; I used it to find the entries above.
Automated review of 9527653 by Claude. The pull request was built and tested locally.
1bf11c5 to
5ec129e
Compare
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The <strz/> check does not stop the false positives on bounded string functions. Those library entries don't mark their string arguments <strz/>. The current version puts back the cfg entries and skips the literal size only when isargstrz() is true. But strnlen/wcsnlen, strncasecmp/wcsncasecmp, wcpncpy (arg 2), _strnicmp*, _strnicmp_l*, g_strncasecmp, _strncpy_s_l/_mbsncpy_s and others have minsize type="argvalue" on an in argument that has no <strz/>. So valid calls such as strnlen("abc", 100) and strncasecmp(s, "abc", 10) are again reported as bufferAccessOutOfBounds, and base reports nothing on them.
There are two ways to fix this:
- Add
<strz/>to those arguments in the cfg files. Then the real overread on non-terminated arrays (strnlen(buf,3)) is still found. - Limit the literal size to functions that work on raw memory.
The new line strnlen("ab",3) in test/cfg/posix.c doesn't test this: the bound 3 equals the literal size, so it passes, while strnlen("ab",4) is flagged. Apart from this I found no new problems in the C++ change.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both built fine.
Unit tests: ./testrunner TestBufferOverrun passed: 198 tests, 0 failures.
cfg test: test/cfg/posix.c gives no warning on the new line 722, strnlen("ab",3), because 3 is not larger than the literal's size.
False positives: scratch/a.c:
#include <string.h>
#include <strings.h>
#include <wchar.h>
int f1(const char *s) { return strncasecmp(s, "abc", 10); }
size_t f2(void) { return strnlen("abc", 100); }
int f3(const char *s) { return _strnicmp(s, "abc", 10); }
wchar_t* f4(wchar_t *d) { return wcpncpy(d, L"ab", 20); }
int f5(const char *s) { return g_strncasecmp(s, "abc", 10); }
void f6(lua_State *L) { lua_pushlstring(L, "abc", 10); }
int f7(const char *s) { return memcmp(s, "abc", 10); }
char *f8(char *d) { return strncpy(d, "ab", 20); }
char *f9(char *d) { return strncat(d, "ab", 20); }
int f10(const char *s) { return strncmp(s, "abc", 10); }Command: cppcheck -q --library=posix --library=windows --library=gtk --library=lua a.c
- Base: no output.
- PR:
bufferAccessOutOfBoundson lines 4, 5, 6, 7, 8, 9 and 10.- Lines 4–8 are false positives, because these functions stop at the NUL.
- Lines 9 (
lua_pushlstring) and 10 (memcmp) are correct new detections. strncpy,strncatandstrncmpstay quiet because their arguments have<strz/>.
Finding the affected entries: a small Python script lists the minsize type="argvalue" entries on non-output arguments without <strz/> in cfg/*.cfg. Besides the functions above it found wcsncasecmp, _wcsnicmp*, _mbsnicmp*, _strncpy_s_l, _wcsncpy_s_l, _mbsncpy_s and _mbsncpy_s_l.
Automated review of 5ec129e by Claude. The pull request was built and tested locally.
|
Test results for commit 5ec129e (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new <strz/> on strncasecmp in posix.cfg causes a new false positive. <strz/> makes CheckFunctions require a NUL-terminated argument. strncasecmp reads at most n bytes, so a char array without a terminator, or &c, is valid when n is no larger than the buffer. The PR now reports both cases as invalidFunctionArgStr errors, and base reports nothing. So adding <strz/> to the bounded string functions that the earlier review listed is not a free fix either: it trades the literal false positive for this one. A narrower option is to skip the literal size for these functions in bufferOverflow(), for example when the argument is marked <strz/> or has a minsize tied to a length argument, without changing the cfg.
The other remaining gaps were already raised in comments.md: strnlen, _strnicmp, wcpncpy and g_strncasecmp still have no <strz/>, and strnlen("ab",3) does not test the change. I found no other new problems in the C++ change.
Testing
- Builds:
make -j8 cppcheck testrunnerin pr/ andmake -j8 cppcheckin base/. Both built fine. - Unit tests:
./testrunner TestBufferOverrun TestFunctionsreported 0 failures. - cfg test:
test/cfg/posix.cwith--library=posix --check-library --enable=style,information --inline-suppr --error-exitcode=1exits with 0.
New false positive from adding <strz/> to strncasecmp. scratch/s.c:
#include <strings.h>
int f1(const char *s) {
char buf[3] = {'a','b','c'};
return strncasecmp(buf, s, 3);
}
int f2(const char *s) {
char c = 'a';
return strncasecmp(&c, s, 1);
}Command: cppcheck -q --library=posix s.c
- Base: no output.
- PR:
s.c:4:24: error: Invalid strncasecmp() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
s.c:8:24: error: Invalid strncasecmp() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
Both calls are valid because strncasecmp reads at most n bytes.
Automated review of 1fa524a by Claude. The pull request was built and tested locally.
|
Test results for commit 1fa524a (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new hasMinSize() exception in CheckFunctions::invalidFunctionUsage() turns off invalidFunctionArgStr for every <strz/> argument that also has a <minsize>. It doesn't only cover the bounded comparison functions it was added for. That includes the destination of strcat, strncat, wcsncat, strcat_s, strlcat, _mbsncat and others, where a NUL terminator really is needed. So the PR loses real detections: char buf[3] = {'a','b','c'}; strcat(buf, "x"); and strncat(d, "x", 1) on a non-terminated d were reported by base and are silent with the PR. No test in test/cfg covers these cases, so CI does not catch it. Limiting the exception to direction="in" arguments with minsize type="argvalue" (or dropping the global exception and handling bounded functions differently) would avoid this.
I found no new problems in the getBufferSize() change beyond what comments.md already has.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/. Both built fine.
Unit tests: ./testrunner TestBufferOverrun TestFunctions gave Tests failed: 0.
cfg tests: I ran test/cfg/{std.c,std.cpp,posix.c,windows.cpp,bsd.c} through both binaries with --inline-suppr. In the PR there is no remaining invalidFunctionArgStr output. In base, the only output is the 3 strxfrm lines whose suppressions the PR removes. No cfg test covers strz on a strcat/strncat destination.
Affected entries: a small script lists every arg in cfg/*.cfg that has both <strz/> and <minsize>. The PR now skips the strz check for all of them. Among them are strcat arg1 (minsize strlen), strncat/wcsncat arg1, strcat_s/wcscat_s arg1, strlcat arg1, _mbscat_s, _mbsncat, _mbsncat_l, _wcsncat_l arg1, strncat_s/strncpy_s arg3, sethostname and mq_timedsend.
Lost detections (false negatives), scratch/a.c:
#include <string.h>
void f1(void) { char buf[3] = {'a','b','c'}; strcat(buf, "x"); }
void f2(void) { char c = 'a'; strcat(&c, "x"); }
void f3(void) { char buf[10] = {'a','b','c'}; char d[3]={'a','b','c'}; strncat(d, "x", 1); }
void f4(void) { char c = 'a'; strncat(&c, "", 1); }Command: cppcheck -q --library=posix a.c
Base:
a.c:3:38: error: Buffer is accessed out of bounds: &c [bufferAccessOutOfBounds]
a.c:2:53: error: Invalid strcat() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
a.c:3:38: error: Invalid strcat() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
a.c:4:80: error: Invalid strncat() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
a.c:5:39: error: Invalid strncat() argument nr 1. A nul-terminated string is required. [invalidFunctionArgStr]
PR:
a.c:3:38: error: Buffer is accessed out of bounds: &c [bufferAccessOutOfBounds]
All four invalidFunctionArgStr errors are lost. They are real bugs, because strcat/strncat look for the NUL terminator in the destination.
Automated review of 558148b by Claude. The pull request was built and tested locally.
|
Test results for commit 558148b (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found two new problems in the string-literal branch of getBufferSize(). Neither causes false positives, so neither is a blocker.
- A definite overread of a literal is reported at the wrong severity. The new
BUFFER_SIZEvalue is never set to known, andValueFlow::Valuestarts out asPossible. Somemcpy(d, "abc", 5)is reported as awarning, while the same overread onchar a[4]is reported as anerror. Themymemset("abc", 0, 20)TODO in testbufferoverrun.cpp originally expected(error). The PR changed that expectation to(warning)without saying why. - The index in
&"abc"[i]is ignored.getBufferSize()readsindexand then returns the whole literal size, somemcpy(d, &"abc"[2], 3)is not reported. The array branch subtracts the index (dim - index).
Everything else I checked works. The cfg <strz/> additions and the ARGVALUE-only hasArgMinSize() now keep strnlen("ab",5) and strncasecmp(s,"abc",10) quiet. They also keep the strcat destination check. The issues already raised in comments.md are not repeated here: the lost strncat destination check, and wcsncasecmp, _strnicmp_l and _strncpy_s_l without <strz/>.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both succeeded.
Unit tests: ./testrunner TestBufferOverrun TestFunctions reported Tests failed: 0.
cfg entries: a script listed every arg in cfg/*.cfg that has <strz/> together with an argvalue minsize, for base and for the PR. The only difference is the new entries: strncasecmp, strnlen/wcsnlen, wcpncpy arg 2, _strnicmp* and g_strncasecmp.
Mixed inputs (scratch/a.c, --library=posix,gtk,gnu):
- Now reported, all real overreads:
memcmp(s,"abc",10),fwrite("abc",1,10,stdout),fwrite("abc",10,1,stdout),memcpy(buf,"abc",sizeof(buf)),memcpy(d,"a\0b",5)andg_string_new_len("abc",10). - Not reported, as expected:
strnlen("ab",5),strncasecmp(s,"abc",10),strncmp(s,"abc",10)andg_string_new_len("abc",-1).
Severity (scratch/c.cpp):
#include <cstring>
void f3(char *d) { char a[4] = "abc"; memcpy(d, a, 5); }
void f4(char *d) { memcpy(d, "abc", 5); }Command: pr/cppcheck -q --enable=warning c.cpp
c.cpp:2:49: error: Buffer is accessed out of bounds: a [bufferAccessOutOfBounds]
c.cpp:3:30: warning: Buffer is accessed out of bounds: "abc" [bufferAccessOutOfBounds]
Index ignored (scratch/b.c):
#include <string.h>
void s2(char *d) { memcpy(d, &"abc"[2], 3); }
void s3(char *d) { memcpy(d, &"abc"[3], 2); }
void a1(char *d) { char a[4]="abc"; memcpy(d, &a[2], 3); }Command: pr/cppcheck -q --enable=warning b.c
Only a1 is reported (error: Buffer is accessed out of bounds: &a[2]). s2 and s3 are real overreads and get no warning.
Automated review of 062dd28 by Claude. The pull request was built and tested locally.
|
Test results for commit 062dd28 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found one new problem: the PR loses a real detection for strxfrm and strxfrm_l.
- Cause: the new
hasArgMinReadSize()exception inCheckFunctionsImpl::invalidFunctionUsage()turns off the<strz/>check for everydirection="in"argument with aminsize type="argvalue". - Why it is wrong here: that is right for bounded reads such as
strncasecmpandstrnlen. Butstrxfrmandstrxfrm_lread the whole source up to its NUL terminator;nonly limits what is written to the destination. Their source argument (arg 2) has both<strz/>and aminsize argvalue, so a source that is not NUL-terminated is no longer reported. - Test change: the PR removes the
invalidFunctionArgStrsuppressions inbufferAccessOutOfBounds_strxfrmin test/cfg/std.c. That makes the cfg test accept the missed warning.
Apart from this, I found no new problems beyond what comments.md already covers.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/. Both built fine.
Unit tests: ./testrunner TestBufferOverrun TestFunctions reported Tests failed: 0.
cfg tests: I ran test/cfg/std.c and test/cfg/posix.c through the PR's cppcheck with --inline-suppr --check-library --error-exitcode=1. They gave no invalidFunctionArgStr or bufferAccessOutOfBounds output. My options differed from runtests.sh, so a few unrelated unmatchedSuppression notes appeared.
Affected cfg entries: a script listed the args that now skip the strz check because they have <strz/>, direction="in" and minsize argvalue. They include strxfrm/std::strxfrm arg 2 (std.cfg) and strxfrm_l arg 2 (posix.cfg). Both need a NUL-terminated source.
Lost detection: scratch/x.c
#include <string.h>
#include <locale.h>
void f1(char *d) { const char src[3] = {'a','b','c'}; (void)strxfrm(d, src, 1); }
void f2(char *d) { char c = 'a'; (void)strxfrm(d, &c, 1); }
void f3(char *d, locale_t l) { const char src[3] = {'a','b','c'}; (void)strxfrm_l(d, src, 1, l); }Command: cppcheck -q --library=posix x.c
Base:
x.c:3:72: error: Invalid strxfrm() argument nr 2. A nul-terminated string is required. [invalidFunctionArgStr]
x.c:4:51: error: Invalid strxfrm() argument nr 2. A nul-terminated string is required. [invalidFunctionArgStr]
x.c:5:86: error: Invalid strxfrm_l() argument nr 2. A nul-terminated string is required. [invalidFunctionArgStr]
PR: no output. All three calls are real overreads, because strxfrm reads src up to its NUL whatever n is.
Other checks:
memcpy(d, "", 2)is reported.memcpy(d, "abc", 4),strncmp(s, "abc", 10)andwrite(fd, "abc", sizeof("abc"))are quiet, as expected.
Automated review of bb3c7f8 by Claude. The pull request was built and tested locally.
| } | ||
| // check <strz> | ||
| if (mSettings.library.isargstrz(functionToken, argnr)) { | ||
| if (mSettings.library.isargstrz(functionToken, argnr) && !hasArgMinReadSize(functionToken, argnr, mSettings.library)) { |
There was a problem hiding this comment.
AI review
This exception also covers strxfrm/std::strxfrm arg 2 (std.cfg) and strxfrm_l arg 2 (posix.cfg). Those functions read the whole source up to its NUL terminator; n only limits what is written to the destination, so the strz check is needed there. The PR misses const char src[3] = {'a','b','c'}; strxfrm(d, src, 1);, char c='a'; strxfrm(d, &c, 1); and the same strxfrm_l call, all of which base reports as invalidFunctionArgStr.
Two possible fixes:
- Mark the functions that should skip the check explicitly instead of inferring it from
minsize argvalue. - Remove the misleading
minsize arg="3"from the source argument ofstrxfrm/strxfrm_l; it bounds the destination, not the source.
| const char src[3] = "abc"; | ||
| char dest[1] = "a"; | ||
| // cppcheck-suppress invalidFunctionArgStr | ||
| (void)strxfrm(dest,src,1); |
There was a problem hiding this comment.
AI review
src is {'a','b','c'}, which has no NUL terminator. strxfrm reads all of src whatever n is, so invalidFunctionArgStr is a real error here. Removing these suppressions makes the cfg test accept the detection that hasArgMinReadSize() loses, instead of catching it.
|
Test results for commit bb3c7f8 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
…out of bounds not found - cppcheck-opensource#8944
bb3c7f8 to
18d5f15
Compare
|
Test results for commit 9759bf7 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8944/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems in the current version (9759bf7). The literal sizes are correct for narrow, u8, L, u and U literals on unix64 and win64. They are also correct for \u/\U escapes, raw strings and concatenated literals, and for &"abc"[i], including i past the end. Definite overreads are now reported as error. The memory-style functions (memcpy, memcmp, write, std::ostream::write, …) report real overreads of literals. The bounded <strz/> functions stay quiet.
One problem from earlier reviews is still there:
strxfrm_larg 2 in posix.cfg keeps<strz/>together withminsize type="argvalue", sohasArgMinReadSize()still turns off itsinvalidFunctionArgStrcheck. The latest commit fixed this forstrxfrmonly.wcsncasecmp,_strncpy_s_land_mbsncpy_sare also unchanged.
comments.md already covers all of these, so I don't repeat them as comments.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both built fine.
Unit tests: ./testrunner TestBufferOverrun TestFunctions reported Tests failed: 0.
cfg tests: I ran test/cfg/{std.c,posix.c,gnu.c,gtk.c,sqlite3.c,std.cpp,windows.cpp} through the PR's cppcheck with --check-library --inline-suppr --suppress=unmatchedSuppression. There was no bufferAccessOutOfBounds or invalidFunctionArgStr output. The only finding was one constVariable in windows.cpp, and that comes from my platform option (unix64), not from the PR.
cfg audit: a script listed every direction="in" argument with minsize type="argvalue", split into those with and without <strz/>:
- Without
<strz/>: these now get the literal size. They are all functions that read exactlynbytes (mem*, write/send, zlib, lua_pushlstring, …). The exceptions are_mbsncpy_sand_strncpy_s_l, which comments.md already covers. - With
<strz/>: these skip the literal size. That list includesstrxfrm_l, which comments.md also already covers.
Scratch tests (PR cppcheck):
- Reported, all correct:
memcpy(d, L"abc", 17),u"abc", 9andU"abc", 17on unix64.memcpy(d, L"abc", 16)on win64.os.write("abc", 10).wmemcmp(d, L"abc", 17).memcpy(d, &"abc"[5], 1), reported asarrayIndexOutOfBounds.
- Quiet, all correct:
memcpy(d, "é", 3),"\U0001F600", 5,R"(ab\n)", 5,"\xe9\x41", 3,"\101\0", 3,"é", 3,u8"é", 3,L"\U0001F600", 8,u"\U0001F600", 6and"a" "é", 4.memcpy(d, &"abc"[4], 0),os.write("abc", 3)andwrite(fd, "hello\n", sizeof("hello\n")-1).
- Missed, not caused by this PR:
wmemcpy(d, L"abc", 5). Thewmemcpy/wmemcmpminsize has nobaseType, sonis compared as a byte count. Awchar_t a[4]array is missed in the same way.
Automated review of 9759bf7 by Claude. The pull request was built and tested locally.
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
just a nit. with that fixed, in my opinion you can merge this.
| } | ||
| } | ||
| } else if (checkStringLiteral && bufTok->tokType() == Token::Type::eString && bufTok->valueType()) { | ||
| ValueFlow::Value strSizeVal; |
There was a problem hiding this comment.
a nit: strSizeVal can be moved into the inner scope.
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems in the current version (91ff78c). The earlier review points are now fixed:
strxfrmandstrxfrm_lno longer have the sourceminsize, so theirinvalidFunctionArgStrcheck is back.<strz/>is now on the bounded string functions in posix, windows and gtk (strncasecmp,wcsncasecmp,strnlen/wcsnlen,wcpncpy,_strnicmp*,_mbsncpy_s,_strncpy_s_l,g_strncasecmp). Calls likestrnlen("abc", 100)therefore stay quiet.
One small thing comes from an existing cfg entry, not from this PR. In posix.cfg, mq_timedsend arg 2 (the message buffer) is marked <strz/>, which is wrong for a binary message. Because of that, the new literal check skips it, and mq_timedsend(q, "abc", 10, 0, 0) is not reported. The same applies to wxString::From8BitData arg 1 and sethostname arg 1. You could drop those <strz/> tags in a follow-up.
Testing
Builds: make -j8 cppcheck testrunner in pr/ and make -j8 cppcheck in base/ both built fine.
Unit tests: ./testrunner TestBufferOverrun TestFunctions reported Tests failed: 0.
cfg tests: I ran test/cfg/{std.c,posix.c,gnu.c,gtk.c,std.cpp,windows.cpp} through the PR's cppcheck with --check-library --inline-suppr --suppress=unmatchedSuppression. There was no bufferAccessOutOfBounds or invalidFunctionArgStr output.
cfg audit: a script compared base and the PR for direction="in" args that have both <strz/> and minsize type="argvalue".
- Added by the PR: the bounded string functions listed above.
- Removed by the PR:
strxfrmandstrxfrm_l. - Still there from before:
sethostname,mq_timedsend,sqlite3_stricmp,strncpy_s,strncat_s,wcswidthandwxString::From8BitData.
A second script looked for input args without <strz/> that have other minsize types. It found only type="value" entries, and none of them take string literals.
Scratch tests: cppcheck -q --library=posix --library=gnu t.c, compared with base.
- New and correct:
memmove(d, "abc", 5)is reported. - Quiet, as expected:
strncpy(d,"abc",10),strncmp(s,"abc",10),strncat(d,"abc",10)wcsnlen(L"abc",100),wcpncpy(d,L"ab",10),strxfrm(d,"abc",10)memcpy(d, c ? "a" : "abc", 5)
- Missed, same as for arrays:
memcpy(d, &"abc"[4], 1). The size is 0, and the code skips sizes of 0 or less. - Missed because of the cfg entry:
mq_timedsend(q, "abc", 10, 0, 0), as explained in the summary.
strxfrm overread: the PR deletes the "source size is too small" test from testbufferoverrun.cpp. I checked that this case is still caught. const char src[1] = "a"; strxfrm(dest,src,1); and char src[2]="ab"; strxfrm(d,src,1); both still give invalidFunctionArgStr in the PR, the same as in base.
Automated review of 91ff78c by Claude. The pull request was built and tested locally.
4 participants
No description provided.