All of lore.kernel.org
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: [PULL 56/69] qcow2: Fix overly long snapshot tables
Date: Mon, 28 Oct 2019 13:14:48 +0100	[thread overview]
Message-ID: <20191028121501.15279-57-mreitz@redhat.com> (raw)
In-Reply-To: <20191028121501.15279-1-mreitz@redhat.com>

We currently refuse to open qcow2 images with overly long snapshot
tables.  This patch makes qemu-img check -r all drop all offending
entries past what we deem acceptable.

The user cannot choose which snapshots are removed.  This is fine
because we have chosen the maximum snapshot table size to be so large
(64 MB) that it cannot be reasonably reached.  If the snapshot table
exceeds this size, the image has probably been corrupted in some way; in
this case, it is most important to just make the image usable such that
the user can copy off at least the active layer.
(Also note that the snapshots will be removed only with "-r all", so a
plain "check" or "check -r leaks" will not delete any data.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20191011152814.14791-13-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-snapshot.c | 88 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 582eb3386a..366d9f574c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -29,15 +29,24 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 
+static void qcow2_free_single_snapshot(BlockDriverState *bs, int i)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    assert(i >= 0 && i < s->nb_snapshots);
+    g_free(s->snapshots[i].name);
+    g_free(s->snapshots[i].id_str);
+    g_free(s->snapshots[i].unknown_extra_data);
+    memset(&s->snapshots[i], 0, sizeof(s->snapshots[i]));
+}
+
 void qcow2_free_snapshots(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
     int i;
 
     for(i = 0; i < s->nb_snapshots; i++) {
-        g_free(s->snapshots[i].name);
-        g_free(s->snapshots[i].id_str);
-        g_free(s->snapshots[i].unknown_extra_data);
+        qcow2_free_single_snapshot(bs, i);
     }
     g_free(s->snapshots);
     s->snapshots = NULL;
@@ -48,6 +57,14 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  * If @repair is true, try to repair a broken snapshot table instead
  * of just returning an error:
  *
+ * - If the snapshot table was too long, set *nb_clusters_reduced to
+ *   the number of snapshots removed off the end.
+ *   The caller will update the on-disk nb_snapshots accordingly;
+ *   this leaks clusters, but is safe.
+ *   (The on-disk information must be updated before
+ *   qcow2_check_refcounts(), because that function relies on
+ *   s->nb_snapshots to reflect the on-disk value.)
+ *
  * - If there were snapshots with too much extra metadata, increment
  *   *extra_data_dropped for each.
  *   This requires the caller to eventually rewrite the whole snapshot
@@ -59,6 +76,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  *   extra data.)
  */
 static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+                                   int *nb_clusters_reduced,
                                    int *extra_data_dropped,
                                    Error **errp)
 {
@@ -67,7 +85,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
     QCowSnapshotExtraData extra;
     QCowSnapshot *sn;
     int i, id_str_size, name_size;
-    int64_t offset;
+    int64_t offset, pre_sn_offset;
     uint64_t table_length = 0;
     int ret;
 
@@ -83,6 +101,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
     for(i = 0; i < s->nb_snapshots; i++) {
         bool truncate_unknown_extra_data = false;
 
+        pre_sn_offset = offset;
         table_length = ROUND_UP(table_length, 8);
 
         /* Read statically sized part of the snapshot header */
@@ -197,9 +216,31 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
         if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
             offset - s->snapshots_offset > INT_MAX)
         {
-            ret = -EFBIG;
-            error_setg(errp, "Snapshot table is too big");
-            goto fail;
+            if (!repair) {
+                ret = -EFBIG;
+                error_setg(errp, "Snapshot table is too big");
+                error_append_hint(errp, "You can force-remove all %u "
+                                  "overhanging snapshots with qemu-img check "
+                                  "-r all\n", s->nb_snapshots - i);
+                goto fail;
+            }
+
+            fprintf(stderr, "Discarding %u overhanging snapshots (snapshot "
+                    "table is too big)\n", s->nb_snapshots - i);
+
+            *nb_clusters_reduced += (s->nb_snapshots - i);
+
+            /* Discard current snapshot also */
+            qcow2_free_single_snapshot(bs, i);
+
+            /*
+             * This leaks all the rest of the snapshot table and the
+             * snapshots' clusters, but we run in check -r all mode,
+             * so qcow2_check_refcounts() will take care of it.
+             */
+            s->nb_snapshots = i;
+            offset = pre_sn_offset;
+            break;
         }
     }
 
@@ -214,7 +255,7 @@ fail:
 
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 {
-    return qcow2_do_read_snapshots(bs, false, NULL, errp);
+    return qcow2_do_read_snapshots(bs, false, NULL, NULL, errp);
 }
 
 /* add at the end of the file a new list of snapshots */
@@ -382,6 +423,7 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
 {
     BDRVQcow2State *s = bs->opaque;
     Error *local_err = NULL;
+    int nb_clusters_reduced = 0;
     int extra_data_dropped = 0;
     int ret;
     struct {
@@ -419,7 +461,8 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
 
     qemu_co_mutex_unlock(&s->lock);
     ret = qcow2_do_read_snapshots(bs, fix & BDRV_FIX_ERRORS,
-                                  &extra_data_dropped, &local_err);
+                                  &nb_clusters_reduced, &extra_data_dropped,
+                                  &local_err);
     qemu_co_mutex_lock(&s->lock);
     if (ret < 0) {
         result->check_errors++;
@@ -432,7 +475,32 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
 
         return ret;
     }
-    result->corruptions += extra_data_dropped;
+    result->corruptions += nb_clusters_reduced + extra_data_dropped;
+
+    if (nb_clusters_reduced) {
+        /*
+         * Update image header now, because:
+         * (1) qcow2_check_refcounts() relies on s->nb_snapshots to be
+         *     the same as what the image header says,
+         * (2) this leaks clusters, but qcow2_check_refcounts() will
+         *     fix that.
+         */
+        assert(fix & BDRV_FIX_ERRORS);
+
+        snapshot_table_pointer.nb_snapshots = cpu_to_be32(s->nb_snapshots);
+        ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
+                               &snapshot_table_pointer.nb_snapshots,
+                               sizeof(snapshot_table_pointer.nb_snapshots));
+        if (ret < 0) {
+            result->check_errors++;
+            fprintf(stderr, "ERROR failed to update the snapshot count in the "
+                    "image header: %s\n", strerror(-ret));
+            return ret;
+        }
+
+        result->corruptions_fixed += nb_clusters_reduced;
+        result->corruptions -= nb_clusters_reduced;
+    }
 
     return 0;
 }
-- 
2.21.0



  parent reply	other threads:[~2019-10-28 13:23 UTC|newest]

Thread overview: 71+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-28 12:13 [PULL 00/69] Block patches for softfreeze Max Reitz
2019-10-28 12:13 ` [PULL 01/69] iotests: Prefer null-co over null-aio Max Reitz
2019-10-28 12:13 ` [PULL 02/69] iotests: Allow skipping test cases Max Reitz
2019-10-28 12:13 ` [PULL 03/69] iotests: Use case_skip() in skip_if_unsupported() Max Reitz
2019-10-28 12:13 ` [PULL 04/69] iotests: Let skip_if_unsupported accept a function Max Reitz
2019-10-28 12:13 ` [PULL 05/69] iotests: Test driver whitelisting in 093 Max Reitz
2019-10-28 12:13 ` [PULL 06/69] iotests: Test driver whitelisting in 136 Max Reitz
2019-10-28 12:13 ` [PULL 07/69] iotests: Cache supported_formats() Max Reitz
2019-10-28 12:14 ` [PULL 08/69] hbitmap: handle set/reset with zero length Max Reitz
2019-10-28 12:14 ` [PULL 09/69] block/mirror: simplify do_sync_target_write Max Reitz
2019-10-28 12:14 ` [PULL 10/69] block/block-backend: add blk_co_pwritev_part Max Reitz
2019-10-28 12:14 ` [PULL 11/69] block/mirror: support unaligned write in active mirror Max Reitz
2019-10-28 12:14 ` [PULL 12/69] Revert "mirror: Only mirror granularity-aligned chunks" Max Reitz
2019-10-28 12:14 ` [PULL 13/69] iotests: Introduce $SOCK_DIR Max Reitz
2019-10-28 12:14 ` [PULL 14/69] iotests.py: Store socket files in $SOCK_DIR Max Reitz
2019-10-28 12:14 ` [PULL 15/69] iotests.py: Add @base_dir to FilePaths etc Max Reitz
2019-10-28 12:14 ` [PULL 16/69] iotests: Filter $SOCK_DIR Max Reitz
2019-10-28 12:14 ` [PULL 17/69] iotests: Let common.nbd create socket in $SOCK_DIR Max Reitz
2019-10-28 12:14 ` [PULL 18/69] iotests/083: Create " Max Reitz
2019-10-28 12:14 ` [PULL 19/69] iotests/140: " Max Reitz
2019-10-28 12:14 ` [PULL 20/69] iotests/143: " Max Reitz
2019-10-28 12:14 ` [PULL 21/69] iotests/147: " Max Reitz
2019-10-28 12:14 ` [PULL 22/69] iotests/181: " Max Reitz
2019-10-28 12:14 ` [PULL 23/69] iotests/182: " Max Reitz
2019-10-28 12:14 ` [PULL 24/69] iotests/183: " Max Reitz
2019-10-28 12:14 ` [PULL 25/69] iotests/192: " Max Reitz
2019-10-28 12:14 ` [PULL 26/69] iotests/194: Create sockets " Max Reitz
2019-10-28 12:14 ` [PULL 27/69] iotests/201: Create socket " Max Reitz
2019-10-28 12:14 ` [PULL 28/69] iotests/205: " Max Reitz
2019-10-28 12:14 ` [PULL 29/69] iotests/208: " Max Reitz
2019-10-28 12:14 ` [PULL 30/69] iotests/209: " Max Reitz
2019-10-28 12:14 ` [PULL 31/69] iotests/222: " Max Reitz
2019-10-28 12:14 ` [PULL 32/69] iotests/223: " Max Reitz
2019-10-28 12:14 ` [PULL 33/69] iotests/240: " Max Reitz
2019-10-28 12:14 ` [PULL 34/69] iotests/267: " Max Reitz
2019-10-28 12:14 ` [PULL 35/69] iotests: Drop TEST_DIR filter from _filter_nbd Max Reitz
2019-10-28 12:14 ` [PULL 36/69] block/block-copy: allocate buffer in block_copy_with_bounce_buffer Max Reitz
2019-10-28 12:14 ` [PULL 37/69] block/block-copy: limit copy_range_size to 16 MiB Max Reitz
2019-10-28 12:14 ` [PULL 38/69] block/block-copy: refactor copying Max Reitz
2019-10-28 12:14 ` [PULL 39/69] util: introduce SharedResource Max Reitz
2019-10-28 12:14 ` [PULL 40/69] block/block-copy: add memory limit Max Reitz
2019-10-28 12:14 ` [PULL 41/69] block/block-copy: increase buffered copy request Max Reitz
2019-10-28 12:14 ` [PULL 42/69] block/nvme: add support for write zeros Max Reitz
2019-10-28 12:14 ` [PULL 43/69] block/nvme: add support for discard Max Reitz
2019-10-28 12:14 ` [PULL 44/69] mirror: Do not dereference invalid pointers Max Reitz
2019-10-28 12:14 ` [PULL 45/69] include: Move endof() up from hw/virtio/virtio.h Max Reitz
2019-10-28 12:14 ` [PULL 46/69] qcow2: Use endof() Max Reitz
2019-10-28 12:14 ` [PULL 47/69] qcow2: Add Error ** to qcow2_read_snapshots() Max Reitz
2019-10-28 12:14 ` [PULL 48/69] qcow2: Keep unknown extra snapshot data Max Reitz
2019-10-28 12:14 ` [PULL 49/69] qcow2: Make qcow2_write_snapshots() public Max Reitz
2019-10-28 12:14 ` [PULL 50/69] qcow2: Put qcow2_upgrade() into its own function Max Reitz
2019-10-28 12:14 ` [PULL 51/69] qcow2: Write v3-compliant snapshot list on upgrade Max Reitz
2019-10-28 12:14 ` [PULL 52/69] qcow2: Separate qcow2_check_read_snapshot_table() Max Reitz
2019-10-28 12:14 ` [PULL 53/69] qcow2: Add qcow2_check_fix_snapshot_table() Max Reitz
2019-10-28 12:14 ` [PULL 54/69] qcow2: Fix broken snapshot table entries Max Reitz
2019-10-28 12:14 ` [PULL 55/69] qcow2: Keep track of the snapshot table length Max Reitz
2019-10-28 12:14 ` Max Reitz [this message]
2019-10-28 12:14 ` [PULL 57/69] qcow2: Repair snapshot table with too many entries Max Reitz
2019-10-28 12:14 ` [PULL 58/69] qcow2: Fix v3 snapshot table entry compliancy Max Reitz
2019-10-28 12:14 ` [PULL 59/69] iotests: Add peek_file* functions Max Reitz
2019-10-28 12:14 ` [PULL 60/69] iotests: Test qcow2's snapshot table handling Max Reitz
2019-10-28 12:14 ` [PULL 61/69] block: Handle filter truncation like native impl Max Reitz
2019-10-28 12:14 ` [PULL 62/69] block/cor: Drop cor_co_truncate() Max Reitz
2019-10-28 12:14 ` [PULL 63/69] block: Do not truncate file node when formatting Max Reitz
2019-10-28 12:14 ` [PULL 64/69] block: Add @exact parameter to bdrv_co_truncate() Max Reitz
2019-10-28 12:14 ` [PULL 65/69] block: Evaluate @exact in protocol drivers Max Reitz
2019-10-28 12:14 ` [PULL 66/69] block: Let format drivers pass @exact Max Reitz
2019-10-28 12:14 ` [PULL 67/69] block: Pass truncate exact=true where reasonable Max Reitz
2019-10-28 12:15 ` [PULL 68/69] Revert "qemu-img: Check post-truncation size" Max Reitz
2019-10-28 12:15 ` [PULL 69/69] qemu-iotests: restrict 264 to qcow2 only Max Reitz
2019-10-28 21:13 ` [PULL 00/69] Block patches for softfreeze Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191028121501.15279-57-mreitz@redhat.com \
    --to=mreitz@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.