views:

290

answers:

5

I recently ran into a very sneaky bug, in which I forget to dereference a pointer to a string (char array) and thus sometimes overwrote one byte on the stack.

Bad:

char ** str;
(*str) = malloc(10);
...
str[2] = 'a'; //overwrites 3 bytes from the location in which str is stored

Corrected:

char ** str;
(*str) = malloc(10);
...
(*str)[2] = 'a'; 

GCC produced no warnings, and this error would've resulted in a very serious and real exploit as the value it sometimes overwrote held the size of a buffer. I only caught this bug because I got luckly and it caused an obvious failure.

  • Other than relying on luck and/or never using C for anything, what defensive coding techniques and tricks do you use to catch wierd C bugs?

  • I'm thinking about moving to valgrind's MemCheck, has anyone used it? I suspect it wouldn't have caught this bug. Anyone know?

  • Are there tools for catching pointer dereferencing or arithmetic bugs? Is that even possible?

UPDATE

Here is the requested example code, it does not throw any warnings.

#include <stdlib.h>

void test(unsigned char** byteArray){
    (*byteArray) = (unsigned char*)malloc(5);
    byteArray[4] = 0x0;
}

int main(void){
    unsigned char* str;
    test(&str);  
    return 0;
}

Compiling causes no errors:

gcc -Wall testBug.c -o testBug

Running causes a seg fault:

./testBug
Segmentation fault

This is the version of GCC I'm using:

gcc -v

Using built-in specs.
Target: i486-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.4.1-4ubuntu9' --with-bugurl=file:///usr/share/doc/gcc-4.4/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --enable-shared --enable-multiarch --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.4 --program-suffix=-4.4 --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-targets=all --disable-werror --with-arch-32=i486 --with-tune=generic --enable-checking=release --build=i486-linux-gnu --host=i486-linux-gnu --target=i486-linux-gnu
Thread model: posix
gcc version 4.4.1 (Ubuntu 4.4.1-4ubuntu9) 
+1  A: 

I use Valgrind, it's a lifesaver!

valgrind --tool=memcheck -v ./yourapp

And MemCheck will detect that you have an invalid write with `str[2] = 'a';´.

tur1ng
+2  A: 

GCC should give you

 warning: assignment makes pointer from integer without a cast

No?

Jimmeh
Sadly, I got no such error. When I encountered this bug I fixed the only warning gcc was generating.
e5
@e5 What version of gcc are you using? It certainly gives me that warning.
anon
gcc 4.4.1, I just tested it, it does not give any warning. Code follows: unsigned char** str; (*str) = (char*)malloc(5); str[5] = 0x0;could it be the fact that I'm casting (char*) to unsigned char*?
e5
@e5 I can't understand from this or from your question which code causes the bug (or do both of them?). The first piece of code you posted in your question, labelled "bad" causes a compilation warning. If that isn't what you are asking about, please clarify your question.
anon
@Neil Butterworth, the code I'm running is essentially that but it is inside of a function in a header file. Maybe somehow that is suppressing the warning. I will write up the exact code I am compiling and post it to the question.
e5
@e5 - that is not giving you a warning because 0x0 is both a valid integer and a valid pointer. Change `str[5] = 0x0;` to `str[5] = 'a';` to see the warning.
R Samuel Klatchko
+1 @R Samuel Klatchko right you are! Well done. I wanted to set the final char to NULL since it was a null terminated string. How can I do that in a safe way?
e5
@R Samuel Klatchko, can you write that up as an answer and I'll mark it answered.
e5
@e5: The character NUL is represented by the escape sequence `'\0'`.
Chuck
@Chuck: that doesn't help - GCC doesn't warn with `'\0'` either. In principle maybe it could, since using `'\0'` to generate a null pointer is odd, but `'\0'` is the exact same type (int) and value (zero) as `0x0` (and `0`). I'm guessing the warning is generated based on the value, not the literal used to generate it.
Steve Jessop
+3  A: 

My best defensive pointer strategy: Strongly avoid using more than one level of indirection. Dereferencing the pointer-to-pointer to assign memory to it is OK. But to then use the assigned memory as an array is asking for trouble, which you got. I would make it something like:

char **outStr;
*outStr = malloc(10);
char *str = *outStr;
str[2] = 10;

OK, actually it's just a keep-my-sanity strategy that happens to have defensive value as well. Pointers are fairly easy to understand when there's never more than one level of indirection at a time, and it's easier to make code work right when you understand it well.

Chuck
A: 

My suggestion is not a tool, but a best practice: testing. Such bugs are usually very easy to find with rigorous testing of the code, beginning at the lowest-level of unit testing.

The code you show would never produce the correct result - it's not something that sometimes work and sometimes doesn't. Having a unit test for that piece of code can save hours of debugging later when it's integrated with other parts of the system.

Unit testing can be supplemented by coverage checking: either using an automatic tool, or just manually scanning the code and writing tests that target each part - this is actually a great way to re-read your code (another debugging tool) and is surprisingly effective.

Eli Bendersky
I totally agree that a good test code will help a lot. But, running valgring on the good test code will double check your code and keep it perfect.
Jay
Unittesting is great, I'm all for, it but it likely would not have caught this bug since the bug was only visible when two components (units) were integrated. Furthmore if the overwriting value increased the overwritten value rather than decreased it, no failure mode would've be activated.
e5
+1  A: 

Please use Valgrind. It's one of the best memory checking tools I have come across. It will surely detect your error.

Apart from detecting memory errors, valgrind also helps in detecting memory leaks, in-use memory blocks etc.

Even IBM Rational Purify will help you in detecting such errors. Though my personal favourite is Valgrind.

Jay