Mailman 3 Re: [Python-Dev] cpython: Use strncat() instead of strcat() to silence some warnings. - Python-Dev

newer
Re: [Python-Dev] [Python-checkins]...

Re: [Python-Dev] cpython: Use strncat() instead of strcat() to silence some warnings.

older
Summary of Python tracker Issues

Serhiy Storchaka

20 Jul 2013 20 Jul '13

8:23 a.m.

20.07.13 15:12, christian.heimes написав(ла):

...

http://hg.python.org/cpython/rev/c92f4172d122 changeset: 84723:c92f4172d122 user: Christian Heimes date: Sat Jul 20 14:11:28 2013 +0200 summary: Use strncat() instead of strcat() to silence some warnings. CID 486616, CID 486617, CID 486615

files: Modules/ossaudiodev.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Modules/ossaudiodev.c b/Modules/ossaudiodev.c --- a/Modules/ossaudiodev.c +++ b/Modules/ossaudiodev.c @@ -245,7 +245,7 @@ int arg;

assert(strlen(fname) <= 30); - strcat(argfmt, fname); + strncat(argfmt, fname, 30); if (!PyArg_ParseTuple(args, argfmt, &arg)) return NULL;

@@ -270,7 +270,7 @@ int arg = 0;

assert(strlen(fname) <= 30); - strcat(argfmt, fname); + strncat(argfmt, fname, 30); if (!PyArg_ParseTuple(args, argfmt, &arg)) return NULL;

@@ -290,7 +290,7 @@ int rv;

assert(strlen(fname) <= 30); - strcat(argfmt, fname); + strncat(argfmt, fname, 30); if (!PyArg_ParseTuple(args, argfmt)) return NULL;

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

Show replies by date

Antoine Pitrou

20 Jul 20 Jul

8:36 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

On Sat, 20 Jul 2013 15:23:46 +0300 Serhiy Storchaka wrote:

...

20.07.13 15:12, christian.heimes написав(ла):

...
http://hg.python.org/cpython/rev/c92f4172d122 changeset: 84723:c92f4172d122 user: Christian Heimes date: Sat Jul 20 14:11:28 2013 +0200 summary: Use strncat() instead of strcat() to silence some warnings. CID 486616, CID 486617, CID 486615

[...]

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

So, for the record, this is roughly how Rasmus Lerdorf introduced a security hole in PHP 5.3.7: "For people asking me out-of-band what the screw-up was, it was pretty simple. I changed this code: memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strcat(passwd, "$"); to: memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strlcat(passwd, "$", 1); **because the Coverity static analyzer warned about using strcat** [emphasis mine] and we generally try to avoid naked strcat/strcpy in the codebase even though in this case it is safe to do." https://plus.google.com/113641248237520845183/posts/g68d9RvRA1i Regards Antoine.

Serhiy Storchaka

8:48 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

20.07.13 15:36, Antoine Pitrou написав(ла):

...

On Sat, 20 Jul 2013 15:23:46 +0300 Serhiy Storchaka wrote:

...
20.07.13 15:12, christian.heimes написав(ла):

...
http://hg.python.org/cpython/rev/c92f4172d122 changeset: 84723:c92f4172d122 user: Christian Heimes date: Sat Jul 20 14:11:28 2013 +0200 summary: Use strncat() instead of strcat() to silence some warnings. CID 486616, CID 486617, CID 486615

[...]

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

So, for the record, this is roughly how Rasmus Lerdorf introduced a security hole in PHP 5.3.7:

"For people asking me out-of-band what the screw-up was, it was pretty simple. I changed this code:

memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strcat(passwd, "$");

to:

memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strlcat(passwd, "$", 1);

**because the Coverity static analyzer warned about using strcat** [emphasis mine] and we generally try to avoid naked strcat/strcpy in the codebase even though in this case it is safe to do."

https://plus.google.com/113641248237520845183/posts/g68d9RvRA1i

strlcat != strncat. strlcat(dst, src, 1) actually do nothing.

Antoine Pitrou

9 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

On Sat, 20 Jul 2013 15:48:09 +0300 Serhiy Storchaka wrote:

...

20.07.13 15:36, Antoine Pitrou написав(ла):

...
On Sat, 20 Jul 2013 15:23:46 +0300 Serhiy Storchaka wrote:

...
20.07.13 15:12, christian.heimes написав(ла):

...
http://hg.python.org/cpython/rev/c92f4172d122 changeset: 84723:c92f4172d122 user: Christian Heimes date: Sat Jul 20 14:11:28 2013 +0200 summary: Use strncat() instead of strcat() to silence some warnings. CID 486616, CID 486617, CID 486615

[...]

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

So, for the record, this is roughly how Rasmus Lerdorf introduced a security hole in PHP 5.3.7:

"For people asking me out-of-band what the screw-up was, it was pretty simple. I changed this code:

memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strcat(passwd, "$");

to:

memcpy(passwd, MD5_MAGIC, MD5_MAGIC_LEN); strlcpy(passwd + MD5_MAGIC_LEN, sp, sl + 1); strlcat(passwd, "$", 1);

**because the Coverity static analyzer warned about using strcat** [emphasis mine] and we generally try to avoid naked strcat/strcpy in the codebase even though in this case it is safe to do."

https://plus.google.com/113641248237520845183/posts/g68d9RvRA1i

strlcat != strncat. strlcat(dst, src, 1) actually do nothing.

This is true. But the trigger in the "fix" was the same (trying to suppress a Coverity warning about strcat). Regards Antoine.

Christian Heimes

9:27 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

...

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

That's not how strncat() works. strncat(dest, src, n) writes n+1 chars to the end of dest: n chars from src and +1 for the final NUL char. For this reason dest must be large enough to hold strlen(dest) + n + 1 chars.

Serhiy Storchaka

9:56 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

20.07.13 16:27, Christian Heimes написав(ла):

...

...
This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

That's not how strncat() works. strncat(dest, src, n) writes n+1 chars to the end of dest: n chars from src and +1 for the final NUL char. For this reason dest must be large enough to hold strlen(dest) + n + 1 chars.

Oh, true. strncat() always results NUL-terminated string. It's strncpy() can produce not NUL-terminated string. Sorry for noise.

Christian Heimes

10:04 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

Am 20.07.2013 15:56, schrieb Serhiy Storchaka:

...

Oh, true. strncat() always results NUL-terminated string. It's strncpy() can produce not NUL-terminated string. Sorry for noise.

You're welcome! Better safe than sorry. :)

Christian Heimes

9:29 a.m.

New subject: cpython: Use strncat() instead of strcat() to silence some warnings.

Am 20.07.2013 14:23, schrieb Serhiy Storchaka:

...

This will wrong when strlen(fname) is 30. strncat() will copy only 30 bytes, without terminal NUL.

http://linux.die.net/man/3/strncat The strncat() function is similar, except that * it will use at most n bytes from src; and * src does not need to be null-terminated if it contains n or more bytes. If src contains n or more bytes, strncat() writes n+1 bytes to dest (n from src plus the terminating null byte). Therefore, the size of dest must be at least strlen(dest)+n+1.

3930

Age (days ago)

3930

Last active (days ago)

List overview

Download

7 comments

3 participants

participants (3)

Antoine Pitrou
Christian Heimes
Serhiy Storchaka

Re: [Python-Dev] cpython: Use strncat() instead of strcat() to silence some warnings.

tags

participants (3)