[1/9] block/vpc: Make vpc_open() read the full dynamic header

The dynamic header's size is 1024 bytes.

vpc_open() reads only the 512 bytes of the dynamic header into buf[].
Works, because it doesn't actually access the second half.  However, a
colleague told me that GCC 11 warns:

    ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]

Clean up to read the full header.

Rename buf[] to dyndisk_header_buf[] while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On 17.12.20 17:19, Markus Armbruster wrote:
> The dynamic header's size is 1024 bytes.
> 
> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
> Works, because it doesn't actually access the second half.  However, a
> colleague told me that GCC 11 warns:
> 
>      ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
> 
> Clean up to read the full header.
> 
> Rename buf[] to dyndisk_header_buf[] while there.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vpc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 1ab55f9287..2fcf3f6283 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       bool use_chs;
> -    uint8_t buf[HEADER_SIZE];
> +    uint8_t dyndisk_header_buf[1024];

Perhaps this should be heap-allocated, but I don’t know whether qemu has 
ever established a (perhaps just inofficial) threshold on what goes on 
the stack and what goes on the heap.

>       uint32_t checksum;
>       uint64_t computed_size;
>       uint64_t pagetable_size;
> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       if (disk_type == VHD_DYNAMIC) {
> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> -                         HEADER_SIZE);
> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
> +                         dyndisk_header_buf, 1024);

sizeof() would be better, but I see that’s what patch 6 is for.

>           if (ret < 0) {
>               error_setg(errp, "Error reading dynamic VHD header");
>               goto fail;
>           }
>   
> -        dyndisk_header = (VHDDynDiskHeader *) buf;
> +        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
>   
>           if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
>               error_setg(errp, "Invalid header magic");

Reviewed-by: Max Reitz <mreitz@redhat.com>

Max Reitz <mreitz@redhat.com> writes:

> On 17.12.20 17:19, Markus Armbruster wrote:
>> The dynamic header's size is 1024 bytes.
>> 
>> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
>> Works, because it doesn't actually access the second half.  However, a
>> colleague told me that GCC 11 warns:
>> 
>>      ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
>> 
>> Clean up to read the full header.
>> 
>> Rename buf[] to dyndisk_header_buf[] while there.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vpc.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 1ab55f9287..2fcf3f6283 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>       QemuOpts *opts = NULL;
>>       Error *local_err = NULL;
>>       bool use_chs;
>> -    uint8_t buf[HEADER_SIZE];
>> +    uint8_t dyndisk_header_buf[1024];
>
> Perhaps this should be heap-allocated, but I don’t know whether qemu has 
> ever established a (perhaps just inofficial) threshold on what goes on 
> the stack and what goes on the heap.

There is no official per-function limit.  I generally don't worry about
a few kilobytes unless it's recursive.  We have many, many functions
with stack frames in the order of a kilobyte.  Our coroutine and thread
stacks are reasonable (corotine stacks are 1MiB, the default thread
stack size 2MiB on x86-64, and I can't see code that sets a different
size).

>>       uint32_t checksum;
>>       uint64_t computed_size;
>>       uint64_t pagetable_size;
>> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>       }
>>   
>>       if (disk_type == VHD_DYNAMIC) {
>> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>> -                         HEADER_SIZE);
>> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
>> +                         dyndisk_header_buf, 1024);
>
> sizeof() would be better, but I see that’s what patch 6 is for.

Yes, but sizeof what?  VHDDynDiskHeader becomes usable for that only in
PATCH 5.

I chose to separate the buffers first, and only then tidy up their use.

>>           if (ret < 0) {
>>               error_setg(errp, "Error reading dynamic VHD header");
>>               goto fail;
>>           }
>>   
>> -        dyndisk_header = (VHDDynDiskHeader *) buf;
>> +        dyndisk_header = (VHDDynDiskHeader *)dyndisk_header_buf;
>>   
>>           if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
>>               error_setg(errp, "Invalid header magic");
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

On 18.12.20 14:49, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 17.12.20 17:19, Markus Armbruster wrote:
>>> The dynamic header's size is 1024 bytes.
>>>
>>> vpc_open() reads only the 512 bytes of the dynamic header into buf[].
>>> Works, because it doesn't actually access the second half.  However, a
>>> colleague told me that GCC 11 warns:
>>>
>>>       ../block/vpc.c:358:51: error: array subscript 'struct VHDDynDiskHeader[0]' is partly outside array bounds of 'uint8_t[512]' [-Werror=array-bounds]
>>>
>>> Clean up to read the full header.
>>>
>>> Rename buf[] to dyndisk_header_buf[] while there.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    block/vpc.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 1ab55f9287..2fcf3f6283 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -220,7 +220,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>>        QemuOpts *opts = NULL;
>>>        Error *local_err = NULL;
>>>        bool use_chs;
>>> -    uint8_t buf[HEADER_SIZE];
>>> +    uint8_t dyndisk_header_buf[1024];
>>
>> Perhaps this should be heap-allocated, but I don’t know whether qemu has
>> ever established a (perhaps just inofficial) threshold on what goes on
>> the stack and what goes on the heap.
> 
> There is no official per-function limit.  I generally don't worry about
> a few kilobytes unless it's recursive.  We have many, many functions
> with stack frames in the order of a kilobyte.

Which doesn’t mean it’s what we want.  But I suppose in respect to my 
implicit question that means we don’t have any hard limits.

> Our coroutine and thread
> stacks are reasonable (corotine stacks are 1MiB, the default thread
> stack size 2MiB on x86-64, and I can't see code that sets a different
> size).

I’m not too worried about some on-stack buffers in functions like *open 
and *create.  It’s just that I would have put it on the heap, probably.

(Speaking of coroutine stack sizes, I remember a recent bug report on 
how long a backing chain may become, and that one of the limiting 
factors is that the coroutine stack eventually overflows. *shrug*)

>>>        uint32_t checksum;
>>>        uint64_t computed_size;
>>>        uint64_t pagetable_size;
>>> @@ -340,14 +340,14 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>>>        }
>>>    
>>>        if (disk_type == VHD_DYNAMIC) {
>>> -        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>>> -                         HEADER_SIZE);
>>> +        ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset),
>>> +                         dyndisk_header_buf, 1024);
>>
>> sizeof() would be better, but I see that’s what patch 6 is for.
> 
> Yes, but sizeof what?  VHDDynDiskHeader becomes usable for that only in
> PATCH 5.

sizeof(dyndisk_header_buf).

> I chose to separate the buffers first, and only then tidy up their use.

Understood.  It’s just that I noticed, then looked further in the patch 
series to see whether you’d clean up the magic number, and indeed you 
did in patch 6. :)

Max