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

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(-)

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

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.