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!
> >> +
> >> + /* 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!).
pgpzXR_Lcq_Ea.pgp
Description: PGP signature
