Skip to content

Fix #15195: False negative: bufferAccessOutOfBounds with address of member - #9012

Open
ludviggunne wants to merge 2 commits into
cppcheck-opensource:mainfrom
ludviggunne:15195-buf-memb-ptr
Open

ludviggunne wants to merge 2 commits into
cppcheck-opensource:mainfrom
ludviggunne:15195-buf-memb-ptr

Conversation

@ludviggunne

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

The PR fixes the reported false negative, and its tests pass. It also adds a new class of bufferAccessOutOfBounds errors on a common C idiom: clearing or copying a struct through the address of its first (or a later) scalar member, e.g. memset(&s.first, 0, sizeof(s)) or memset(&p->second, 0, sizeof(*p) - sizeof(int)). Strictly speaking these go past the subobject, but the memory written is inside the enclosing object. Before the PR there was no warning, and the new message is error severity. Maintainers should decide whether this is wanted. If not, the unwrapping could be limited to members that are arrays, or the size of the outermost object could be used. Among the true positives I tried I found no wrong results, crashes or hangs.

Testing

  • Built cppcheck and testrunner in pr/ and cppcheck in base/.
  • ./testrunner TestBufferOverrun: 201 tests, 0 failed.
  • I compared PR and base on scratch files.

True positives that are new and correct:

  • memcpy(d,&s.a,16)
  • &v->a[1] with char a[4][4] and 16 bytes
  • &v->arr[1].a
  • &ps->a[2] with 4 bytes
  • strcpy(&s.a,"abcdef")
  • &N::buf

Correctly not reported:

  • char data[1] trailing members reached through a pointer (&u->data, &u->data[0])
  • a non-constant index &v->a[i]

New errors on the idiom of writing through a member's address (no output from base):

#include <cstring>
struct S { int first; int second; char a[4]; void clr(); };
void S::clr() { memset(&this->first, 0, sizeof(*this)); }
void g1(S &s) { memset(&s.first, 0, sizeof(S)); }
void g3(S *s) { memset(&s->second, 0, sizeof(S) - sizeof(int)); }
struct T { S s; int x; };
void f3(T *t) { memset(&t->s, 0, sizeof(*t)); }

Command: pr/cppcheck -q t2.cpp. Output with the PR:

t2.cpp:3:24: error: Buffer is accessed out of bounds: &this->first [bufferAccessOutOfBounds]
t2.cpp:4:24: error: Buffer is accessed out of bounds: &s.first [bufferAccessOutOfBounds]
t2.cpp:5:24: error: Buffer is accessed out of bounds: &s->second [bufferAccessOutOfBounds]

f3 (which was in a separate scratch file) gives error: Buffer is accessed out of bounds: &t->s. Base prints nothing for any of these.


Automated review of 76406a8 by Claude. The pull request was built and tested locally.

}
bufTok = bufTok->astOperand1();
}
while (Token::Match(bufTok, ".|::"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI review

This unwraps &x.m to m for every kind of member, scalars and struct members included. So there are now error reports on the common idiom of writing the enclosing object through the address of a member:

struct S { int first; int second; };
void g1(S &s) { memset(&s.first, 0, sizeof(S)); }               // error: &s.first
void g3(S *s) { memset(&s->second, 0, sizeof(S) - sizeof(int)); } // error: &s->second
void S2::clr() { memset(&this->first, 0, sizeof(*this)); }       // error

Base reported none of these. The ticket is about array members, so consider unwrapping only when the member has array dimensions, or skipping the case where the access stays inside the outer object.

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.

2 participants