On 10/08/2018 06:31 PM, Max Reitz wrote: > On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote: >> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat >> an unpredictable amount of memory on corrupted table entries, which are >> referencing regions far beyond the end of file. >> >> Prevent this, by skipping such regions from further processing. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> block/qcow2-refcount.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 615847eb09..566c19fbfa 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, >> BdrvCheckResult *res, >> { >> BDRVQcow2State *s = bs->opaque; >> uint64_t start, last, cluster_offset, k, refcount; >> + int64_t file_len; >> int ret; >> >> if (size <= 0) { >> return 0; >> } >> >> + file_len = bdrv_getlength(bs->file->bs); >> + if (file_len < 0) { >> + return file_len; >> + } > > Doesn't this slow things down? Can we not cache the length somewhere > and update it whenever the image is modified?
hmm. bdrv_getlength is used everywhere in Qemu, and I don't think it is good idea to improve it locally for these series. If we can improve it somehow with a cache or something like this, it should be done for all users and therefore it is outside of these series.. > >> + >> + if (offset + size - file_len > s->cluster_size) { >> + fprintf(stderr, "ERROR: counting reference for region exceeding the >> " >> + "end of the file by more than one cluster: offset 0x%" >> PRIx64 >> + " size 0x%" PRIx64 "\n", offset, size); > > Why is one cluster OK? Is there a specific case you're trying to catch > here? raw file under qcow2 may be not aligned in real size to qcow2 cluster, as I understand, it's normal for the last cluster to be semi-allocated > > Max > >> + res->corruptions++; >> + return 0; >> + } >> + >> start = start_of_cluster(s, offset); >> last = start_of_cluster(s, offset + size - 1); >> for(cluster_offset = start; cluster_offset <= last; >> > >