“It ain’t what you don’t know that gets you into trouble. It’s what you know for sure that just ain’t so.”
–Mark Twain
Buffer overflows are among the most frequent causes of security flaws in software. They typically arise in situations such as when a programmer is 100% certain that the buffer to hold a user’s name is big enough — until a guy from India logs in. Thus, well-behaved developers always use the bounded-length versions of string functions. Alas, they come with differing, dangerously confusing interfaces.

THE GOOD

Let’s start with ‘fgets‘:

No matter what users type into their terminals, ‘fgets’ will ensure that ‘user_name’ is a well-formed, zero-terminated string of at most 29 characters (one character is needed for the ‘\0’ terminator). The same goes for the ‘snprintf‘ function. After executing the following code

‘buffer’ will contain the string “The”, again, properly zero-terminated.

Both functions follow the same, easy-to-grasp pattern: you pass a pointer to a target buffer as well as the buffer’s total size and get back a terminated string that doesn’t overflow the buffer. Awesome!

THE BAD

In order to copy strings safely, developers often reach for ‘strncpy‘ to guard themselves against dreaded buffer overruns:

Unfortunately, this is not how ‘strncpy’ works! We assumed that it followed the pattern established by ‘fgets’ and ‘snprintf’ but that’s not the case. Even if ‘strncpy’ promises that it never overflows the target buffer, it doesn’t necessarily zero-terminate it. What it does is copy up to ‘sizeof(buffer)’ bytes from ‘user_name’ to ‘buffer’ but if the last byte that is copied is not ‘\0’ (i. e. ‘user_name’ comprises more than ‘sizeof(buffer)’ characters), ‘strncpy’ leaves you with an untermiated string! A traditional approach to solve this shortcoming is to enforce zero-termination by putting a ‘\0’ character as the last element of the target buffer after the call to ‘strncpy’:

Using ‘strncpy’ without such explicit string termination is almost always an error — a rather insidious one, as your code will work most of the time but not when the buffer is completely filled (i. e. your Indian colleague “Villupuram Chinnaih Pillai Ganesan” logs on).

Boy, oh boy is this inconsistent! ‘fgets’ and ‘snprintf’ give you guaranteed zero-termination but ‘strncpy’ doesn’t. A clear violation of the principle of least surprise. Apparently, ‘strncpy’ fixes one safety problem and at the same time lays the foundation for another one.

THE UGLY

Can it get worse? You bet! How do you think ‘strncat‘, the bounded-length string concatenation function, behaves? Ponder this code:

But this is wrong, of course: the third argument to ‘strncat’ (let’s call this argument ‘n’) is not the size of the target buffer. It is the maximum number of characters to copy from the source string (‘string2’) to the destination buffer (‘buffer’). If the length of the source string is greater or equal to ‘n’, ‘strncat’ copies ‘n’ characters plus a ‘\0’ to terminate the target string. Confused? Don’t worry, here’s how you would use it to avoid concatenation buffer overruns:

Yuck! What’s the likelihood that people remember this correctly?

THE REMEDY

Even if the different interfaces and behaviors of the bounded-length string functions in the C API make sense for certain use cases (or made sense at some point in time), the upshot is that they confuse programmers and potentially lead to new security holes when in fact they were supposed to plug them. What’s a poor C coder supposed to do?

As always, you can roll your own versions of bounded/safe string functions or use my safe version of ‘strcpy’. If you rather prefer something from the standard library, I’d suggest that you use ‘snprintf’ as a replacement for both, ‘strncpy’ and ‘strncat’:

Looks like ‘snprintf’ is the swiss army knife of safe string processing, doesn’t it? The moral is this: use whatever you’re comfortable with, but refrain from using ‘strncpy’ or ‘strncat’ directly.

More dangerously confusing interfaces…

Just like the other “Dangerously Confusing Interfaces” posts, this one was also inspired by a real-world blunder that I made.

Here’s the background: usually, routines that accept data via a pointer from the caller either execute synchronously or copy the data into their own internal data structures for later processing. Take the venerable ‘fwrite’ from the C standard library as an example:

‘fwrite’ blocks until the data has been written, either to disk or to an internal buffer. In either case, once ‘fwrite’ returns, it doesn’t care about the original data anymore. That’s why it’s safe (and common practice) to pass a pointer to a local buffer on the stack:

All standard library and POSIX APIs behave like ‘fwrite’, which is both, safe and convenient. However, with embedded systems, the story is different: in some cases, memory is so tight that additional buffers/internal storage can’t be afforded. Such functions don’t copy the provided data but only store a pointer to your data and expect the memory pointed-to by this pointer to be still valid long after the function call has returned. Here is an example from the AUTOSAR standard, which is used by almost all embedded automotive products:

‘NvM_WriteBlock’ is used to store data to a given non-volatile memory block. However, what this function does is only enqueue a request for the given block ID together with the data pointer (not a copy of your data). This is done for the sake of efficiency, because there can be multiple write requests in parallel. The queue is later processed in another task, long after any local buffer would have been removed from the stack.

Passing a pointer to a buffer with automatic storage is an easy mistake to make, especially since such “non-copy” interfaces are so rarely encountered. How can “write-like” interfaces that don’t make a copy of the provided data be made safer, such that misuse is less likely? Obviously, just adding documentation is not enough — nobody reads documentation, especially in the heat of the moment.

In my view, the root of the problem is that such functions accept just about any pointer. What if the caller was forced to explicitly cast the pointer to another type? A type with a cunningly chosen typename, one that reminded the caller of the potential pitfall? Here is my approach:

Whenever a pointer is passed to this function, developers have to write something like this to make the compiler happy:

Typing ‘uncopied_memory’ should shake up even the most focused developers and remind them to double-check what they are passing into this function.

Of course, within ‘SomeWritelikeFunction’, the provided pointer needs to be cast back into something more useful, like a ‘const uint8_t*’. Further, note that the ‘dummy’ member within ‘uncopied_memory’ must not be used; it only exists to make sure that the cast to ‘uncopied_memory*’ in the calling function is safe: a pointer to a struct is aligned such that it is aligned with the struct’s first member, which is ‘char’ and ‘char’ has the weakest alignment requirements.

Designing intuitive interfaces that are easy to use and easy to learn is hard, often very hard; and for economic reasons it might not always be possible to strive for perfection. Nevertheless, in my view, at the very least, interfaces should be designed such that obvious, day-to-day usage doesn’t lead to damage.

In his classic book “Writing Solid Code”, Steve Maguire calls confusing interfaces that lead to unexpected bugs “Candy Machine Interfaces”. He tells a story from a vending machine at Microsoft that used to cause him grief: The machine displayed “45 cent” for “number 21”, but after he had finally inserted the last coin he would sometimes enter “45” instead of “21” (and would get a jalapeño flavored bubble-gum instead of the peanut butter cookie that he wanted so much — Ha Ha Ha!). He suggests an easy fix: replace the numeric keypad with a letter keypad and no confusion between money and items would be possible anymore.

The other day I did something like this:

My goal was to recursively copy the ‘gamma’ folder to my home folder. What I expected was a ‘gamma’ folder within my home directory, but instead I ended up with hundreds of files from the ‘gamma’ directory right at the top-level of my home directory — the ‘gamma’ directory simply wasn’t created!

I have to confess that similar things sometimes happen to me with other recursive-copy-like tools, too — this seems to be my candy machine problem. Now you know it.

As for ‘rsync’, there is a feature that allows you to copy just the contents of a directory, without creating the directory, flat into a target directory. Granted, this is sometimes useful, but do you know how to activate this mode? By appending a trailing slash to the source directory! That’s what happened in my case. But I didn’t even add the slash myself: if you use Bash’s TAB completion (like I did) a trailing slash is automatically appended for directories…

But good old ‘cp’ puzzles me even more. If you use it like this

it will copy ‘from3’ to a folder named ‘to2’ under ‘to1’ such that both directories (‘from3’ and ‘to2’) will have the same contents, which is more or less a copy-and-rename-at-the-same-time operation. Unless ‘to2’ already exists, in which case ‘from3’ will be copied in ‘to2’ resulting in ‘to1/to2/from3’. Unless, as an exception within an exception, there is already a ‘from3’ directory under ‘to2’; in this case ‘cp’ will copy ‘from3’ flat into the existing ‘to2/from3’ which might overwrite existing files in that folder.

Both, ‘cp’ and ‘rsync’ suffer from fancy interfaces that try to add smart features — which is normally good — but they do it in an implicit, hard-to-guess, hard-to-remember way — which is always bad. Flat copies are sometimes useful but they might be dangerous as they could inadvertently overwrite existing files or at least deluge a target directory. A potential cure could be an explicit ‘–flat’ command-line option.

To me, a wonderfully simple approach is the one taken by Subversion: checkouts are always flat and I’ve never had any problems with it:

This copies (actually checks-out) the contents of the ‘trunk’ flat into the specified destination directory — always, without any exceptions. That’s the only thing you have to learn and remember. There are no trailing backslashes or any other implicit rules. It will also create the target parent directories up to any level, if needed.

Naturally, dangerously confusing interfaces exist in programming interfaces, too. Sometimes the behavior of a method depends on some global state, sometimes it is easy to confuse parameters. The ‘memset’ function from the C standard library is a classic example:

Does this put 40 times the value of 32 in ‘buffer’ or is it the other way around?

I have no idea how many programmers don’t know the answer to this question or how many bugs can be attributed to this bad interface, but I suspect that in both cases the answer must be “way too many”. I don’t want to guess or look up the specification in a manual — I want the compiler to tell me if I’m wrong. Here is an alternative implementation:

Now you write

If you confuse the fill character with the length parameter the compiler will bark at you — a parameter mix-up is impossible. Even though this is more to type than the original (dangerous) interface: it is usually worth the while if there are two parameters of the same (or convertible) type next to each other.

Like I said in the beginning: designing intuitive interfaces is hard but spending extra effort to avoid errors for the most typical cases is usually a worthwhile investment: don’t make people think, make it difficult for them to do wrong things — even if it sometimes means a little bit more typing.