Re: [Qemu-devel] [PATCH v5] Support vhd type VHD_DIFFERENCING

[Stefan Hajnoczi]

On Wed, Oct 29, 2014 at 09:30:31PM +0800, Xiaodong Gong wrote:
> On 10/28/14, Stefan Hajnoczi <address@hidden> wrote:
> > On Wed, Oct 08, 2014 at 08:42:32PM +0800, Xiaodong Gong wrote:
> >> +#define PLATFORM_MACX 0x5863614d /* big endian */
> >> +#define PLATFORM_W2RU 0x75723257
...
> >> +static int vpc_read_backing_loc(VHDDynDiskHeader *dyndisk_header,
> >> +                                BlockDriverState *bs,
> >> +                                Error **errp)
> >> +{
> >> +    BDRVVPCState *s = bs->opaque;
> >> +    int64_t data_offset = 0;
> >> +    int data_length = 0;
> >> +    uint32_t platform;
> >> +    bool done = false;
> >> +    int parent_locator_offset = 0;
> >> +    int i;
> >> +    int ret = 0;
> >> +
> >> +    for (i = 0; i < PARENT_LOCATOR_NUM; i++) {
> >> +        data_offset =
> >> +            be64_to_cpu(dyndisk_header->parent_locator[i].data_offset);
> >> +        data_length =
> >> +            be32_to_cpu(dyndisk_header->parent_locator[i].data_length);
> >> +        platform = dyndisk_header->parent_locator[i].platform;
> >
> > be32_to_cpu() missing?
> 
> this platform is big-ending

QEMU compiles on both little-endian and big-endian hosts.  You cannot
define PLATFORM_* constants with the assumption that the host is
little-endian because it won't work on big-endian hosts!

> >> +
> >> +        /* Read location of backing file */
> >> +        if (platform == PLATFORM_MACX || platform == PLATFORM_W2RU) {
> >> +            if (data_offset > s->max_table_entries * s->block_size) {
> >> +                return -1;
> >> +            }
> >> +            if (data_length > BDRV_SECTOR_SIZE) {
> >> +                return -1;
> >> +            }
> >> +            ret = bdrv_pread(bs->file, data_offset, bs->backing_file,
> >> +                data_length);
> >
> > Please check data_length against bs->backing_file[] size before reading
> > into it.
> 
> upper data_length > BDRV_SECTOR_SIZE get this done

I know but that assumes that BDRV_SECTOR_SIZE will always be less than
sizeof(bs->backing_file[]) in the future.

There must never be a buffer overflow, ever, even in the future when
other parts of QEMU are changed.

It's safer to check the size of bs->backing_file[] explicitly.

> >
> >> +            if (ret < 0) {
> >> +                return ret;
> >> +            }
> >> +            bs->backing_file[data_length] = '\0';
> >> +        }
> >> +
> >> +        /* Convert location to ACSII string */
> >> +        if (platform == PLATFORM_MACX) {
> >> +            done = true;
> >> +
> >> +        } else if (platform == PLATFORM_W2RU) {
> >> +            /* Must be UTF16-LE to ASCII */
> >
> > I guess this is where you wanted to use iconv?
> 
> I used the iconv first time, but changed it to the following things.
> There are tow reasons, it could fail because the right codeset packet
> is not installed and it must be UTF16-LE to ASCII. How about your ?

I just wanted to make sure I understood the reason for #include
<iconv.h> correctly.

How about using glib's charset conversion function?  It seems a bit
hacky to implement it manually (while ignoring the error cases if a
UTF16-LE character doesn't map to ASCII!).

pgpgN9UqtYHjc.pgp
Description: PGP signature