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

[Max Reitz]

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>