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

[Jim Meyering]

Stefan Weil wrote:
> Am 30.05.2012 09:46, schrieb Jim Meyering:
>> From: Jim Meyering<address@hidden>
>>
>> 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<address@hidden>
>> Signed-off-by: Jim Meyering<address@hidden>
>> ---
>>   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.