I am an intermediate C programmer. If you have made any coding mistake that you came to know later that it was the most hazardous / harmful to the total application please share that code or description. I want to know this because in future I may come across such situations and I want to have your advice to avoid such mistakes.

+24  A: 
if (c = 1) // insert code here
+1 - there is an idiom you might see around (1 == c) that people use to avoid this error. It's one of the classics.
Upvoted - a very common mistake. Newer compilers will warn about it. Also, one can avoid it by putting the constant on the left, as in "if (1 == c)", which will make a typo like this an error.
Sherm Pendley
D'oh! That'll teach me to read the other comments before adding my own. :-)
Sherm Pendley
I often hear the statement that this is a common mistake, but almost never find people (other than online) who have actually made it. I, for one, find (1 == c) to be terrible on the readability scale and avoid using it like the plague.
@DocMax i don't like it as well, especially taking in account that compiler will warn about it (at least compilers i used).
On the 'constant on left of equality` idiom, older compilers, lint, etc did not pick this up. Many, MANY hours have been lost trying to find this type of bug. It is counter-intuitive to read at first, but I find them about the same after 10+ years of C and C-like language development. I use it.
Ken Gentle
Is there a "standard" answer on how to address (x = y) vs. (x == y) when both are variables? I've seen none.
DocMax, i find and fix this kind of error every other month. And sometimes it's the other way around, "==" instead of "=" :)
It is sad where lots of time in software engineering has gone. Finding out wrong operators, misplaced spaces, converting datetimes, locales, and so on...
I don't like (1 == c) either, but I have written (c = 1) accidentally a few times. I'm glad nobody's life depended on it.
Mike Dunlavey
I do the same thing sometimes, even on the web dev side. PHP loooooves infinite loops.
+4  A: 
for(int i = 0; i<10; ++i)
  //code here
  //code added later

Note that the later added code is not in the for loop.

so flower brackets should be provided for every for
Manoj Doubts
true. That's what i'm doing now.
No, you should switch to python where indenting controls flow :-).
@Pax I think that indentation as a scope declaration is possibly one of the worst features of python esp. when you're reading it away from the screen.
@stephbu in practice, it has never caused me any problems and I've come to like it now.

One thing to look after are array bounds. If you go out of bounds, with bad luck you may end up overwriting memory that is used for other data.

One nasty bug related to this was going out of bounds for a static array variable in a function. That ended up as a function changing values of the local variables of the calling function. That wasn't so straight-forward to debug..

+11  A: 

Few years ago I've got a call from my ex-colleague telling me about the problem he had to fix with my code, which was a router for credit card transactions.

Card number prefix consists of 6-digit BIN (Bank Identification Number) and extra few digits that banks use at own discretion, e.g. bank has BIN for Visa Classic card 456789, and reserve 2 extra digits to indicate sub-product, like 01 for student's card, 02 for co-branded card with local department store and so on. In this case card prefix, which is basically product identifier, becomes 8 digits long. When I coded this part, I decided that 9 digits "ought to be enough for everyone". I was running ok for 2 years until one day bank make a new card products with 10-digit-long prefix (have no idea why they needed it). Not too hard to imagine what has happened - router segfaulted, the whole system halted because it cannot function without transaction router, all ATMs of that bank (one of biggest in the country) became non-operational for few hours, until problem was found and fixed.

I cannot post the code here firstly because I don't have it and secondly it is copyrighted by the company, but it is not hard to imagine the strcpy() without checking size of target buffer.

Just like man strcpy says:

If the destination string of a strcpy() is not large enough (that is, if the programmer was stupid or lazy, and failed to check the size before copying) then anything might happen. Overflowing fixed length strings is a favorite cracker technique.

I was very embarrassed. It was a good time to commit seppuku :)

But I learned the lesson well and do not forget (usually :) ) to check size of target buffer. I wouldn't recommend you to learn it the hard way - just develop a habit to check target buffer before strcpy() and strcat().

Edit: good suggestion from Healthcarel - use strncpy() rather than strcpy(). It doesn't add trailing 0 but I usually use following macro to get around it:

#define STRNCPY(A,B,C) do {strncpy(A,B,C); A[C] = 0; } while (0)

That's why i always use strncpy as suggested in my man strcpy.
@Heathcarel, a function that omits the null-termination, if the buffer is too small isn't much better. Better use strlcpy or snprintf().
The strlcpy/g_strlcpy-family from BSD and glib is useful here -- they always take a destination buffer size and won't write beyond it.
Ben Combee
+10  A: 

It has been a long time, but some things you never forget ;-).

  • forget the \0 at the end of a string.
  • allocate n characters for a string with n characters.
  • forgetting the break in a switch statement.
  • 'creative' macro use.
What's the point of slash and zero at the end of string? :)
No point. We like to have endless texts ;-).
@Constantin, C-strings (well char* ) are null terminated .. and \0 is the null terminator escape code.
Robert Paulson
+1  A: 

Using non-limited string functions such as strcpy() or strcmp(), instead of safe versions like strncpy() and strncmp().

Sherm Pendley
do you mean strcat instead of strcmp? Because strcmp is safe as long as your strings are null-terminated, which should always the case when you use strings.
It *should* always be the case. But it's not safe to assume that should == will, especially if you're comparing strings that some other code is passing to yours.
Sherm Pendley
well then you can add all str* functions to your list, because they all assume that the strings are null-terminated.
It's meant as an example, not an exhaustive list.
Sherm Pendley
+1  A: 

Passing virtual address to the DMA engine was a worst one, not exactly C related, but i assume that 99% of DMA related stuff written in C so it's kind of match. This small error lead to memory corruption that took me 1.5 month to find.

+3  A: 

You should worry more about little mistakes. Big/spectacular mistakes are usually documented in books (with reasons why they are bad, alternative approaches etc.).

It's the little design/coding mistakes that get to you, because they tend to add up.

So my advice is to try and read the books Kernighan wrote or co-authored ("The C Programming Language", "Practice of Programming" etc.) because they are full of common-sense (common for experienced C programmers) advice and list principles that are very useful in avoiding both small and big mistakes.

They also list many potential big mistakes, so they answer your initial question.

Miron Brezuleanu
+13  A: 
if(a == true);
  //Do sth when it is true. But it is allways executed.

Edit: Another variant of the same mistake.

for(i=0; i<max_iterations;i++);
  //Do sth but unexpectedly only once
Fernando Miguélez
ohhohhooo... i took 2 minutes to find that semicolon there.really.this is the funniest mistake and dangerous too. this is why i am addicted to stackoverflow. i got really good examples thank u once again
Manoj Doubts
I still don't see why the compiler just doesn't flag that as an error... if(condition); warning/error: statement/block missing.
It does flag a warning on VS2008 default settings ...but not everyone takes warnings seriously.
Vulcan Eager
There's no statement missing, really; it's just the empty statement. After that, there's a block.
The Digital Mars C compiler does a warning for these, and the D programming language makes them out and out errors. (To have a null statement, use { } )
Walter Bright
+1 for being really hard to spot!
+2  A: 

The most dangerous thing I ever did in C was trying to write code which managed my own memory. Effectively, this means the most dangerous thing I ever did in C was write C code. (I hear that you can get around it these days. Hip hip for sanity. Use those approaches whenever appropriate!)

  • I don't write paging algorithms -- OS geeks do that for me.
  • I don't write database caching schemes -- database geeks do that for me.
  • I don't build L2 processor caches -- hardware geeks do that for me.

And I do not manage memory.

Someone else manages my memory for me -- someone who can design better than I can, and test better than I can, and code better than I can, and patch when they make critical security-compromising mistakes which only get noticed 10 years later because absolutely everyone who attempts to allocate memory fails some of the time.

Patrick McKenzie
+2  A: 
+2  A: 

I take the definition of dangerous as "we may ship with that bug and discover only years later when it's to late":

char* c = malloc(...);
c[...] = ...;


// char* s is an input string
char* c = malloc(strlen(s));
strcpy(c, s);

But if you write multiplatform (not limited to x86/x64) this is also great:

char* c = ...;
int i = *((int*)c); // <-- alignment fault

And if your buffer comes from an untrusted source.. basically most code around is dangerous.

But, anyway, in C it's so easy to shoot yourself in the foot, that a topic about shot feet could go around the thousands of pages.

Marco M.
+2  A: 
{ //code - where 'a' never reaches 0 :( }
How is this dangerous? Infinite loops almost always get detected and fixed quickly compared to other mistakes.
This one reminds an infinitive main loop for micro-controllers...
+1  A: 

switch case with no break.


I remember two blunders:

  1. returning address of an auto variable from within a function in which it was created;
  2. copying string to an uninitialised and unallocated pointer to char.
+3  A: 

Uninitialized data.

Walter Bright
+3  A: 

system() with some user-supplied string in the argument. Same goes for popen().

Use exec*() instead.

Of course, this is not unique to C.

Mitch Haile
Not on Windows! Quoth MSDN:"Spaces embedded in strings may cause unexpected behavior; for example, passing _exec the string "hi there" will result in the new process getting two arguments, "hi" and "there". ... You can avoid this by quoting the string: "\"hi there\""."
Simon Buchan
Sounds bad, what if the user supplied string contains a quote?
Mitch Haile
If the user supplied string contains a quote, there’s no correct thing to do. Command instantiation and argument handling on Windows is a disaster. Calls to system() and exec() are mapped to CreateProcess(), and if you look at the documentation for that function you’ll see it takes a single argument for the whole command line, not separate parameters. That whole command line is passed to the new process, and parameters must be separated out by the application, so theoretically the argument handling behaviour can differ between applications.
Daniel Cassidy
+1  A: 

I'm in agreement with Pat Mac here (despite his downvoting). The most dangerous thing you can do in C is simply to use it for something important.

For example, a sensible language would by default check array bounds and stop your program immediately (raise an exception or something) if you try to wander outside of it. Ada does this. Java does this. Tons of other languages do this. Not C. There are entire hacking industries built around this flaw in the language.

One personal experience with this. I worked with a company that ran a network of flight simulators, tied together with reflective (shared) memory hardware. They were having a nasty crash bug they couldn't track down, so our two best engineers got sent up there to track it down. It took them 2 months.

It turned out that there was an off-by-one error in a C loop on one of the machines. A competent language would have stopped things right there of course, but C let it go ahead and write a piece of data in the next location past the end of the array. This memory location happened to be used by another machine on the network which passed it to a third machine, which used the (garbage) value as an array index. Since this system was also coded in C, it didn't care either that it was indexing way outside of its array, and trashing semi-random memory locations in its program.

So, due to a lack of array bounds checking, a simple easy to make off-by-one bug caused random crashes in a computer two entire hops away from the source of the bug! Cost to the company: 4 man-months of their best engineers' time, plus however much was spent by other engineers and support personnel, plus all the downtime from the simulators in question not working right.

And Ada and Java were written in what language?
Simon Buchan
I can't speak for Java, but I know most compilers are written in the very language they compile (the process is called bootstrapping). If you don't believe me, go check out the code for gcc-ada. It's in Ada.
I don't think C is a bad language (I love it), it's just a dangerous language that doesn't have training wheels. Too many students today go straight to Java/.NET without understanding it. I'm still surprised though at the amount of new critical systems being written in C++.
C fans like to talk about "training wheels". I think of it more like C being a chainsaw with no guards, chain brakes, chain catcher, or safety throttle, and an old-fashioned kickback-prone chain.
#include <string>

Me thinking C supports string natively (using Metroworks codewarrior, about 8 years ago).

I did this, for a final project of about 15,000 lines of code. I used this library to do everything relating to strings(appending, splitting, etc.) Only to have the TA's not able to compile any bit of my assignment(using GCC.)

Little did i learn that metroworks had created their own string library. I failed that class.

+1  A: 

Two things comes to mind. Ths first was a function in embedded C (MCU) i tried to have some restrictions on an timer value as a entered a function. so I wrote

if(55000 < my_var < 65000)

My ida was to check like this:

if( (55000<my_var) < 65000)

But this is the equivilant or the result

if( (55000<my_var) || (my_var<65000))

and the result ended up that the if test was always true.

The secound was a pointer mistake. (simplyfied presented here)

get_data(BYTE **dataptr)
  ubyte* data = malloc(10);
  ... code ...
  *dataptr = &data[1];

   BYTE *data

Thus resulting in a loss of 1 byte of memory for each time the get_data() function was called

I am afraid you wrote another mistake here. "But this is the equivilant". No, it is not. This is equivalent to (55000 < my_var) < 65000, which is comparing a boolean to 65000 - which always yields true.
Not to mention I doubt free would work with an offset from the original pointer. It was probably failing outright, loosing you the 10 bytes + whatever overhead the heap had (~32 on most isn't it?)
Simon Buchan
Suma : I stand corrected... Edit now.
Simon Buchan: Depends on the system you are working in. My linux system did not catch this but I an not running any large memory manager. So I ended up consumin all available memory.
if (importantvar = importantfunction() == VALID_CODE)

This is when I meant this:

if ((important var = importantfunction()) == VALID_CODE)

This led to many hours of debugging troubles when I assumed it worked like the latter.


Having been a Lisp programmer, I was used to indenting closing braces, as in:

    ((eq a foo)(bar ...

and I carried this into C programming:

if (a == foo){

Then I got on a good-size project in C, and another programmer had to make changes in the vicinity of my code. He mis-read my closing brackets, and freed some memory too early. This caused an extremely subtle bug that happened at crunch time. When it was found, he got blamed, badly. But you could say it was my fault. That was not fun, to say the least.

Mike Dunlavey
This is exactly why you should try to match the coding style already in use in whatever project you’re working on.
Daniel Cassidy
+1  A: 

Forgetting architecture constraints and happily memcpy()ing into the memory mapped I/O area on a microcontroller. The magic smoke was released from the test rig.

Mihai Limbășan
+1  A: 

I was dealing with dynamically allocated 2D arrays and I instead of free()'ing N rows, I decided to free M columns. This was fine for smaller inputs where N == M but on large inputs, I only free()'d 50% of what I allocated.


Live and learn.

Nick Presta