Consequences of this buffer overflow?

Question

So here I believe I have a small buffer overflow problem I found when reviewing someone else's code. It immediately struck me as incorrect, and potentially dangerous, but admittedly I couldn't explain the ACTUAL consequences of this "mistake", if any.

I had written up a test app to demonstrate the error, but found (to my dismay) that it seems to run correctly regardless of the overflow. I want to believe that this is just by chance, but wanted some feedback to determine if my thinking were wrong, or if there truly is a problem here that just isn't showing its head in my test app.

The problem code (I think it is, anyway):

char* buffer = new char[strlen("This string is 27 char long" + 1)];
sprintf(buffer, "This string is 27 char long");

Now, the reason this stood out to me and I want to flag it as a possible buffer overflow is because of the first strlen. Due to pointer arithmetic, the 'incorrect' placement of the + 1 will cause the strlen to return 26 instead of 27 (taking the length of "his string is 27 char long"). sprintf, I believe, then prints 27 char into the buffer and has caused a buffer overflow.

Is that a correct assessment?

I wrote a test app to demonstrate this for the person who's code I was looking at, and found that even in the debugger the string will print correctly. I also attempting putting other variables on the stack and heap before and after this code to see if I could affect neighboring areas of memory, but was still receiving correct output. I realize that my newly allocated heap memory might not be adjacent, which would explain the lack of useful overflow, but I just really wanted to confirm with others' opinions if this is in fact an issue.

Since this is a pretty simple "question", it'd be nice if you could support your answer with some sort of reference as well. While I value and welcome your input, I'm not going to accept "yes it is" as the final answer. Thank you kindly in advance.

---

Update: Many good answers with a lot of additional insight. Unfortunately, I can't accept them all. Thank you for sharing your knowledge and for being my 'second opinion'. I appreciate the help.

Answer 1

Many historic malloc implementations put bookkeeping data immediately before and/or after the allocated block. It's possible that you're overwriting such data, in which case you would not see any error/crash until you try to free the memory (or perhaps free whatever the next block happens to be). Likewise, it's possible that the bookkeeping information for a subsequent allocation will later overwrite your string.

I suspect modern malloc implementations make some effort to protect against heap corruption by padding allocations with integrity-check data, so if you're lucky, nothing bad will happen or you might get a warning message during a later allocation/free operation.

Answer 2

You are correct that pointer arithmetic in this example would produce an incorrect (shorter) length passed to new. The most probable reason why you are not able to make this crash is because there is some uncertainty as to how much buffer space is actually provided by the memory allocation.

The library is allowed to provide a larger buffer than was requested. Furthermore, it is also possible that whatever follows your buffer is prefixed by an allocation header that is subject to machine word alignment rules. This means there could be up to three padding bytes (depending on platform) before the very next allocation header.

Even if you overwrote the next allocation header (which is used to manage the allocated memory blocks) it would not manifest itself as a problem until the owner of that next block attempted to return it to the heap.

Answer 3

The reason the string is printing fine in the debugger is that as part of the sprintf, the trailing NULL character is being written to memory (in this case beyond the buffer you allocated) and when it comes to reading the string the NULL character is present to terminate the string as expected.

The problem is that the byte containing the NULL character hasn't been allocated as part of the original new and so could be used for a different allocation later. In this case, when you come to read the string afterwards you will likely get your original string with garbage appended.

Answer 4

I tried it with heap allocations, variables are not continuous in memory in this case. That is why it is hard to make buffer overflow in this case.

Buy try it with stack overflow

#include "stdio.h"
#include "string.h"

int main()
{
     unsigned int  y      = (0xFFFFFFFF);
     char buffer[strlen("This string is 27 char long" + 1)];
      unsigned int  x      = (0xFFFFFFFF);
      sprintf(buffer, "This string is 27 char long");

      printf("X (%#x) is %#x, Y (%#x) is %#x, buffer '%s' (%#x) \n", &x, x,&y, y, buffer, buffer);
      return 0;
  }

You will see that Y is corrupted.

Answer 5

You assessment is correct, except that the springf will put 28 characters in the buffer counting the end-of-string NUL at the end (That's why you needed the misplaced "+1" in the first place)

Note that in my experiences, if something fails outside of a debugger, but works with stepping through in the debugger, in 100% of the time, you've overrun a local buffer. Debuggers push a lot more onto the stack, so it's less likely the something important was overwritten.