Both strncpy()
and (even more so) strncat()
have non-obvious behaviours and you would be best off not using either.
strncpy()
If your target string is, for sake of argument, 255 bytes long, strncpy()
will always write to all 255 bytes. If the source string is shorter than 255 bytes, it will zero pad the remainder. If the source string is longer than 255 bytes, it will stop copying after 255 bytes, leaving the target without a null terminator.
strncat()
The size argument for most of the 'sized' functions (strncpy()
, memcpy()
, memmove()
, etc) is the number of bytes in the target string (memory). With strncat()
, the size is the amount of space left after the end of the string that's already in the target. Therefore, you can only safely use strncat()
when you know both how big the target buffer is (S
) and how long the target string currently is (L
). The safe parameter to strncat()
is then S-L
(we'll worry about whether there's an off-by-one some other time). But given that you know L
, there is no point in making strncat()
skip the L
characters; you could have passed target+L
as the place to start, and simply copied the data. And you could use memmove()
or memcpy()
, or you could use strcpy()
, or even strncpy()
. If you don't know the length of the source string, you've got to be confident that it makes sense to truncate it.
Analysis of code in question
char filename[255];
strncpy(filename, getenv("HOME"), 235);
strncat(filename, "/.config/stationlist.xml", 255);
The first line is unexceptionable unless the size is deemed too small (or you run the program in a context where $HOME
is not set), but that's out of scope for this question. The call to strncpy()
does not use sizeof(filename)
for the size, but rather an arbitrarily small number. It isn't the end of the world, but there's no guarantee that the last 20 bytes of the variable are zero bytes (or even that any of them is a zero byte), in general. Under some circumstances (filename
is a global variable, previously unused) the zeros might be guaranteed.
The strncat()
call tries to append 24 characters to the end of the string in filename
that might already be 232-234 bytes long, or that might be arbitrarily longer than 235 bytes. Either way, that is a guaranteed buffer overflow. The usage of strncat()
also falls directly into the trap about its size. You've said that it is OK to add up to 255 characters beyond the end of what's already in filename
, which is blatantly wrong (unless the string from getenv("HOME")
happens to be empty).
Safer code:
char filename[255];
static const char config_file[] = "/.config/stationlist.xml";
const char *home = getenv("HOME");
size_t len = strlen(home);
if (len > sizeof(filename) - sizeof(config_file))
...error file name will be too long...
else
{
memmove(filename, home, len);
memmove(filename+len, config_file, sizeof(config_file));
}
There will be those who insist that 'memcpy()
is safe because the strings cannot overlap', and at one level they're correct, but overlap should be a non-issue and with memmove()
, it is a non-issue. So, I use memmove()
all the time...but I've not done the timing measurements to see how big of a problem it is, if it is a problem at all. Maybe the other people have done the measurements.
Summary
- Don't use
strncat()
.
- Use
strncpy()
cautiously (noting its behaviour on very big buffers!).
- Plan to use
memmove()
or memcpy()
instead; if you can do the copy safely, you know the sizes necessary to make this sensible.
cppcheck
static analyzer – ouah