| Message ID | 1399174300-27583-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 04.05.2014 um 05:31 hat Max Reitz geschrieben: > If the very first allocation has a length of 0, the free_cluster_index > is still 0 after the for loop, which means that subtracting one from it > will underflow and signal an invalid range of clusters by returning > -EFBIG. However, there is no such range, as its length is 0. > > Fix this by preventing underflows on free_cluster_index during the > check. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Heh, I wondered about this when I reviewed that other patch, and came to the conclusion that it probably doesn't happen. Did you find a case where it does happen in fact? Anyway, this can't hurt: Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On Sun, May 04, 2014 at 05:31:40AM +0200, Max Reitz wrote: > If the very first allocation has a length of 0, the free_cluster_index > is still 0 after the for loop, which means that subtracting one from it > will underflow and signal an invalid range of clusters by returning > -EFBIG. However, there is no such range, as its length is 0. > > Fix this by preventing underflows on free_cluster_index during the > check. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-refcount.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On 05.05.2014 11:36, Kevin Wolf wrote: > Am 04.05.2014 um 05:31 hat Max Reitz geschrieben: >> If the very first allocation has a length of 0, the free_cluster_index >> is still 0 after the for loop, which means that subtracting one from it >> will underflow and signal an invalid range of clusters by returning >> -EFBIG. However, there is no such range, as its length is 0. >> >> Fix this by preventing underflows on free_cluster_index during the >> check. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > Heh, I wondered about this when I reviewed that other patch, and came to > the conclusion that it probably doesn't happen. Did you find a case > where it does happen in fact? Yes, deleting the last internal snapshot results in a 0-byte allocation for the new (empty) snapshot table. If this is the first allocation after an image has been opened (which is the case for qemu-img snapshot -d), you'll receive the “File too big” error message (which confused me quite a bit at first, as I wasn't specifically testing this series). Max > Anyway, this can't hurt: > > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index e79895d..9507aef 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -656,7 +656,9 @@ retry: /* Make sure that all offsets in the "allocated" range are representable * in an int64_t */ - if (s->free_cluster_index - 1 > (INT64_MAX >> s->cluster_bits)) { + if (s->free_cluster_index > 0 && + s->free_cluster_index - 1 > (INT64_MAX >> s->cluster_bits)) + { return -EFBIG; }
If the very first allocation has a length of 0, the free_cluster_index is still 0 after the for loop, which means that subtracting one from it will underflow and signal an invalid range of clusters by returning -EFBIG. However, there is no such range, as its length is 0. Fix this by preventing underflows on free_cluster_index during the check. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2-refcount.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)