Writing to a zone with insufficient space yields memory overrun

[Iulian Serbanoiu]

I had the following in some legacy code and neither cppcheck or coverity found this issue.

#include <sys/msg.h>

int main() {
  int msqid = 2;
  struct msqid_ds *buf;

   msgctl(msqid, IPC_STAT, (struct msqid_ds*)&buf);

  return 0;
}

The problem is the fact that it tries to write an entire structure (tens of bytes) in the space held by a pointer (8 bytes on 64 bit linux).

Is this feasable to be implemented in cppcheck?

[Daniel Marjamäki]

it sounds interesting.

However a checker might need to understand what the first and second arguments for msgctl does exactly and we don't like to hard code such specific knowledge about each function.

It's not possible to say that the 3rd argument for msgctl must always point at data that is larger than 8 bytes right? No matter what the first and second arguments are.

I wonder how much noise we would get if we warn whenever some struct sometype** is casted to struct sometype*

[Daniel Marjamäki]

As you say a pointer could be 8 bytes on a platform.

I am thinking about a warning for the cast. How we can motivate that and if there are some heuristics we can use to avoid noise.

Let's say 'struct sometype*' is casted to 'struct sometype' in a function call like in your example.

If sometype is not 8 bytes then the cast is suspicious it means that the pointed data has incompatible size and then reads/writes are rather dangerous. For instance if Cppcheck checks the struct size and finds that it is 2 bytes then I guess we could write a warning like "function expects a 'struct sometype *' (size of sometype is 2) but the provided argument points at a 8-byte pointer. If the function writes data it will likely result in an invalid address (undefined behavior). If the function only reads data then it's also suspicious, a few bytes in a address is nonsense data.

[Iulian Serbanoiu]

I don't know the exact capabilities of cppcheck related to determining structure sizes defined but as long as we do the following:

1. check the function prototype
2. see the size of each parameter passed as pointer
3. compare size of parameter to the required size
4. issue
   1. an error in case the size of the varable is less than required
   2. a warning in case the variable is more than required (possibly uninitialized data because of incomplete writing)

This issue was very hard to discover because it was writing to a static variable which made writing to the data segment in places no one could think of. This apparently worked well in older releases, probably by luck, but in the new release we were simply stunned for about 1 week without any real idea on how to progress, especially since the code didn't suffer any changes.

This is my motivation for reporting this to any tool that help development.

[Daniel Marjamäki]

the problem is mostly for cppcheck to determine "required size". for msgqtl it's hard because it magically depends on what integer value parameter #2 has.

whenever the required datasize is provided in a parameter it should be possible to detect it with cppcheck:

void foo() {
     int buf[10];
     memset(buf, 0, 1000);
}

Cppcheck output:

1.c:5:13: error: Buffer is accessed out of bounds: buf [bufferAccessOutOfBounds]

However I can see there are false negatives when a struct is written :-( We should of course also warn here:

struct XY { int x; int y; };
void foo() {
     struct XY xy;
     memset(&xy, 0, 1000);
}

[Iulian Serbanoiu]

the problem is mostly for cppcheck to determine "required size".
for msgqtl it's hard because it magically depends on what integer value parameter #2 has.

I would say that this is not the case here, namely we just care about the structure parameters passed as pointers (assuming we want to store or read values, as I don't see other purpose). That being said if we don't pass NULL (case in which we don't care as it will either crash, or work properly based on the function parameters) we need to make sure that the size of the structure required in the prototype (via a pointer) is matched by the size of the parameter (if less bytes -> serious error, if more bytes -> warning)

[Daniel Marjamäki]

I have added a warning to the cppcheck premium that warns about such cast:

(struct A*)&expr;

if expr is some pointer expression. When I test it out on Debian code it does not write very much warnings and those that are reported seem reasonable to me. It allows casts to/from void pointers though.

Test case:

struct A { char buf[10]; };
void f() {
    struct A *a;
    dostuff((struct A*)&a);
}

=> tmp1.cpp:4:13:style: Suspicious cast, loss of pointer depth. Casting from 'A * *' to 'A *'. Expression '&a' uses address-of operator, should address-of operator be removed? [pointerCastLossOfIndirectionAddressOf]

Right now the warning is only written if a address-of result is casted.. but I think it should be interesting to test with more general warnings also.

If you would like to try it out let me know.

[Iulian Serbanoiu]

Well I hope this makes it to the code in the future. I just wanted to report it.

Thanks!

[Jens Yllman]

For me the size check is not the most important here. Taking a pointer to a pointer is somewhat suspicious to me. And also the cast, especially since it is a cast from ptr* to ptr. But of curse the size also makes it more suspicious.