Patchwork [PATCHv2,03/22] block: avoid buffer overrun by using pstrcpy, not strncpy

login
register
mail settings
Submitter Jim Meyering
Date May 30, 2012, 7:46 a.m.
Message ID <1338364001-13892-4-git-send-email-jim@meyering.net>
Download mbox | patch
Permalink /patch/161880/
State New
Headers show

Comments

Jim Meyering - May 30, 2012, 7:46 a.m.
From: Jim Meyering <meyering@redhat.com>

Also, use PATH_MAX, rather than the arbitrary 1024.
Using PATH_MAX is more consistent with other filename-related
variables in this file, like backing_filename and tmp_filename.

Acked-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Stefan Weil - May 30, 2012, 3:35 p.m.
Am 30.05.2012 09:46, schrieb Jim Meyering:
> From: Jim Meyering<meyering@redhat.com>
>
> Also, use PATH_MAX, rather than the arbitrary 1024.
> Using PATH_MAX is more consistent with other filename-related
> variables in this file, like backing_filename and tmp_filename.
>
> Acked-by: Kevin Wolf<kwolf@redhat.com>
> Signed-off-by: Jim Meyering<meyering@redhat.com>
> ---
>   block.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index af2ab4f..efc7071 100644
> --- a/block.c
> +++ b/block.c
> @@ -1232,7 +1232,7 @@ int bdrv_commit(BlockDriverState *bs)
>       int n, ro, open_flags;
>       int ret = 0, rw_ret = 0;
>       uint8_t *buf;
> -    char filename[1024];
> +    char filename[PATH_MAX];
>       BlockDriverState *bs_rw, *bs_ro;
>
>       if (!drv)
> @@ -1252,7 +1252,8 @@ int bdrv_commit(BlockDriverState *bs)
>
>       backing_drv = bs->backing_hd->drv;
>       ro = bs->backing_hd->read_only;
> -    strncpy(filename, bs->backing_hd->filename, sizeof(filename));
> +    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
> +    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
>       open_flags =  bs->backing_hd->open_flags;
>
>       if (ro) {

PATH_MAX can have any value, from 259 or 512 on Windows to
4096 on Linux or even more.

I usually avoid arrays with more than some hundred bytes on the stack.
Even if the stack is large enough, it's not good for caching.

Of course, avoiding arbitrary limits like PATH_MAX would be best.

Using a QEMU_PATH_MAX which is the same for all hosts might be
the second best solution.

Regards,
Stefan Weil
Jim Meyering - May 30, 2012, 3:58 p.m.
Stefan Weil wrote:
> Am 30.05.2012 09:46, schrieb Jim Meyering:
>> From: Jim Meyering<meyering@redhat.com>
>>
>> Also, use PATH_MAX, rather than the arbitrary 1024.
>> Using PATH_MAX is more consistent with other filename-related
>> variables in this file, like backing_filename and tmp_filename.
>>
>> Acked-by: Kevin Wolf<kwolf@redhat.com>
>> Signed-off-by: Jim Meyering<meyering@redhat.com>
>> ---
>>   block.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index af2ab4f..efc7071 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1232,7 +1232,7 @@ int bdrv_commit(BlockDriverState *bs)
>>       int n, ro, open_flags;
>>       int ret = 0, rw_ret = 0;
>>       uint8_t *buf;
>> -    char filename[1024];
>> +    char filename[PATH_MAX];
>>       BlockDriverState *bs_rw, *bs_ro;
>>
>>       if (!drv)
>> @@ -1252,7 +1252,8 @@ int bdrv_commit(BlockDriverState *bs)
>>
>>       backing_drv = bs->backing_hd->drv;
>>       ro = bs->backing_hd->read_only;
>> -    strncpy(filename, bs->backing_hd->filename, sizeof(filename));
>> +    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
>> +    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
>>       open_flags =  bs->backing_hd->open_flags;
>>
>>       if (ro) {
>
> PATH_MAX can have any value, from 259 or 512 on Windows to
> 4096 on Linux or even more.

We've seen PATH_MAX values of 32KiB, and even INT_MAX.
On the Hurd it wasn't defined at all for many years --
that may still be the case.  It's enough of a portability
rats nest that we've removed all non-advisory uses from the
100+ programs in the coreutils package.

I'm 100% with you that PATH_MAX is best avoided, but it's certainly
an improvement over a hard-coded constant like 1024 -- who knows
if/when it may mistakenly used as an array dimension supposedly
adequate to hold a PATH_MAX=4096-length buffer.

As the commit log comment suggests, I'd prefer consistency first
(at least here), with a larger scope clean-up effort to do something
about all of these:

    $ git grep -E '\[(4096|2048|1024|512)\]'|wc -l
    135

But if you'd prefer, I'm happy to revert that part of the above change.
It certainly shouldn't hold up the buffer overrun fix.

> I usually avoid arrays with more than some hundred bytes on the stack.
> Even if the stack is large enough, it's not good for caching.
>
> Of course, avoiding arbitrary limits like PATH_MAX would be best.
>
> Using a QEMU_PATH_MAX which is the same for all hosts might be
> the second best solution.

Patch

diff --git a/block.c b/block.c
index af2ab4f..efc7071 100644
--- a/block.c
+++ b/block.c
@@ -1232,7 +1232,7 @@  int bdrv_commit(BlockDriverState *bs)
     int n, ro, open_flags;
     int ret = 0, rw_ret = 0;
     uint8_t *buf;
-    char filename[1024];
+    char filename[PATH_MAX];
     BlockDriverState *bs_rw, *bs_ro;

     if (!drv)
@@ -1252,7 +1252,8 @@  int bdrv_commit(BlockDriverState *bs)

     backing_drv = bs->backing_hd->drv;
     ro = bs->backing_hd->read_only;
-    strncpy(filename, bs->backing_hd->filename, sizeof(filename));
+    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
+    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
     open_flags =  bs->backing_hd->open_flags;

     if (ro) {