Message ID | 1359025358-5725-5-git-send-email-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
Kevin Wolf <kwolf@redhat.com> writes: > Return -errno instead of -1 on errors. While touching the > code, fix even some more memory leaks than in the other drivers... LOL > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/dmg.c | 136 ++++++++++++++++++++++++++++++++++++++++++++-------------- > 1 files changed, 103 insertions(+), 33 deletions(-) > > diff --git a/block/dmg.c b/block/dmg.c > index ac397dc..d47d2d8 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -57,29 +57,53 @@ static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename) > return 0; > } > > -static off_t read_off(BlockDriverState *bs, int64_t offset) > +static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result) > { > - uint64_t buffer; > - if (bdrv_pread(bs->file, offset, &buffer, 8) < 8) > - return 0; > - return be64_to_cpu(buffer); > + uint64_t buffer; > + int ret; > + > + ret = bdrv_pread(bs->file, offset, &buffer, 8); > + if (ret < 0) { > + return ret; > + } > + > + *result = be64_to_cpu(buffer); > + return 0; > } > > -static off_t read_uint32(BlockDriverState *bs, int64_t offset) > +static int read_off(BlockDriverState *bs, int64_t offset, off_t *result) > +{ > + uint64_t buffer; > + int ret; > + > + ret = read_uint64(bs, offset, &buffer); > + *result = buffer; > + > + return ret; > +} > + > +static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result) > { > uint32_t buffer; Indentation of the line above is now off. > - if (bdrv_pread(bs->file, offset, &buffer, 4) < 4) > - return 0; > - return be32_to_cpu(buffer); > + int ret; > + > + ret = bdrv_pread(bs->file, offset, &buffer, 4); > + if (ret < 0) { > + return ret; > + } > + > + *result = be32_to_cpu(buffer); > + return 0; > } You're fixing bugs here. Please mention them in the commit message. Code before: * read_off() silently truncates when off_t is narrower than 64 bits, and can't distinguish between error and successful read of zero. * Three out of four callers of read_off() don't bother to check for failure. * read_uint32() can't distinguish between error and successful read of zero. Truncation isn't a problem, because off_t is surely at least 32 bits wide. * Two out of four callers of read_uint32() don't bother to check for failure. After: * All callers check for errors. * read_uint64() and read_uint32() are fine. * read_off() silently truncates when off_t is narrower than 64 bits. Why isn't that a problem? Hmm, the only remaining caller reads into off_t info_begin, which is then passed to int64_t parameters, either directly, or via off_t offset. What about replacing the remaining off_t by int64_t? > > static int dmg_open(BlockDriverState *bs, int flags) > { > BDRVDMGState *s = bs->opaque; > off_t info_begin,info_end,last_in_offset,last_out_offset; > - uint32_t count; > + uint32_t count, tmp; > uint32_t max_compressed_size=1,max_sectors_per_chunk=1,i; > int64_t offset; > + int ret; > > bs->read_only = 1; > s->n_chunks = 0; > @@ -88,21 +112,32 @@ static int dmg_open(BlockDriverState *bs, int flags) > /* read offset of info blocks */ > offset = bdrv_getlength(bs->file); > if (offset < 0) { > + ret = offset; > goto fail; > } > offset -= 0x1d8; > > - info_begin = read_off(bs, offset); > - if (info_begin == 0) { > - goto fail; > + ret = read_off(bs, offset, &info_begin); > + if (ret < 0) { > + goto fail; > + } else if (info_begin == 0) { > + ret = -EINVAL; > + goto fail; > } > > - if (read_uint32(bs, info_begin) != 0x100) { > + ret = read_uint32(bs, info_begin, &tmp); > + if (ret < 0) { > + goto fail; > + } else if (tmp != 0x100) { > + ret = -EINVAL; > goto fail; > } > > - count = read_uint32(bs, info_begin + 4); > - if (count == 0) { > + ret = read_uint32(bs, info_begin + 4, &count); > + if (ret < 0) { > + goto fail; > + } else if (count == 0) { > + ret = -EINVAL; > goto fail; > } > info_end = info_begin + count; > @@ -114,12 +149,20 @@ static int dmg_open(BlockDriverState *bs, int flags) > while (offset < info_end) { > uint32_t type; > > - count = read_uint32(bs, offset); > - if(count==0) > - goto fail; > + ret = read_uint32(bs, offset, &count); > + if (ret < 0) { > + goto fail; > + } else if (count == 0) { > + ret = -EINVAL; > + goto fail; > + } > offset += 4; > > - type = read_uint32(bs, offset); > + ret = read_uint32(bs, offset, &type); > + if (ret < 0) { > + goto fail; > + } > + > if (type == 0x6d697368 && count >= 244) { > int new_size, chunk_count; > > @@ -134,8 +177,11 @@ static int dmg_open(BlockDriverState *bs, int flags) > s->sectors = g_realloc(s->sectors, new_size); > s->sectorcounts = g_realloc(s->sectorcounts, new_size); > > - for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) { > - s->types[i] = read_uint32(bs, offset); > + for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) { Since you're touching this line anyway, you could treat the poor thing to a few spaces. > + ret = read_uint32(bs, offset, &s->types[i]); > + if (ret < 0) { > + goto fail; > + } > offset += 4; > if(s->types[i]!=0x80000005 && s->types[i]!=1 && s->types[i]!=2) { > if(s->types[i]==0xffffffff) { > @@ -149,17 +195,31 @@ static int dmg_open(BlockDriverState *bs, int flags) > } > offset += 4; > > - s->sectors[i] = last_out_offset+read_off(bs, offset); > - offset += 8; > - > - s->sectorcounts[i] = read_off(bs, offset); > - offset += 8; > - > - s->offsets[i] = last_in_offset+read_off(bs, offset); > - offset += 8; > - > - s->lengths[i] = read_off(bs, offset); > - offset += 8; > + ret = read_uint64(bs, offset, &s->sectors[i]); > + if (ret < 0) { > + goto fail; > + } > + s->sectors[i] += last_out_offset; > + offset += 8; > + > + ret = read_uint64(bs, offset, &s->sectorcounts[i]); > + if (ret < 0) { > + goto fail; > + } > + offset += 8; > + > + ret = read_uint64(bs, offset, &s->offsets[i]); > + if (ret < 0) { > + goto fail; > + } > + s->offsets[i] += last_in_offset; > + offset += 8; > + > + ret = read_uint64(bs, offset, &s->lengths[i]); > + if (ret < 0) { > + goto fail; > + } > + offset += 8; > > if(s->lengths[i]>max_compressed_size) > max_compressed_size = s->lengths[i]; > @@ -173,15 +233,25 @@ static int dmg_open(BlockDriverState *bs, int flags) > /* initialize zlib engine */ > s->compressed_chunk = g_malloc(max_compressed_size+1); > s->uncompressed_chunk = g_malloc(512*max_sectors_per_chunk); > - if(inflateInit(&s->zstream) != Z_OK) > - goto fail; > + if(inflateInit(&s->zstream) != Z_OK) { > + ret = -EINVAL; As in PATCH 2/4: good enough. > + goto fail; > + } > > s->current_chunk = s->n_chunks; > > qemu_co_mutex_init(&s->lock); > return 0; > + > fail: > - return -1; > + g_free(s->types); > + g_free(s->offsets); > + g_free(s->lengths); > + g_free(s->sectors); > + g_free(s->sectorcounts); > + g_free(s->compressed_chunk); > + g_free(s->uncompressed_chunk); > + return ret; > } > > static inline int is_sector_in_chunk(BDRVDMGState* s,
diff --git a/block/dmg.c b/block/dmg.c index ac397dc..d47d2d8 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -57,29 +57,53 @@ static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename) return 0; } -static off_t read_off(BlockDriverState *bs, int64_t offset) +static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result) { - uint64_t buffer; - if (bdrv_pread(bs->file, offset, &buffer, 8) < 8) - return 0; - return be64_to_cpu(buffer); + uint64_t buffer; + int ret; + + ret = bdrv_pread(bs->file, offset, &buffer, 8); + if (ret < 0) { + return ret; + } + + *result = be64_to_cpu(buffer); + return 0; } -static off_t read_uint32(BlockDriverState *bs, int64_t offset) +static int read_off(BlockDriverState *bs, int64_t offset, off_t *result) +{ + uint64_t buffer; + int ret; + + ret = read_uint64(bs, offset, &buffer); + *result = buffer; + + return ret; +} + +static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result) { uint32_t buffer; - if (bdrv_pread(bs->file, offset, &buffer, 4) < 4) - return 0; - return be32_to_cpu(buffer); + int ret; + + ret = bdrv_pread(bs->file, offset, &buffer, 4); + if (ret < 0) { + return ret; + } + + *result = be32_to_cpu(buffer); + return 0; } static int dmg_open(BlockDriverState *bs, int flags) { BDRVDMGState *s = bs->opaque; off_t info_begin,info_end,last_in_offset,last_out_offset; - uint32_t count; + uint32_t count, tmp; uint32_t max_compressed_size=1,max_sectors_per_chunk=1,i; int64_t offset; + int ret; bs->read_only = 1; s->n_chunks = 0; @@ -88,21 +112,32 @@ static int dmg_open(BlockDriverState *bs, int flags) /* read offset of info blocks */ offset = bdrv_getlength(bs->file); if (offset < 0) { + ret = offset; goto fail; } offset -= 0x1d8; - info_begin = read_off(bs, offset); - if (info_begin == 0) { - goto fail; + ret = read_off(bs, offset, &info_begin); + if (ret < 0) { + goto fail; + } else if (info_begin == 0) { + ret = -EINVAL; + goto fail; } - if (read_uint32(bs, info_begin) != 0x100) { + ret = read_uint32(bs, info_begin, &tmp); + if (ret < 0) { + goto fail; + } else if (tmp != 0x100) { + ret = -EINVAL; goto fail; } - count = read_uint32(bs, info_begin + 4); - if (count == 0) { + ret = read_uint32(bs, info_begin + 4, &count); + if (ret < 0) { + goto fail; + } else if (count == 0) { + ret = -EINVAL; goto fail; } info_end = info_begin + count; @@ -114,12 +149,20 @@ static int dmg_open(BlockDriverState *bs, int flags) while (offset < info_end) { uint32_t type; - count = read_uint32(bs, offset); - if(count==0) - goto fail; + ret = read_uint32(bs, offset, &count); + if (ret < 0) { + goto fail; + } else if (count == 0) { + ret = -EINVAL; + goto fail; + } offset += 4; - type = read_uint32(bs, offset); + ret = read_uint32(bs, offset, &type); + if (ret < 0) { + goto fail; + } + if (type == 0x6d697368 && count >= 244) { int new_size, chunk_count; @@ -134,8 +177,11 @@ static int dmg_open(BlockDriverState *bs, int flags) s->sectors = g_realloc(s->sectors, new_size); s->sectorcounts = g_realloc(s->sectorcounts, new_size); - for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) { - s->types[i] = read_uint32(bs, offset); + for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) { + ret = read_uint32(bs, offset, &s->types[i]); + if (ret < 0) { + goto fail; + } offset += 4; if(s->types[i]!=0x80000005 && s->types[i]!=1 && s->types[i]!=2) { if(s->types[i]==0xffffffff) { @@ -149,17 +195,31 @@ static int dmg_open(BlockDriverState *bs, int flags) } offset += 4; - s->sectors[i] = last_out_offset+read_off(bs, offset); - offset += 8; - - s->sectorcounts[i] = read_off(bs, offset); - offset += 8; - - s->offsets[i] = last_in_offset+read_off(bs, offset); - offset += 8; - - s->lengths[i] = read_off(bs, offset); - offset += 8; + ret = read_uint64(bs, offset, &s->sectors[i]); + if (ret < 0) { + goto fail; + } + s->sectors[i] += last_out_offset; + offset += 8; + + ret = read_uint64(bs, offset, &s->sectorcounts[i]); + if (ret < 0) { + goto fail; + } + offset += 8; + + ret = read_uint64(bs, offset, &s->offsets[i]); + if (ret < 0) { + goto fail; + } + s->offsets[i] += last_in_offset; + offset += 8; + + ret = read_uint64(bs, offset, &s->lengths[i]); + if (ret < 0) { + goto fail; + } + offset += 8; if(s->lengths[i]>max_compressed_size) max_compressed_size = s->lengths[i]; @@ -173,15 +233,25 @@ static int dmg_open(BlockDriverState *bs, int flags) /* initialize zlib engine */ s->compressed_chunk = g_malloc(max_compressed_size+1); s->uncompressed_chunk = g_malloc(512*max_sectors_per_chunk); - if(inflateInit(&s->zstream) != Z_OK) - goto fail; + if(inflateInit(&s->zstream) != Z_OK) { + ret = -EINVAL; + goto fail; + } s->current_chunk = s->n_chunks; qemu_co_mutex_init(&s->lock); return 0; + fail: - return -1; + g_free(s->types); + g_free(s->offsets); + g_free(s->lengths); + g_free(s->sectors); + g_free(s->sectorcounts); + g_free(s->compressed_chunk); + g_free(s->uncompressed_chunk); + return ret; } static inline int is_sector_in_chunk(BDRVDMGState* s,
Return -errno instead of -1 on errors. While touching the code, fix even some more memory leaks than in the other drivers... Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/dmg.c | 136 ++++++++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 103 insertions(+), 33 deletions(-)