Skip to content

Fix #15124 FN bufferAccessOutOfBounds with wmemcpy() - #8951

Open
chrchr-github wants to merge 3 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15124
Open

chrchr-github wants to merge 3 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15124

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

The change is correct for wmemcpy and wmemcmp, but it misses some functions. Their sibling functions have the same minsize type="argvalue" arg="3" without baseType="wchar_t", so they still miss the same overflow (false negative):

  • wmemmove (cfg/std.cfg, ~line 4084/4090)
  • wmemset (~4144)
  • wmemchr (~3931)
  • wmempcpy in cfg/gnu.cfg (lines 662/667)

These should get baseType="wchar_t" in this PR as well, along with matching tests in test/cfg/std.c. I found no false positives and no other problems.

Testing

I built cppcheck and testrunner in pr/ without problems. cppcheck --check-library --library=std --inline-suppr --error-exitcode=1 passes on test/cfg/std.c and test/cfg/std.cpp, so the new suppressions match.

The checks below use scratch/t.c:

#include <wchar.h>
void f1(void){ wchar_t a[4]; wmemcpy(a, L"abcd", 5); }
int f2(const wchar_t*b){ const wchar_t a[4]={0}; return wmemcmp(a,b,5); }
void f3(void){ wchar_t a[4]; wmemmove(a, L"abcd", 5); }
void f4(void){ wchar_t a[4]; wmemset(a, L'x', 5); }
const wchar_t* f5(void){ static const wchar_t a[4]={0}; return wmemchr(a, L'x', 5); }
void ok1(void){ wchar_t a[4]; wmemcpy(a, L"abc", 4); }
void ok3(void){ wchar_t a[4]; wmemmove(a, L"abc", 4); }
void ok4(void){ wchar_t a[4]; wmemset(a, L'x', 4); }

Command: ./cppcheck -q --library=std t.c

  • Reported: f1 (wmemcpy) and f2 (wmemcmp) get bufferAccessOutOfBounds.
  • Not reported: f3 (wmemmove), f4 (wmemset) and f5 (wmemchr) have the same overflow and get nothing.
  • No false positives: the in-bounds ok* cases don't produce any warning.

Automated review of 1367f16 by Claude. The pull request was built and tested locally.

Comment thread cfg/std.cfg
<not-null/>
<not-uninit/>
<minsize type="argvalue" arg="3"/>
<minsize type="argvalue" arg="3" baseType="wchar_t"/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

wmemmove, wmemset and wmemchr in this file, and wmempcpy in cfg/gnu.cfg (lines 662/667), also take a count of wide characters. They still use <minsize type="argvalue" arg="3"/> without baseType="wchar_t", so they have the same false negative. For example, wchar_t a[4]; wmemmove(a, L"abcd", 5); and wmemset(a, L'x', 5); give no warning with this PR. Please fix them here too and add tests.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

I found no new problems. The commit "Fix more functions" fixes the gaps that the earlier review raised: wmemmove, wmemset, wmemchr and wmempcpy now have baseType="wchar_t", and each of them has a test. CheckBufferOverrun multiplies the count by sizeOfType("wchar_t"), so the result follows the platform. On win64, where wchar_t is 2 bytes, the allocated buffer and the count are scaled the same way. I tested fixed arrays, struct members, &a[i] offsets, malloc buffers and std::wstring::data(), both in bounds and out of bounds, and found no false positives.

Testing

  • Build: make -j8 cppcheck testrunner in pr/ succeeded. I did not build base/, because none of the comparisons needed it.
  • testrunner: ./testrunner TestBufferOverrun TestLibrary passed with 0 failures.
  • cfg tests: I ran test/cfg/std.c, std.cpp and gnu.c with --check-library --inline-suppr --error-exitcode=1 (adding posix,gnu for gnu.c). The exit code was 1 only because of unmatched valueFlowBailout*, autoNoType and missingIncludeSystem suppressions plus a few others in gnu.c. That happens because I ran without the --debug-warnings that runtests.sh passes, and none of these warnings are on lines this PR touches. All of the new bufferAccessOutOfBounds suppressions match.
  • Own tests (scratch/t.c, scratch/t.cpp), run with ./cppcheck -q --library=std --library=gnu and again with --platform=win64:
    • Reported (correct): these overflows were reported on both platforms:
      • wmemset(p,0,5) on malloc(4*sizeof(wchar_t))
      • wmemset(s->x,0,5) with wchar_t x[4]
      • wmemset(&a[4],0,5) with wchar_t a[8]
      • wmemcpy(a, t.data(), 6) where t is std::wstring(L"abcd") (C++ test, default platform only)
    • Not reported (correct): the in-bounds versions of these calls gave no warning. This includes malloc(16) followed by wmemset(p,0,4), and long wide string literals passed as the source.
    • Missed, but not a regression: a short wide string literal as the source, as in wmemcpy(d, L"abc", 5), is still not reported. This is the same as before the PR.

Automated review of 12ac67f by Claude. The pull request was built and tested locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants