Wednesday 12 November 2008

Hey, it is only a warning. How important can it be?

Caveat – only of interest to C or C++ devs today.

You might think that compiler warnings are just nagging. Well, that is mostly true. If you are in a relationship, you may well have been nagged to do the washing up or empty the kitchen bin at some point. Some nagging has a point.

I am going to be talking here about the Microsoft compilers because those are the ones that I know best but the same principles apply to other compilers and even code checkers like Lint. Ah, those happy days when we could name tools in ways that amused us. Lint picked fluffs from your code and you used DDT to kill bugs. Anyway, I digress. Compilers allow you to set the warning level that they compile your code against. If you do certain things, you will get warned. I want to talk about some of those warnings.

So, let us look at one that it is probably OK to ignore:

Compiler Warning (level 4) C4201
Error Message : nonstandard extension used : nameless struct/union

Ok, this just means that you have used something which is not supported by ANSI C++. Maybe you need this to be multiplatform in which case that is probably a bad thing. Maybe you plan to change compiler at some point in the future (which I only recommend for masochists) and you want the code to stay as portable as possible. Maybe your contract demands that you use ANSI level C++ for compliance reasons. This is a minor warning but there are some pretty good reasons for at least considering what it is telling you.

How about one that we should worry about?

Compiler Warning (level 3) C4018
Error Message: 'expression' : signed/unsigned mismatch

This one has some brothers and sisters but they have the same basic pattern. You treat something as a signed and an unsigned value. Ah, but you know that the value will only ever be 0 to 40 and so what does it matter? Well, quite a bit. Let me explain how.

Imagine that we have an application that reads a data file and makes sense of it. There are millions of applications like that. So, the data is coming from a file. Further imagine that we have a buffer which is 100 bytes long – it is char[100] so element 0 to element 99 are fine. We are going to fill it from a structure that has been passed to us. You have an integer which holds the length of the buffer and a pointer to part of the file. You check that the length is less than 101. Yes it is. You read that many bytes and copy that many bytes into the array. You go on and do the next thing. All is well and there are millions of bits of code that do just that.

Why do you check the length? Because you don’t want to overflow the buffer. However, what happens if the length that is read from the file is -10,000 rather than 42, for example? Well, -10,000 is less than 100 so that check works fine. The routine that reads the file takes an unsigned value so -10,000 becomes DF80, a much larger number, 55536 to be precise. So, you read 55536 bytes from the file and copy it into the 100 byte array. Oops, that is the stack gone. If you are lucky, you will crash and your user will curse your name. However, that could only happen with a corrupt file since you also write the files and there are never negative lengths in there. It is, accordingly, a purely theoretical risk right up until someone writes a malicious file and mails it to your customer. Odds on, this will be remote code execution vulnerability. It happened with dozens of products including Abobe, Microsoft and many other household names. Linux and Unix have both had this one over and over and smarter people than me missed it.

My recommendation is that you compile all production code at the maximum warning level and document any warnings that you can’t get rid of. I would even go so far as to say that compiler warnings should be logged as bugs so that they get fixed in the next version. You might think otherwise and that is your right... and I will sell you or your client my services when you or they hot problems as a result

Signing off

Mark Long, Digital Looking Glass

No comments: