Incorrect memory allocation

[mkiran]

I am looking to have a portable code (which works in both Pelles C IDE and in the Linux box).
When I use below code (code snippet below) in Pelles C, it works most of the time (Not yet found the failure).
And when I take it to Linux environment, it fails with Segmentation fault.

To my guess, due to a vague coincidence, this problem is not noticed in Pelles environment.
My intent of this message is, what is that helping not showing in Pelles C ?
(One solution as I know is that allocate one more character space to hold end character.
But, I am looking to have this code to work in Linux, since it is working in Pelles C IDE.)

Code snippet.
struct node *f1(char *linebuf)
{
             char *pdest = NULL;

   pdest=linebuf;
   char *tmp=NULL;
   //tmp is used to hold linebuf
   tmp = (char *)calloc(strlen(linebuf), sizeof(char));
   strcpy(tmp,linebuf);
...
...
...
}

[severach]

(One solution as I know is that allocate one more character space to hold end character.

If you don't allocate the extra byte for the \0 then it is a buffer overflow bug. Linux must be storing the allocator links at the end allowing the the \0 off the end which crashed the allocator or maybe it's doing simple bounds checking and voluntarily crashing. A real page fault on strcpy() is unlikely since the memory would need to fall at the end of a page boundry to be caught. I wrote my own malloc-safe library with bounds checking, forgot to free pointers checking, and using pointers after free checking to find this kind of error. It was easy here but it's not so easy when the program gets complicated.

Code: [Select]

tmp = (char *)calloc(strlen(linebuf)+1, sizeof(char));
Use malloc instead of calloc. No reason to zero the memory if you're just going to write over it.

Code: [Select]

tmp = (char *)malloc((strlen(linebuf)+1)*sizeof(char));
Don't use sizeof(char) in the malloc. This will be wrong if tmp changes width in the future. The compiler generated code is just as efficient. Always use the destination variable unless you have a good reason not to since the source can be a different width during a width conversion.

Code: [Select]

tmp = (char *)malloc((strlen(linebuf)+1)*sizeof(*tmp)); // do not use sizeof(*linebuf)
(Jedi hand wave)  I think strdup() is the function you're looking for.

But, I am looking to have this code to work in Linux, since it is working in Pelles C IDE.)

strdup() is portable.

[mkiran]

Thanks for bringing out nicely different ways of allocating memory.
Ok..let me more specific about my question at this point of time.

1. Is there at anytime the code snippet fails in Pelles C.
(And it is sure that in Linux it fails because it needs one extra byte for \0).
If it not fails in Pelles C at anytime,
what is the memory management technique that is used which is preventing
it from failing. If the technique is just to add one extra byte when strlen() is invoked,
then it would be sure that this code always works in Pelles C.

Of course, to make this portable, I guess I need to change my code. But before concluding,
I want to make sure if the current code snippet always works in Pelles C.

[severach]

The general rule for malloc is that you can only write to the number of bytes you've requested even if the operating system seems to tolerate a little more. Here are my comments from the NPPTextFX project of Notepad++.

/* Windows 98 pads the length of GlobalAlloc() buffers to multiples of 8
   and buffers submitted to the Clipboard to 32. Windows XP and NT4 do not
   pad the buffer size at all. This info will be used to produce a reliable
   binary copy-n-paste */

Pelles clib, Linux clib, MSVCRT.DLL clib may all behave differently. My safe allocator adds 8 test bytes at the beginning and end to find buffer overflows. If your allocator decides to pad the buffer then writing extra bytes will be successful most of the time. If your allocator or other allocations place no important information after your buffer and writable memory exists there writing extra bytes may not cause immediate damage. Your program will crash once in a while which will make the problem hard to find. Crashing ASAP like Linux did is the best way.

If the buffer fell at the end of a page boundary and the next page wasn't allocated Windows would shut you down with a segmentation fault as you wrote the \0. It would be quite a challenge to get your buffer to fall there but it would fall there often enough that people would say your program is unreliable but not often enough to let you easily find where the problem is. Long before I had a safe allocator I wrote a program that crashes every few weeks and only when certain operators are using it and it's never the same steps twice. I've never even tried to fix it. For all the memory allocations I've seen in Pelles I've never seen the allocator links before or after so they must be stored elsewhere. This means you'd need to scorch for quite a ways in either direction to trigger a seg fault and initially you'd only be destroying your own data. This has happened to me in a complex loop that was supposed to stop at 0 but somehow got past. It's a long ways from 0xFFFFFFFF to 0 and quite a surprise when the program disappears under a fog of 0x20. My program didn't stop until the offending instructions overwrote themselves and because of instruction caching probably a ways beyond.

If the technique is just to add one extra byte when strlen() is invoked,
then it would be sure that this code always works in Pelles C.

Doing it right means you get to hassle Pelle if it doesn't work. He's expected to follow the rules but not expected to bend them for you like Microsoft did for Sim City. This tells me you don't yet see why what you're doing is wrong. That one extra byte is needed everywhere including systems where it appears to not make any difference. Maybe I can demonstrate the size of things more clearly.

Code: [Select]

static char string1[]="12345"; // sizeof says 6 bytes, strlen says 5 chars because it doesn't count the \0
char string2[5]; // sizeof says 5 bytes so this string is not long enough to hold string 1
strcpy(string2,string1); // this causes a buffer overflow. Some compilers might pad this to 6 or 8 bytes so the overflow may never cause a crash or damaged data. Whether it can always avoid crashing or not is something out of your control.
char *string3=malloc(sizeof(string1)); // this is big enough for string1
char *string4=malloc((strlen(string1)+1)*sizeof(*string4)); // this is big enough for string1
WCHAR wstring1=L"12345"; // sizeof says 12 bytes, wstrlen says 5 WCHARs
WCHAR wstring2[5]; // sizeof says 10 bytes so this string is not long enough to hold wstring1
WCHAR *wstring4=malloc((strlen(string1)+1)*sizeof(*string4)); // this is big enough for string1 for conversion with MultiByteToWideChar though possibly not for MBCS

Run your bad code in a Linux debugger to see what you are overwriting.

Buffer overflow

[mkiran]

So, it is possible that the code snippet would fail occassionally in Pelles C.

For all the memory allocations I've seen in Pelles I've never seen the allocator links before or after so they must be stored elsewhere.

Could you explain more about your program which crashed. I tried reading that paragraph a couple of times, but could not understand it properly. I guess, if you have time, please explain more about your program- about what made it crash.

Doing it right means you get to hassle Pelle if it doesn't work. He's expected to follow the rules but not expected to bend them for you like Microsoft did for Sim City.

Not at all ...  I was just trying to know more if I can understand it better.

[severach]

Code: [Select]

for(i=10; i>0; ) {--i; ...}A simple for loop like this won't cause any trouble. My for loop was much more complicated.

Code: [Select]

for(i=10; i>0; ) {
  if (condition) {
    --i;
     if (condition) --i;
  }
  if (condition) --i;
}
Depending on the conditions the loop could be decremented any number of times. If the loop is bug free then no set of conditions will allow the counter to go beyond zero. One case I ran found a bug in the loop which let the counter around to 0xFFFFFFFF and beyond eventually overwriting most of memory. After the Pelles-C debugger stopped it there wasn't anything left to see.

I mention this example because it took until the program overwrote it's own instructions before a fault was generated. Not crashing is a poor indicator of correctness.

Pelles debugger lets you watch the action live so you can see what's wrong. Here's your buffer overflow code with some extra lines to make the problem easy to see in the debugger.

Code: [Select]

struct node *f1(char *linebuf) {
   char *pdest = linebuf;
   char *tmp=NULL; //tmp is used to hold linebuf
   size_t cb=strlen(linebuf)*sizeof(*tmp); // buffer overflow here
   tmp = (char *)malloc(cb+2); // pretend that we only asked for cb bytes here
   memset(tmp,0x92,cb+2); memset(tmp,0x90,cb); // breakpoint on this line, select the memory window and type tmp in the box.
   strcpy(tmp,linebuf); // step to this line. tmp is painted with cb 90's and two extra 92's are placed beyond temp for monitoring.
   // step to this line. Code that works will always leave the two 92's at the end intact. Since you have allocated cb one too few strcpy will overwrite one of them.
}[code]
Ask for cb bytes and write to cb+1 bytes, that's a buffer overflow on any system.
[/code]