| Message ID | 20210504152023.322862-9-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qcow2 check: check some reserved bits and subcluster bitmaps | expand |
On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote: > - use g_autofree for l1_table > - better name for size in bytes variable > - reduce code blocks nesting > - whitespaces, braces, newlines > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-refcount.c | 97 +++++++++++++++++++++--------------------- > 1 file changed, 49 insertions(+), 48 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 44fc0dd5dc..eb6de3dabd 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs, > int flags, BdrvCheckMode fix, bool active) > { > BDRVQcow2State *s = bs->opaque; > - uint64_t *l1_table = NULL, l2_offset, l1_size2; > + size_t l1_size_bytes = l1_size * L1E_SIZE; > + g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes); Note that this now happens... > + uint64_t l2_offset; > int i, ret; > > - l1_size2 = l1_size * L1E_SIZE; > + if (!l1_size) { > + return 0; ...before you validate whether l1_size is non-zero, which can result in g_try_malloc(0). Probably harmless, but it might be better if you declare g_autofree uint64_t *l1_table = NULL; and then initialize it via malloc only after the sanity check.
04.05.2021 22:53, Eric Blake wrote: > On 5/4/21 10:20 AM, Vladimir Sementsov-Ogievskiy wrote: >> - use g_autofree for l1_table >> - better name for size in bytes variable >> - reduce code blocks nesting >> - whitespaces, braces, newlines >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-refcount.c | 97 +++++++++++++++++++++--------------------- >> 1 file changed, 49 insertions(+), 48 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 44fc0dd5dc..eb6de3dabd 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs, >> int flags, BdrvCheckMode fix, bool active) >> { >> BDRVQcow2State *s = bs->opaque; >> - uint64_t *l1_table = NULL, l2_offset, l1_size2; >> + size_t l1_size_bytes = l1_size * L1E_SIZE; >> + g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes); > > Note that this now happens... > >> + uint64_t l2_offset; >> int i, ret; >> >> - l1_size2 = l1_size * L1E_SIZE; >> + if (!l1_size) { >> + return 0; > > ...before you validate whether l1_size is non-zero, which can result in > g_try_malloc(0). Probably harmless, but it might be better if you declare > g_autofree uint64_t *l1_table = NULL; > and then initialize it via malloc only after the sanity check. > Thinking a bit, another thing I don't like is that check if (l1_table == NULL) doesn't immediately follow g_try_malloc() call. So, will move it.
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 44fc0dd5dc..eb6de3dabd 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1864,71 +1864,72 @@ static int check_refcounts_l1(BlockDriverState *bs, int flags, BdrvCheckMode fix, bool active) { BDRVQcow2State *s = bs->opaque; - uint64_t *l1_table = NULL, l2_offset, l1_size2; + size_t l1_size_bytes = l1_size * L1E_SIZE; + g_autofree uint64_t *l1_table = g_try_malloc(l1_size_bytes); + uint64_t l2_offset; int i, ret; - l1_size2 = l1_size * L1E_SIZE; + if (!l1_size) { + return 0; + } /* Mark L1 table as used */ ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size, - l1_table_offset, l1_size2); + l1_table_offset, l1_size_bytes); if (ret < 0) { - goto fail; + return ret; + } + + if (l1_table == NULL) { + res->check_errors++; + return -ENOMEM; } /* Read L1 table entries from disk */ - if (l1_size2 > 0) { - l1_table = g_try_malloc(l1_size2); - if (l1_table == NULL) { - ret = -ENOMEM; - res->check_errors++; - goto fail; - } - ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2); - if (ret < 0) { - fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n"); - res->check_errors++; - goto fail; - } - for(i = 0;i < l1_size; i++) - be64_to_cpus(&l1_table[i]); + ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size_bytes); + if (ret < 0) { + fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n"); + res->check_errors++; + return ret; + } + + for (i = 0; i < l1_size; i++) { + be64_to_cpus(&l1_table[i]); } /* Do the actual checks */ - for(i = 0; i < l1_size; i++) { - l2_offset = l1_table[i]; - if (l2_offset) { - /* Mark L2 table as used */ - l2_offset &= L1E_OFFSET_MASK; - ret = qcow2_inc_refcounts_imrt(bs, res, - refcount_table, refcount_table_size, - l2_offset, s->cluster_size); - if (ret < 0) { - goto fail; - } + for (i = 0; i < l1_size; i++) { + if (!l1_table[i]) { + continue; + } - /* L2 tables are cluster aligned */ - if (offset_into_cluster(s, l2_offset)) { - fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not " - "cluster aligned; L1 entry corrupted\n", l2_offset); - res->corruptions++; - } + l2_offset = l1_table[i] & L1E_OFFSET_MASK; - /* Process and check L2 entries */ - ret = check_refcounts_l2(bs, res, refcount_table, - refcount_table_size, l2_offset, flags, - fix, active); - if (ret < 0) { - goto fail; - } + /* Mark L2 table as used */ + ret = qcow2_inc_refcounts_imrt(bs, res, + refcount_table, refcount_table_size, + l2_offset, s->cluster_size); + if (ret < 0) { + return ret; + } + + /* L2 tables are cluster aligned */ + if (offset_into_cluster(s, l2_offset)) { + fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not " + "cluster aligned; L1 entry corrupted\n", l2_offset); + res->corruptions++; + } + + /* Process and check L2 entries */ + ret = check_refcounts_l2(bs, res, refcount_table, + refcount_table_size, l2_offset, flags, + fix, active); + if (ret < 0) { + return ret; } } - g_free(l1_table); - return 0; -fail: - g_free(l1_table); - return ret; + return 0; } /*
- use g_autofree for l1_table - better name for size in bytes variable - reduce code blocks nesting - whitespaces, braces, newlines Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-refcount.c | 97 +++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 48 deletions(-)