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

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

[Max Reitz]

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

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

[Benoît Canet]

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

I would say "actually should never fail".

> (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:

I don't like the label name because its real meaning is exit: it handle both
fail and regular exits.

> -    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;

The function documentation says:
/*
 * Allocates a new cluster for the given refcount block (represented by its
 * offset in the image file) and copies the current content there. This function
 * does _not_ decrement the reference count for the currently occupied cluster.
 *
 * This function prints an informative message to stderr on error (and returns
 * -errno); on success, 0 is returned.
 */
static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
                                      uint64_t offset)


So why are we returning new_offset on success ? 

>  }
>
> --
> 1.9.0
>
>

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

[Max Reitz]

On 16.03.2014 00:16, 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
> I would say "actually should never fail".

Hm, I'm not sure. Currently, it simply cannot fail. The assertion is 
there basically only for future code changes and to indicate to 
developers that the function is really known to succeed.

I'll try to make the sentence more specific.

>> (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:
> I don't like the label name because its real meaning is exit: it handle both
> fail and regular exits.

I'll try to reshape it into a nicer form of nested labels.

>> -    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;
> The function documentation says:
> /*
>   * Allocates a new cluster for the given refcount block (represented by its
>   * offset in the image file) and copies the current content there. This function
>   * does _not_ decrement the reference count for the currently occupied cluster.
>   *
>   * This function prints an informative message to stderr on error (and returns
>   * -errno); on success, 0 is returned.
>   */
> static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index,
>                                        uint64_t offset)
>
>
> So why are we returning new_offset on success ?

Because the comment is wrong. I'll fix it. ;-)


Thanks,

Max

>>   }
>>
>> --
>> 1.9.0
>>
>>

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

[Benoît Canet]

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.

Best regards

Benoît

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

[Laszlo Ersek]

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