Re: [Qemu-devel] [PATCH] qcow2: Fix fail path in realloc_refcount_block()

[Laszlo Ersek <lersek@redhat.com>]

On 03/16/14 00:26, Benoît Canet wrote:
> The Saturday 15 Mar 2014 à 21:55:54 (+0100), Max Reitz wrote :
>> If qcow2_alloc_clusters() fails, new_offset and ret will both be
>> negative after the fail label, thus passing the first if condition and
>> subsequently resulting in a call of qcow2_free_clusters() with an
>> invalid (negative) offset parameter. Fix this by checking for new_offset
>> being positive instead.
>>
>> While we're at it, clean up the whole fail path. qcow2_cache_put()
>> actually can never fail, hence the return value can safely be ignored
>> (aside from asserting that it indeed did not fail).
>>
>> Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to
>> qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  block/qcow2-refcount.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 6151148..b111319 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -1440,20 +1440,22 @@ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>>      }
>>  
>>  fail:
>> -    if (new_offset && (ret < 0)) {
>> -        qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> -                QCOW2_DISCARD_ALWAYS);
>> -    }
>>      if (refcount_block) {
>> -        if (ret < 0) {
>> -            qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> -        } else {
>> -            ret = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> -        }
>> +        /* This should never fail, as it would only do so if the given refcount
>> +         * block cannot be found in the cache. As this is impossible as long as
>> +         * there are no bugs, assert the success. */
>> +        int tmp = qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
>> +        assert(tmp == 0);
>>      }
>> +
>>      if (ret < 0) {
>> +        if (new_offset > 0) {
>> +            qcow2_free_clusters(bs, new_offset, s->cluster_size,
>> +                                QCOW2_DISCARD_OTHER);
>> +        }
>>          return ret;
>>      }
>> +
>>      return new_offset;
>>  }
>>  
>> -- 
>> 1.9.0
>>
>>
> 
> In fact in wonder if these if after the fail label could be replaced by a proper
> goto chain.

That was my first idea too. I didn't propose it to Max because it's
complicated by the fact that while you need to release the cache entry
in any case (both success & failure), you roll back the cluster alloc
only in case of failure. Using a proper goto chain (without any "if"s)
means that you'd have to keep it fully separate from the success path,
hence duplicate the cache entry release on the success path (including
the assert()). The ifs above allow you to share the action.

It's a matter of taste; I usually like to code actions only once. But I
don't insist of course.

Laszlo