On Thu, Nov 20, 2014 at 06:06:23PM +0100, Max Reitz wrote: > @@ -116,20 +137,24 @@ int64_t qcow2_get_refcount(BlockDriverState *bs, > int64_t cluster_index) > } > > ret = qcow2_cache_get(bs, s->refcount_block_cache, refcount_block_offset, > - (void**) &refcount_block); > + &refcount_block); > if (ret < 0) { > return ret; > } > > block_index = cluster_index & (s->refcount_block_size - 1); > - refcount = be16_to_cpu(refcount_block[block_index]); > + refcount = s->get_refcount(refcount_block, block_index); > > - ret = qcow2_cache_put(bs, s->refcount_block_cache, > - (void**) &refcount_block); > + ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); > if (ret < 0) { > return ret; > } > > + if (refcount < 0) { > + /* overflow */ > + return -ERANGE; > + }
I guess this is because QEMU uses int64_t but the maximum refcount size is 64 bits. If we refuse to open files with 64-bit refcounts, can we drop this check? > @@ -583,7 +608,12 @@ static int QEMU_WARN_UNUSED_RESULT > update_refcount(BlockDriverState *bs, > /* we can update the count and save it */ > block_index = cluster_index & (s->refcount_block_size - 1); > > - refcount = be16_to_cpu(refcount_block[block_index]); > + refcount = s->get_refcount(refcount_block, block_index); > + if (refcount < 0) { > + ret = -ERANGE; > + goto fail; > + } Here again. > @@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs, > } > } > > - if (++(*refcount_table)[k] == 0) { > + refcount = s->get_refcount(*refcount_table, k); Here the refcount < 0 check is missing. That's why it would be simpler to eliminate the refcount < 0 case entirely. > @@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *bs, > BdrvCheckResult *res, > continue; > } > > - refcount2 = refcount_table[i]; > + refcount2 = s->get_refcount(refcount_table, i); Missing here too and in other places. > +typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array, > + uint64_t index); Should the return type be int64_t? > +typedef void Qcow2SetRefcountFunc(void *refcount_array, > + uint64_t index, uint64_t value); Should value's type be int64_t? Just because the type is unsigned doesn't make (uint64_t)-1ULL a valid value.
pgpJ0KoLXdUpe.pgp
Description: PGP signature