On 08.10.18 22:22, Vladimir Sementsov-Ogievskiy wrote: > > > 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 <[email protected]> >>> --- >>> 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..
I wanted to write: Sure it's used everywhere, but usually that is before
someone performs some I/O, so it isn't too bad. But this is a function
that's suppose to just increment a couple of values in memory, which is
different.
However, I put the "wanted to write" prefix there, because: I knew that
we already have a central cache for bdrv_getlength(), but it isn't used
when the block driver reports has_variable_length as true. I thought
file-posix did that. But it only does so for CD-ROM devices.
So I think it should be OK to call the function here, yes.
>>> +
>>> + 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
Ah, that's true, thanks. I'd appreciate a comment here, though, and in
that case I think we don't need to check whether the reference is off by
more than a cluster, but whether it's off by a cluster or more (so >=).
Max
signature.asc
Description: OpenPGP digital signature
