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

[Xiaodong Gong]

On 10/30/14, Stefan Hajnoczi <address@hidden> wrote:
> 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!
>

ok

>> >> +
>> >> +        /* 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.

ok, i get it.

>
>> >
>> >> +            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!).
>

about not must use g_iconv, does it accept to convert manually ?
if error happened in mapping, it shows that this path name is not
legal as well.