Code cleanup on aisle five…

I recently spent two days at work going through projects to clean up compiler warnings. In GNU C, you can enable options such as “-Wall” (all warnings), “-Wextra” (extra warnings) and “-Werror” (warnings as errors). By doing steps like these, the compiler will scream at you and fail to build code that has warnings in it.

Many of these warnings don’t impact how your code runs. They just ask you “are you sure this is what you are meaning to do?”

For example, if you leave out a “break” in a switch/case block, the compiler can warn you about that:

x = 1;

switch( x )
{
case 1:
printf("x is onen");
// did I mean to not have a break here?

case 2:
printf("x is twon");
break;

default:
printf("I don't know what X isn");
break;
}




This code would print:



x is one
x is two



...because without the "break" in the "case 1", the code drops down to the following case. I found several places in our embedded TCP/IP stack where this was being done intentionally, and the author had left comments like "/* falls through below */" to let future observers know their intent. But, with warnings cranked up, it would no longer build for me, even though it was perfectly fine code working as designed.



I found there was a GCC thing you could do where you put in "//no break" as a comment and it removes that warning. I expect that are many more "yes, I really mean to do this" comments GCC supports, but I have not looked in to it yet.



Size (of int) matters



Another issue I would see would be warnings when you used the wrong specifier in a printf. Code might compile fine without warning on a PC, but generate all kinds of warnings on a different architecture where an "int" might be a different size. For example:



int answer = 42;
printf("The answer is %dn", answer);



On my PC, "%d" can print an "int" type just fine. But, if I had used a "long" data type, it would error out:



long answer = 42;
printf("The answer is %dn", answer);



This produces this warning/error:



error: format '%d' expects argument of type 'int', but argument 2 has type 'long int' [-Werror=format=]|



You need to use the "l" (long) specifier ("%ld") to be correct:



long answer = 42;
printf("The answer is %ldn", answer);



I found that code that compiled without warnings on the PC would not do the same on one of my embedded target devices.



%u versus %d: Fight!



Another warning I had to deal with was printf() and using "%d" versus "%u". Most code I see always uses %d, which is for a signed value which can be positive or negative. It seems works just fine is you print an unsigned integer type:



unsigned int z;

z = 100;
printf("z is %dn", z);



Even though the data type for z is unsigned, the value is positive so it prints out a positive number. After all, a signed value can be positive.



But, it is more correct to use "%u" when printing unsigned values. And, here is an example of why it is important to use the proper specifier... Consider this:



#include <limits.h> // for UINT_MAX

unsigned int x;

x = UINT_MAX; // largest unsigned int

printf("x using %%d is %dn", x);
printf("x using %%u is %un", x);



This prints:



x using %d is -1
x using %u is 4294967295



In this case, %d is not giving you what you expect. For a 32-bit int (in this example), ULONG_MAX of 4294967295 is all bits set:



11111111 11111111 11111111 11111111



That represents a -1 if the value was a signed integer, and that's what %d is told it is. Thus, while %d works fine for smaller values, any value large enough to set that end bit (that represents a negative value for a signed int) will produce incorrect results.



So, yeah, it will work if you know you are never printing values that large, but %u would still be the proper one to use when printing unsigned integers... And you won't get that warning :)

Trick C question time … what will this print?

#include <stdio.h>
#include <stdlib.h>

int main()
{
   int x;
   unsigned int y;

   x = -1;
   y = 2;

   printf("x = %dn", x);
   printf("y = %un", y);

   if ( x > y )
   {
      printf("x > yn");
   }
   else if (x < y)
   {
      printf("x < yn");
   }
   else
   {
      printf("x == yn");
   }

   return EXIT_SUCCESS;
}

I recently began looking in to various compiler warnings in some code I am using, and I found quite a few warnings about comparing signed and unsigned values:

warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

I thought I could safely ignore these, since it seems plausible to compare a signed value with an unsigned value. A signed value of -42 should be less than an unsigned value of 42, right?

In the above example, it will print the following:

x = -1
y = 2
x > y

Nope. I was wrong. According to C, -1 is greater than 2.

C does something that I either never knew, or knew and have long since forgotten. I guess I generally try to write code that has no warnings at all, so I’ve avoided doing this. And now I know (or re-know) the reason why.

When dealing with mis-matched comparisons, C makes them both unsigned. Thus, “-1” becomes whatever -1 would be for that data type.

char  achar  = -1;
short ashort = -1;
int   aint   = -1;
long  along  = -1;

printf("char  -1 as unsigned: %un", (unsigned char)achar);
printf("short -1 as unsigned: %un", (unsigned short)ashort);
printf("int   -1 as unsigned: %un", (unsigned int)aint);
printf("long  -1 as unsigned: %un", (unsigned long)along);

This outputs:

char  -1 as unsigned: 255
short -1 as unsigned: 65535
int   -1 as unsigned: 4294967295
long  -1 as unsigned: 4294967295

Thus, on a PC, an 8-bit signed value of -1 is treated as a 255 when comparing against an unsigned value, and a 16-bit as 65535. It seems an int and long as both 32-bits on my system, but these could all be different on other architectures (on Arduino, and int is 16-bits, I believe).

So, without this warning enabled, any comparison that looks correct might be doing something quite wrong.

Warnings are our friends. Even if we hate them and want them to go away.

Consider this code:

int i;

i = 42;

someFunction( i );

printf( "The value of i is %d\n", i );

What value will it print?

Comment away…

I started learning C programming back in the late 1980s. I was using the original Microware OS-9 C compiler on a Radio Shack Color Computer 3. It was a K&R compiler, meaning it was the original version of C that was before the ANSI-C standard. Back then, I recall reading a magazine article that claimed C would be “the language of the 80s” so I decided to see what the fuss was about.

A Commodore-using friend of mine, Mark, was helping me learn C. He loaned me a Pocket C reference guide. That, and the Microware documentation, was all I had for reference material. At the time, Mark had moved from his Commodore 64 to a powerful Amiga computer. He would dial in to my OS-9 BBS system and upload source code he wrote on his Amiga and then compile it on my machine to see if it ran there, too. It was amazing that this was even possible, considering how non-portable BASIC programs were from machine to machine.

It was a fun time.

Over the years, I learned much about C from books and friends and just general experimentation. One thing I learned was this weird conditional assignment operation:

a = (b == 10 ? 100 : 200);

It is basically doing this:

condition ? value_if_true : value_if_false

The value of a would be set to 100 if b was 10, otherwise it would be set to 200. It was a shortcut to writing the code like this:

if (b == 10)
{
  a = 100;
}
else
{
  a = 200
}

…or…

switch( b )
{
  case 10:
    a = 100;
    break;
  default:
    a = 200;
    break;
}

I have used this many times over the years, but don’t even know what it’s called. I asked a coworker, and they told me it was a “ternary operator“. Here is the Wikipedia entry on how it works in C:

https://en.wikipedia.org/wiki/Ternary_conditional_operator#C

It is a great shortcut for response strings. For instance, turning a boolean true/false result in to a string:

printf( "Status: %s\n", (status == true ? "Enabled" : "Disabled" );

This will print “Status: Enabled” if status==true, or “Status: Disabled” if status==false. What a neat shortcut.

Recently, I saw a nested use of this ternary operator. It never dawned on me that you could do this. It was something like this:

char *colorStr = (value == RED) ? "Red" : (value == BLUE) ? "Blue" : "Unknown";

It was using a second ternary operator for the “value_if_false” condition, allowing it to have three conditions rather than just two. I realized you could nest these in many different ways to create rather complex things… Though, readability would likely suffer. I think I’d just stick with simple if/then or switch/case things for anything more than two choices, but in this case it seemed simple enough.

I thought it was neat, and decided I’d share it here in this quick article.

In my day job, I am an embedded C programmer. Our devices work with flow measurement and use a bunch of floating point math for accumulators and such. In the past three years, I have learned some interesting things about floating point numbers and how they cannot represent certain values. I was therefore amused to see an easy example of the floating point problem when I was paying for postage last night:

I wonder how much it deducted from my PayPal account . . .

Updates:

• 2025-02-13 – fixing code formatting that WordPress messed up at some point.

In an earlier series, I discussed some easy ways to prevent buffer overrun problems when doing copies of C strings. As part of this, I created a few of my own implementations for things like strncpy() and strnlen(). Much of what I did as a workaround could be simplified if we had a smarter string concatenate function, so today I’d like to present one.

Fixing strncat

The max-limited string copy (strncpy) works well enough to prevent buffer overruns when copying strings, but the max-limited string concatenate (strncat) does not. It only limits how many characters it copies from the source buffer, without any regard to how much room is left in the destination buffer. (Is this of any use?)

We can do better.

The first thing strcat has to do is look at the destination buffer and seek to the end of whatever null terminated C string is there. Since it is already doing this work, it would be easy for it to limit how many characters it copies based on being told the maximum size of the destination buffer (as opposed to strncat, which limits based on a maximum size of the source buffer).

I am envisioning a function that looks like strncat(), but the max number passed in is for the destination buffer. Thus, if I try to append a string of 10 characters to a buffer that can hold up to 40 characters, I’d just append with a value of 40, and the function would check how much is already in the buffer and do the math for me. Because math is hard.

Here is what it might look like:

char * strncatdst( char * destination, const char * source, size_t num )
{
    size_t len;
    size_t left;

    // Step 1 - find out how much data is in the destination buffer.
    len = strlen( destination );

    // If string len is longer than we want...
    if (len > num)
    {
        // ...limit the len to be the max num.
        len = num;
    }

    // len comes back with how much is in the buffer or maxed to num.

    // Step 2 - find out how much room is left
    left = num - len;

    // Step 3 - copy up to a null, or until we hit the max size.
    // We always copy one less because strncat() adds an extra null.
    strncat( destination, source, left-1 );

    // Return destination pointer, because C says so.
    return destination;
}

This demonstrates a simple way to make a custom version of strncat() by using other standard C library functions. It could be used like this:

#define BUFSIZE 20

int main()
{
    char buffer[BUFSIZE];
    char *string = "This is a long string";

    // Put an initial string in the buffer.
    strcpy(buffer, "new:");

    // Using our own strncatdst() function:
    strncatdst(buffer, string, BUFSIZE );

    printf("buffer = '%s'\n", buffer);

    return EXIT_SUCCESS;
}

Of course, we probably want to use a safer strcpy() as well, so going back to an earlier article I wrote, I should have done this:

// Put an initial string in the buffer.
strncpy(buffer, "new:", BUFSIZE-1);
buffer[BUFSIZE-1] = '';

// Using our own strncatdst() function:
strncatdst(buffer, string, BUFSIZE );

printf("buffer = '%s'n", buffer);

Even though I know I am only copying four characters (“new:”) there, I might not be sure of the length if I was copying in some string that was created somewhere else, or if I (or someone) changed that string to something longer without thinking about the buffer size.

Fixing strncpy

strncpy() could also use a bit more work because it does not put in a null terminator (required for C strings) if the string being copied is as long (or longer) as the max length specified.

Let’s see if we can make an improved strncpy() that handles the null terminator:

char * strncpynull( char * destination, const char * source, size_t num )
{
    // Step 1 - copy up to 1 less than num characters.
    strncpy( destination, source, num-1 );

    // Step 2 - make sure there is null terminator, in case num reached.
    destination[num-1] = '\0';

    return destination;
}

As you can see, this is just a wrapper for the standard strncpy() library function that adds a final null just in case the string is that long.

Now we can use it like this:

strncpynull(buffer, "This is a really long string.", BUFSIZE);

printf("buffer = '%s'n", buffer);

…and it will make sure it trims long strings to to len-1, and adds a null terminator (which standard strncpy does not do).

Is it fixed yet?

These two functions should protect us against string buffer overruns, provided we know the size of the destination buffer.

However, by calling existing library functions, we are adding extra overhead. If those functions are highly optimized and very well done, this may still be more efficient than doing them yourself (and it is certainly easier to leverage existing functions rather than rolling your own). However, there are still a few potential issues that bother me.

For instance, for my strncatdst() function, the first thing I do is use strlen() to get the length of the string. It does this by starting at the first byte of the destination buffer and walking through it until it finds a null character. If this was a corrupt pointer, it might find itself walking through bogus memory until it happens to find a zero, potentially crashing the program from a memory access exception (if the operating system has such).

It is also not efficient because, after strlen(), the standard strncat() is used, and internally, it also must start with the first character in the destination buffer and walk through all the bytes until it finds a null (or the max num is reached), to know where it can start appending the source string. If we were doing this to a buffer containing a large 1K string, it would be walking through that 1K twice!

We can do better than that. Let’s see if we can create versions of these functions that do not use the existing C library functions.

Fixing strlen

First, recall my proposal for a version of strlen() that has a limit since there is no such strnlen() function as part of the ANSI-C standard library:

size_t strnlen( const char * str, size_t num )
{
    size_t len;

    len = strlen(str);

    // If string len is longer than we want...
    if (len > num)
    {
        // ...limit the len to be the max num.
        len = num;
    }

    // Return actual len, or max len.
    return len;
}

From the caller’s perspective, that seems fine, but this won’t actually solve the problem — it only hides it, and is still calling strlen() internally.

For this one, we really do need to create our own version so we can prevent it from scanning through 1K of memory if we know the string we expect to find should never be that long.

size_t strnlen2( const char * str, size_t num )
{
    size_t len;

    len = 0;

    while(len < num)
    {
        if (str[len]=='\0') break;
        len++;
    }

    return len;
}

This function will now stop counting at a value you specify. The original version I created used strlen() so it would count endlessly until it found a 0 before returning and having that value (if too big) clipped to the num passed in. This seems to be better for those chances when we are passed a bad pointer. (Not that that EVER happens, right?) It’s just not as efficient as it could be, so we’ll address that later.

Next, let’s use that code inside a new strncatdst() function:

char * strncatdst2( char * destination, const char * source, size_t num )
{
    size_t len;
    size_t index;

    // Step 1 - find out how much data is in the destination buffer.
    // This is basically the strnlen2() code, above.
    len = 0;

    while(len < num)
    {
        if (destination[len]=='\0') break;
        len++;
    }

    // len comes back with how much is in the buffer or maxed to num.

    // Step 2 - copy characters until we are out of room.
    index = 0;

    while(len < num)
    {
        destination[len] = source[index];
        if (source[index]=='\0') break;
        len++;
        index++;
    }

    // Step 3 - make sure string is null terminated. We really only
    // need to do this is len==num, but the overhead of adding the
    // check is probably more than just always doing it.
    if (len == num)
    {
        destination[num-1] = '\0';
    }

    // Return destination pointer, because C says so.
    return destination;
}

Now we can append short or long strings to a destination buffer, and ensure we never copy more than the size of that buffer, including a null terminator we will add if needed.

Next is a version of strncpynull() that does not use library functions. I previously shared a simple strncpy() implementation to demonstrate what it did (padding short strings with nulls). Using that as a reference, we have:

char * strncpynull2( char * destination, const char * source, size_t num )
{
    size_t index;

    // Step 1 - copy up to 1 less than num characters.
    index = 0;

    while(index < num-1) // One less, to leave room for null.
    {
        if (source[index]=='\0') break; // Exit the for loop.
        destination[index] = source[index];
        index++;
    }

    // Here we have copied 'index' characters.

    // If less than num, fill the rest with nulls.
    while(index < num)
    {
        destination[index] = '\0';
        index++;
    }

    return destination;
}

Now we have our own free-standing enhanced versions of strncpy(), strncat() and strnlen().

We should probably look at optimizing them so they are less stupid!

To be continued…

See also: part 1, part 2, part 3, part 4, part 5 and part 6.

Updates:

• 3/3/2016 – Updated note about the problem with strlen(), referencing the discussion in part 5, and the solution in the summary.

And now, a simple one-page summary of how to make copying/appending C strings safer and hopefully avoid potential buffer overrun crashes (or other problems).

Copying Strings

Instead of using strcpy(), which does absolutely no checking to see if it’s copying more data than the destination buffer can hold, use strncpy() to limit how much can be copied. If the max amount is copied, strncpy() will not null terminate the destination, so you need to do that yourself.

#define BUFSIZE 30 // size of desination buffer

char buffer[BUFSIZE] = { 0 }; // Initialize buffer with zeros.
char *longString = "Copy this long string to the buffer.";
char *shortString = "Short string.";

// Instead of strcpy(buffer, shortString), do this:
// Copy up to max buffer size-1 (leaving room for a null).
// In case of max-sized string, make sure to null terminate. 
strncpy( buffer, shortString, BUFSIZE-1 );
buffer[BUFSIZE-1] = '\0';

printf( "Buffer: '%s'\n", buffer );

// Instead of strcpy(buffer, longString), do this:

// Copy up to max buffer size-1 (leaving room for a null).
// In case of max-sized string, make sure to null terminate. 
strncpy( buffer, longString, BUFSIZE-1 );
buffer[BUFSIZE-1] = '\0';

printf( "Buffer: '%s'\n", buffer );

That should produce the following output:

Buffer: 'Short string.'
Buffer: 'Copy this long string to the '

Appending Strings

Instead of using strcat() to append a string to an existing string buffer, use strncat() and some math to not copy more than the buffer can hold (counting how many characters are already in it). NOTE: As mentioned in part 5, the use of strlen() is still a point of failure so this is actually NOT a reliable fix.

#define BUFSIZE 30 // size of destination buffer

char buffer[BUFSIZE] = { 0 }; // Initialize buffer with zeros.
char *string1 = "Buffer start.";
char *string2 = "This is what we will be appending.";

printf( "Initial buffer  : '%s'\n", buffer );

// Let's put something in the buffer to demonstrate.
// In case of max-sized string, make sure to null terminate. 
strncpy( buffer, string1, BUFSIZE-1 );
buffer[BUFSIZE-1] = '\0';

printf( "Copied buffer   : '%s'\n", buffer );

// Instead of strcat(buffer, string2), do this:

// Let's "safely" append something to the buffer.
strncat( buffer, string2, BUFSIZE-strlen(buffer)-1 );
// NOTE: strlen() can cause a failure if the original buffer
// was already bad with too much data. We need an strnlen()
// but no such function exists in ANSI-C. See the followup
// article for a "roll your own" set of functions to solve this.
//strncat( buffer, string2, BUFSIZE-strnlen(buffer, BUFSIZE)-1);

printf( "Appended buffer: '%s'\n", buffer );

This should produce the following output:

Initial buffer : ''
Copied buffer : 'Buffer start.'
Appended buffer: 'Buffer start.This is what we '

Comparing Strings

And, instead of using strcmp() to compare strings, use strncmp() so you can set the maximum string size expected.

#define FIRSTNAME_SIZE 20

char firstName[FIRSTNAME_SIZE] = {0}; // Initialize with zeros.

...

// Instead of strcmp(buffer, firstname), do this:

if (strncmp( buffer, firstName, FIRSTNAME_SIZE ) == 0)
{
  ...

Follow these three simple steps and you will greatly reduce your chance of a buffer overrun that could cause unexpected problems.

Look for an upcoming article that will expand on this with replacement functions you can use to make things easier and more efficient.

See also: part 1, part 2, part 3, part 4, part 5 and part 6.

In our quest to find the safest (or at least a safer) way to copy or append strings, we have been discussing the C library functions strncpy() and strncat(). In the previous installment I mentioned there is a potential problem. That problem is with strlen(), which we are using for strncat() to determine how much data is already in the destination buffer:

// Create full name by starting with first name...
strncpy( fullName, firstName, FULLNAME_SIZE-1 );
fullName[FULLNAME_SIZE-1] = '';

// ...then appending a space...
strncat( fullName, " ", FULLNAME_SIZE-strlen(fullname)-1 );

// ...then appending the last name.
strncat( fullName, lastName, FULLNAME_SIZE-strlen(fullName)-1 );

We are assuming that the length of the data in the buffer is never greater than what the buffer can hold. But what if, somehow, the existing string was too large? If strlen(fullName) was larger than FULLNAME_SIZE, the math would break. Let’s say FULLNAME_SIZE is 20, but the string copied there was 25 (a buffer overrun):

FULLNAME_SIZE-strlen(fullName)-1

…is 20-25-1 which is -6 which means:

strncat( fullName, " ", -6 ); // error!

I would think that strncat() should fail because it is being passed a negative number for the count, but that third parameter is of type size_t, which, according to the cplusplus.com page, is:

Unsigned integral type

Alias of one of the fundamental unsigned integer types.It is a type able to represent the size of any object in bytes: size_t is the type returned by the sizeof operator and is widely used in the standard library to represent sizes and counts.

Unsigned numbers can only be positive (0 and up), while signed numbers can be negative or positive. If you pass in a -1 as an unsigned value, it will actually look like a huge positive number. (For signed numbers, one of the bits is used to indicate if it is positive or negative. If you use an 8-bit value, that can represent the unsigned values of 0-255, or the signed values of -128 to 127).

Thus, because of bad math, strncat() will operate as it was instructed, thinking the limit is some huge number.

Gotcha.

What we really need is a version or strlen() that stops counting when it his a maximuim size (like a strnlen() call). If we did, we could also give it the limit of our buffer:

FULLNAME_SIZE-strnlen(fullName, FULLNAME_SIZE)-1

That way, strnlen() would never return anything greater than FULLNAME_SIZE, so if it didn’t find a null (0) terminator before that character, it would just return the max. Thus, the above example would be 20-20-1 which is… -1.

Crap.

We would have to treat strnlen() the same way we do the other calls, and subtract one from it (a 40 character max buffer would return 39, always leaving room for an extra null terminator):

FULLNAME_SIZE-strnlen(fullName, FULLNAME_SIZE-1)-1

That would give us 20-19-1 which is 0, and passing in a 0 to strncat() should fail, not appending anything.

Problem solved.

Unfortunately, the ANSI-C standard has no such function (though some do offer it as a non-standard function). This means to be safe in these situations, you would have to create our own strnlen() function. A simple wrapper to the existing strnlen() should work:

// Return the length of the string, or num,
// whichever is smaller.
size_t strnlen( const char * str, size_t num )
{
    size_t len;

    len = strlen(str);

    // If string len is longer than we want...
    if (len > num)
    {
        // ...limit the len to be the max num.
    len = num;
    }

    // Return actual len, or max len.
    return len;
}

Problem actually solved.

This may seem to be a very unlikely error, but maybe you have a function written to deal with a buffer up up to 40 characters, but the caller was creating one that could hold up to 60. They might have a string there longer than you expect, so when you go to append (thinking 40 is the max) there might already be a longer string there.

This type of problem can be quite common when using code from other libraries or projects.

Or, perhaps someone properly used the buffer size, but used strncpy() and it filled it up without a null terminator to stop strlen(). Ah! This seems like a much more common issue.

Pity my proposed strnlen() is still potentially inefficient (and could even cause a crash). Do you see why? I guess we will have to fix that, too.

In the next part, I will summarize all of this on one simple “better practices” page for dealing with string copies or appends.

See you then…