Replace uses of strncpy() with strlcpy()

[krader1961]

A recent change broke building ksh on macOS because on macOS strlcpy() and strlcat() are macros. And those macros break the declarations in src/lib/libast/include/ast_sys.h. That problem has been worked around, imperfectly, by moving the #include <string.h>preprocessor directive to the end of the file. Long term we should replaces uses of strlcpy() with strncpy() (there are no uses of strlcat() AFAICT) then remove those functions from the libast code base.

See https://lwn.net/Articles/507319/ for some excellent background on why these functions are not part of GNU libc.

[jens-maus]

IMHO using strncpy() should not be used anymore as buffer overflow issues are more severe than all the other things talked over in your referenced lwn.net article. In fact, strlcpy() should definitely be preferred (in addition to guarding it with if()) to improve general security in ksh.

[krader1961]

Sorry, @jens-maus, but strlcpy() just has a different failure mode. Albeit one whose consequences are less likely to result in a security hole but no less wrong. You can't just use it without dealing with the result being truncated. Any more than you can use strncpy() without ensuring the destination buffer was long enough for the terminating null. It's just a different way to shoot yourself in the foot and isn't as widely available. Which means maintaining our own implementation. No thank you.

Also, implementations of strncpy() are likely to be highly optimized for the machine architecture. That will never be true for the AST strlcpy() implementation.

[kernigh]

strncpy() is slow! After it copies the string, it fills the rest of the buffer with NUL characters. For example, strncpy(dst, "hi", 80) writes 'h', 'i', and 78 NULs. This is a reason to avoid strncpy(). This slowness is unique to strncpy(), not in strncat(), strncmp(), strndup(), nor strnlen().

Consider the code in src/cmd/ksh93/sh/io.c, lines 1870 and 1871:

 strlcpy(buff, xp, size);
 rsize = strlen(buff);

Suppose that we want to use strncpy(). We may calculate the size so we don't write too many NULs,

rsize = strlen(xp);
if (rsize > size - 1) rsize = size - 1;
strncpy(buff, xp, rsize);
buff[rsize] = '\0';

but then we can change strncpy(buff, xp, rsize); to memcpy(buff, xp, rsize). We are copying rsize bytes, these bytes are not NUL, so we don't need strncpy() to search for NUL.

[krader1961]

That is a good point @kernigh. But if performance is the issue then the example you linked to doesn't need to use strlcpy() to be as performant. My point is that people recommending using strlcpy() often do so without regard to a) that it can be more performant, and b) almost always completely ignore its failure modes.

Too, at the end of the day, even if we all agree that strlcpy() is superior the current AST code needs to be modified to use the implementation provided by the host rather than use its own, private, implementation.

[krader1961]

I've been thinking about this some more. I reread the article I linked to in my initial comment as well as every document it linked to. As a grey beard who prides myself on my knowledge of CS trivia I was surprised to realize I did not remember that strncpy() null pads the destination buffer. Which is incredibly inefficient when the destination buffer is large and the source string is small.

The reason for that behavior is that strncpy() was not intended to be a safer version of strcpy(). It was created to make it easier to create fixed size "records". That is, buffers of a fixed size that would be stored or sent over the wire (i.e., IPC) where you wanted to ensure that any unused bytes did not contain data from a previous use of the buffer (itself a potential security problem). And for that use case it is unusual for there to be a big discrepancy between the length of the source string and the size of the destination buffer. And even when there is a big size discrepancy the security concern of ensuring the destination does not contain stale data past the null byte is more important than efficiency.

So I now agree that the entire project should be uniformly using strlcpy() rather than strncpy(). Using either API we still have to do appropriate error checking. But that is somewhat easier with strlcpy() and it will often be faster.

It also turns out that even though GNU libc does not provide strlcpy() many Linux distros provide libbsd which does provide that function.

[krader1961]

See also http://static.usenix.org/event/usenix99/full_papers/millert/millert_html/index.html

[jens-maus]

@krader1961 Great that you finally came to the same conclusion like me at the beginning of the year ;-). Looking forward to see strncpy() dying in ast.