String error-checking

Question

I am using a lot of string functions like strncpy, strncat, sprintf etc. in my code. I know there are better alternatives to these, but I was handed over an old project, where these functions were used, so I have to stick with them for compatibility and consistency. My supervisor is very fussy about error checking and robustness, and insists that I check for buffer-overflow violations everytime I use these functions. This has created a lot of if-else statements in my code, which do not look pretty. My question is, is it really necessary to check for overflow everytime I call one of these functions? Even if I know that a buffer overflow can't possibly occur e.g. when storing an integer in a string using the sprintf function

sprintf(buf,"%d",someInteger);

I know that the maximum length of an unsigned integer on a 64-bit system can be 20 digits. buf on the other hand is well over 20 characters long. Should I still check for buffer overflow in this case?

Answer 1

I think the way to go is using exceptions. Exceptions are very useful when you must decouple the normal control-flow of a program and error-checking.

What you can do is create a wrapper for every string function in which you perform the error checking and throw an exception if a buffer overflow would occur.

Then, in your client code, you can simply call your wrappers inside a try block, and then check for exceptions and return error codes inside the catch block.

Sample code (not tested):

int sprintf_wrapper( char *buffer, int buffer_size, const char *format, ... )
{
    if( /* check for buffer overflow */ )
        throw my_buffer_exception;

    va_list  arg_ptr;
    va_start( arg_ptr, format );
    int ret = sprintf( buffer, , format, arg_ptr );
    va_end(arg_ptr);
    return ret;
}

Error foo()
{
    //...
    try{
        sprintf_wrapper(buf1, 100, "%d", i1);
        sprintf_wrapper(buf2, 100, "%d", i2);
        sprintf_wrapper(buf3, 100, "%d", i3);
    }
    catch( my_buffer_exception& )
    {
        return err_code;
    }
}

Answer 2

Maybe write a test case that you can invoke to simply test the buffer to reduce code duplication and ugliness.

You could abstract the if/else statements into a method of another class, and then pass in the buffer and length expected.

By nature, these buffers are VERY susceptible to overwrites, so be careful ANYTIME you take input in from a user/outside source. You could also try getting a string length (using strlen), or checking for the /0 end string character yourself, and comparing that to the buffer size. If you loop for the /0 character,and it's not there, you will get into an infinite loop if you don't constrain the max size of your loop by the expected buffer size, so check for this too.

Another option, is to refactor code, such that every time those methods are used, you replace them with a length safe version you write, where it calls a method with those checks already in place (but have to pass the buffer size to it). This may not be possible for some projects, as the complexity may be very hard to unit test.

Answer 3

Let me address your last paragraph first: You write code once, in contrast to how long it will be maintained and used. Guess how long you think your code will be in use, and then multiply that by 10-20 to figure out how long it will actually be in use. At the end of that window it's completely likely that an integer could be much bigger and overflow you buffer, so yes you must do buffer checking.

Given that you have a few options:

• Use the "n" series of functions like snprintf to prevent buffer overflows and tell your users that it's undefined what will happen if the buffers overflow.

• Consider it fatal and either abort() or throw an uncaught exception when a length violation occurs.

• Try to notify the user there's a problem and either abort the operation or attempt to let the user modify input and retry.

The first two approaches are definitely going to be easier to implement and maintain because you don't have to worry about getting the right information back to the user in a reasonable way. In any of the cases you could most likely factored into a function as suggested in other answers.

Finally let me say since you've tagged this question C++ and not C, think long and hard about slowly migrating your code base to C++ (because your code base is C right now) and utilize the C++ facilities which then totally remove the need for these buffer checks, as it will happen automatically for you.