Peeter Joot's (OLD) Blog.

Math, physics, perl, and programming obscurity.

ease of getting strncat wrong.

Posted by peeterjoot on December 5, 2012

I’d done a test build with the clang compiler of our db2 code and fired off some defects and emails to some of the code owners about messages from that compiler like:

foo.C:1220:15: warning: the value of the size argument in ‘strncat’ is too large, might lead to a buffer overflow [-Wstrncat-size]

It’s a great message, clear and unambiguous: “your code is wrong”!

Here’s a sample of the code in question that clang was complaining about:

   strncat( dirPath, ptrToDirectories, ( sizeof( dirPath ) - strlen( dirPath ) ) );

The compiler could just as well complain about simpler strncat calls.  Imagine at first that dirPath was all zeros, then the above code would be equivalent to:

   strncat( dirPath, ptrToDirectories, sizeof( dirPath ) ) ;

It’s not immediately obvious reading the strncat man page that this is wrong, since that man page says “[strncat] will use at most n characters from src”. However, it also says “As with strcat(), the resulting string in dest is always null terminated.”

Does this mean that it will use no more than n characters from src, but that it null terminates within the range of those n-characters, or does this mean use (if strlen(src) > n) n characters from src and then add a null terminator after that?

To answer that question, and rule out a possible false positive warning from the compiler I wrote the following simple bit of test code:

#include <string.h>
#include <stdio.h>

struct string
{
   char dest[2] ;
   char overflow ;
} ;

int main()
{
   string o ;
   o.dest[0] = 0  ;
   o.dest[1] = 1  ;
   o.overflow = 1 ;

   printf( "%d\n", (int)o.overflow ) ;

   strncat( o.dest, "abc", sizeof(o.dest) ) ;

   printf( "%d\n", (int)o.overflow ) ;

   return 0 ;
}

And the verdict was:

$ g++ -g t.C ; a.out
1
0

Yup, the code was bad, and the compiler is without fault. We actually have a different function for strncat’ing in our code that takes the actual buffer size, to make this harder to get wrong, but the code in question was not using that.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

 
%d bloggers like this: