[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2 03/22] block: avoid buffer overrun by using ps
From: |
Jim Meyering |
Subject: |
Re: [Qemu-devel] [PATCHv2 03/22] block: avoid buffer overrun by using pstrcpy, not strncpy |
Date: |
Wed, 30 May 2012 17:58:09 +0200 |
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.
- [Qemu-devel] [PATCHv2 00/22] strncpy: best avoided, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 03/22] block: avoid buffer overrun by using pstrcpy, not strncpy, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 01/22] scsi, pci, qdev, isa-bus, sysbus: don't let *_get_fw_dev_path return NULL, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 07/22] lm32: avoid buffer overrun, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 02/22] sparc: use g_strdup in place of unchecked strdup, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 04/22] sheepdog: avoid a few buffer overruns, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 05/22] vmdk: relative_path: use pstrcpy in place of strncpy, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 08/22] os-posix: avoid buffer overrun, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 06/22] hw/9pfs: avoid buffer overrun, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 10/22] linux-user: remove two unchecked uses of strdup, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 16/22] qemu-ga: prefer pstrcpy: consistently NUL-terminate ifreq.ifr_name, Jim Meyering, 2012/05/30
- [Qemu-devel] [PATCHv2 12/22] bt: replace fragile snprintf use and unwarranted strncpy, Jim Meyering, 2012/05/30