Hi Martin, On 12/15/22 21:50, Martin Sebor wrote:
On 12/14/22 16:14, Alejandro Colomar via Libc-alpha wrote:
[...]
int main(void) { char buf[BUFSIZ]; size_t len; buf[0] = '\0'; // There’s no ’cpy’ function to this ’cat’. strncat(buf, "Hello ", 6);There's nothing wrong with this but the two lines above would be more simply coded as one: strcpy(buf, "Hello ");
I used a string literal to avoid having to think hard of an example using utmpx(5), which would be the actual code for which I want users to use this function.
With time, I expect to develop such example program.
The original code suggests a misunderstanding of strncpy's purpose:
By the "original code" what are you exactly referring to? The code quoted above, or another?
In any case, the code is using strncat(3) not strncpy(3).
that it writes exactly 6 bytes into the destination.
That's the purpose of strncpy(3). I'll quote the versions of the pages that I'm proposing:
strncpy(3):
These functions copy the string pointed to by src into a null‐padded
character sequence at the fixed‐width buffer pointed to by dst. If the
destination buffer, limited by its size, isn’t large enough to hold the
copy, the resulting character sequence is truncated. They only differ
in the return value.
The description above says that the destination buffer is limited by its size,
so as you say, this function copies exactly 6 bytes (the character sequence +
null padding).
strncat(3)
This function catenates the input character sequence contained in a
null‐padded fixed‐width buffer, into a string at the buffer pointed to
by dst. The programmer is responsible for allocating a buffer large
enough, that is, strlen(dst) + strnlen(src, sz) + 1.
But strncat(3) writes whatever is the length of the character sequence passed as
input (ignoring the padding null bytes), plus a terminating null byte.
That's what the warning points out.
The warning would be fair for strncpy(3), but not for strncat(3).
strncat(buf, "world", 42); // Padding null bytes ignored. strncat(buf, "!", 1); len = strlen(buf); printf("[len = %zu]: <%s>\n", len, buf); exit(EXIT_SUCCESS); } SEE ALSO string(3), string_copy(3) Linux man‐pages (unreleased) (date) strncat(3) And when you compile that, you get: $ cc -Wall -Wextra ./strncat.c ./strncat.c: In function ‘main’: ./strncat.c:12:12: warning: ‘strncat’ specified bound 6 equals source length [-Wstringop-overflow=] 12 | strncat(buf, "Hello ", 6); | ^~~~~~~~~~~~~~~~~~~~~~~~~ ./strncat.c:14:12: warning: ‘strncat’ specified bound 1 equals source length [-Wstringop-overflow=] 14 | strncat(buf, "!", 1); | ^~~~~~~~~~~~~~~~~~~~ So, what? Where's the problem? This function does exactly that: "take an unterminated character sequence and catenate it to an existing string".Strncat has historically had two distinct use cases. One of them -- to constrain the amount of data to copy to the space remaining in the destination -- gained popularity with the push to reduce buffer overflow weaknesses in code. Mistakes in these uses gave rise to a whole other class of security bugs, to the extent that CERT felt it necessary to publish the strncpy and strncat best practice. The GCC warning in turn was added to support the CERT guideline. I touch on some of this in a blog post I wrote a few years ago: https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8
While this is true, that's because there was nothing more appropriate, such as strlcat(3).
My point is that strncat(3) is far from being a good tool for that, and should never be used for that purpose in good code; especially now that we have strlcat(3) widely available in most if not all POSIX systems (in Linux through libbsd).
I'll quote some part of the link above: | strncat (dest, src, dest_size - strlen (dest) - 1); | | Calls that have this form are not diagnosed.That code is a source of bugs. Such code should _always_ be replaced by strlcat(3) as:
strlcat(dst, src, sizeof(dst);
| Other calls, such as those where
| the size is derived in some way from the size or length of the source string,
| are diagnosed by -Wstringop-overflow. That includes unsafe calls like
The only sane use of strncat(3) is that. It's a use where no other function
serves better. To read a possibly non-terminated character sequence from a
fixed-width buffer; and the size is the only way strncat(3) can know when to
stop reading.
| | strncat (dest, src, strlen (src)); // incorrect - warningThis call doesn't make any sense; I agree. It should _always_ be replaced by strcat(3) as:
strcat(dst, src);
I acknowledge that this is the case I show in the manual page, but only because
there is no form to create an unterminated string literal (I take this
opportunity to re-suggest "foo"u as an unterminated string literal).
I'll try to come up with an example program that avoids this.
|
| and
|
| strncat (dest, src, sizeof src); // incorrect - warning
This is the _perfect_ use case for strncat(3). No other function is better.
Let's say you have the following:
char src[5] = {'h', 'e', 'l', 'l', 'o'};
How do you copy that into a string? strncat(3). utmpx(5) is one of the
examples where this happens.
I'll modify the example program to use this, to avoid being misleading. I'll replace string literals by character sequences.
The specific uses of the function above are contrived (there's no point in calling strncat to append the full string -- strcat will do that more clearly and efficiently) but the general use case -- limiting the amount of copied data to an initial substring of the source sequence -- although valid and originally intended (it's one of the two uses of the function in UNIX v7), is not one that either the guideline or the warning consider. They can only consider one use cases, and they chose the onethat was observed behind security bugs.
It's sad that strncat(3) has been used so much for limiting the dest copy. It's pretty unsafe for that, we agree. How about allowing the safe use of it (i.e., size derived from source buffer), and warn about uses where the size is derived from the dst string? I'm going to rewrite the manual page in that direction.
That choice unavoidably leads to some false positives. The expected way to deal with them is to suppress the warning by one of the usual mechanisms (command line option or #pragma GCC diagnostic).
The only use case that GCC is allowing is better served by strlcpy(3) and strlcat(3). I suggest changing the warning to protect the only natural use case for strncat(3), and warn about uses of it supplanting strlcat(3) (and maybe suggest using strlcat(3)).
Martin
Thanks! Alex -- <http://www.alejandro-colomar.es/>
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
