On 10/30/14, Stefan Hajnoczi <stefa...@redhat.com> wrote:
> On Wed, Oct 29, 2014 at 09:30:31PM +0800, Xiaodong Gong wrote:
>> On 10/28/14, Stefan Hajnoczi <stefa...@gmail.com> 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.
