From: Russell King <hidden> Date: 2003-11-27 22:19:34
On Thu, Nov 27, 2003 at 11:06:10PM +0100, Felipe Alfaro Solana wrote:
On Thu, 2003-11-27 at 21:00, Russell King wrote:
quoted
quoted
I believe that it, to change from strcpy() to strlcpy(), just
eliminates possibility of buffer-overrun.
While this is 100% correct, the bit which raised my attention was the
original message which didn't seem to show that the above had been
considered.
Well, I can't see the difference between using strcpy() and strlcpy().
You misunderstand me. Consider the difference between:
strcpy(d, s)
strlcpy(d, s, sizeof(d));
strncpy(d, s, sizeof(d));
strncpy zeros the remainder of d if strlen(s) < sizeof(d), but does not
zero terminate the buffer if strlen(s) == sizeof(d). (Note: this is
how strncpy under the Linux kernel is supposed to work, and yes, the
generic strncpy version in lib/string.c is still buggy.)
strlcpy copies up to the smaller of strlen(s)-1 and sizeof(d)-1, and
ensures that the string is null terminated. If strlen(s) < sizeof(d)-1,
bytes in d will not be written.
Note my final sentence there. Consider the following:
char foo[256];
strlcpy(foo, "hello", sizeof(foo);
copy_to_user(uptr, foo, sizeof(foo));
That ends up writing uninitialised kernel data to (unprivileged) user
space. So would strcpy() used in that situation.
strncpy() on the other hand, will zero the rest of the buffer (on x86
at least) but you'll have to manually ensure that there is a terminator
on the end. Or, you use strlcpy but memset the entire space you're
copying the string into beforehand, which could be wasteful.
Note: we should really fix the generic strncpy() - there are places in
the kernel source which rely on the x86 strncpy() behaviour today (eg,
binfmt_*.c core file generation.)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
From: Felipe Alfaro Solana <hidden> Date: 2003-11-27 23:03:36
On Thu, 2003-11-27 at 23:19, Russell King wrote:
You misunderstand me. Consider the difference between:
OK, it's perfectly clear now :-)
Note my final sentence there. Consider the following:
char foo[256];
strlcpy(foo, "hello", sizeof(foo);
copy_to_user(uptr, foo, sizeof(foo));
That ends up writing uninitialised kernel data to (unprivileged) user
space. So would strcpy() used in that situation.
strncpy() on the other hand, will zero the rest of the buffer (on x86
at least) but you'll have to manually ensure that there is a terminator
on the end. Or, you use strlcpy but memset the entire space you're
copying the string into beforehand, which could be wasteful.
Note: we should really fix the generic strncpy() - there are places in
the kernel source which rely on the x86 strncpy() behaviour today (eg,
binfmt_*.c core file generation.)
So, as I see:
1. We should fix strncpy()
2. I should replace strlcpy() with strncpy() in my patches.
In article [off-list ref] (at Fri, 28 Nov 2003 00:03:29 +0100), Felipe Alfaro Solana [off-list ref] says:
So, as I see:
1. We should fix strncpy()
2. I should replace strlcpy() with strncpy() in my patches.
I think it is NOT correct.
It SEEMS unsafe to use strncpy() even if it terminated
string correctly.
So, I'd suggest to replace
strlcpy(dst, src, len);
with
1) strlcpy0(dst, src, len);
where strlcpy0() is provided in my previous mail,
or with
2) memset(dst, 0, len);
strncpy(dst, src, len);
(or say, strncpy0())
or, with
3) if (len)
strncpy(dst, src, len - 1);
dst[len] = 0;
(or, say, strncpy0()).
--yoshfuji
From: Felipe Alfaro Solana <hidden> Date: 2003-11-28 09:54:33
On Fri, 2003-11-28 at 01:40, YOSHIFUJI Hideaki / 吉藤英明 wrote:
In article [off-list ref] (at Fri, 28 Nov 2003 09:26:42 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 [off-list ref] says:
quoted
quoted
3) if (len)
strncpy(dst, src, len - 1);
dst[len] = 0;
grr, another mistake...:
if (len) {
strncpy(dst, src, len - 1);
dst[len - 1];
}
----------------
1) use strlcpy0(dst, src, len)
size_t strlcpy0(char *dst, const char *src, size_t maxlen)
{
size_t len = strlcpy(dst, src, maxlen);
if (likely(maxlen != 0) && len < maxlen - 1)
memset(dst + len + 1, 0, maxlen - len - 1);
}
2a) use strncpy0(dst, src, len)
char *strncpy0(char *dst, const char *src, size_t maxlen)
{
memset(dst, 0, maxlen);
if (likely(maxlen != 0))
strncpy(dst, src, maxlen - 1);
}
2b) fix strncpy() to zero-out rest of destination buffer
and use strncpy0(dst, src, len)
char *strncpy0(char *dst, const char *src, size_t maxlen)
{
if (likely(maxlen != 0)) {
strncpy(dst, src, maxlen - 1);
dst[maxlen - 1] = 0;
}
}
I prefer 1 > 2b >> 2a.
What about coding a new function str0cpy()?
/**
* str0cpy - Copy a length-limited, %NUL-terminated string
* @dest: Where to copy the string to
* @src: Where to copy the string from
* @count: The maximum number of bytes to copy
*/
char * str0cpy(char * dest,const char *src,size_t count)
{
if (count) {
char *tmp = dest;
count--;
while (count) {
if ((*tmp = *src) != 0) src++;
tmp++;
count--;
}
*tmp = 0;
}
return dest;
}
It behaves like strncpy() but correctly null-terminates the string in
every case.