| Message ID | 1350305057-6287-3-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 15.10.2012 14:44, schrieb Stefan Hajnoczi: > A malicious or corruption image can contain an infinite loop of backing > files. The qemu-img info --backing-chain command must not hang when > such files are encountered. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> This seems to do what is intended, but I think rather than fixing the 'qemu-img info' special case I'd have fixed bdrv_open() to detect the situation. Do we need to properly communicate what the broken image is with JSON output? This patch will only produce broken JSON in the failure case. Kevin
On Tue, Oct 16, 2012 at 12:11:44PM +0200, Kevin Wolf wrote: > Am 15.10.2012 14:44, schrieb Stefan Hajnoczi: > > A malicious or corruption image can contain an infinite loop of backing > > files. The qemu-img info --backing-chain command must not hang when > > such files are encountered. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > This seems to do what is intended, but I think rather than fixing the > 'qemu-img info' special case I'd have fixed bdrv_open() to detect the > situation. When bdrv_open() on the chain fails we'll still need to iterate each image (with infinite loop detection) in qemu-img info. This allows qemu-img info to print out details of the chain and where the chain goes wrong. > Do we need to properly communicate what the broken image is with JSON > output? This patch will only produce broken JSON in the failure case. Ah, you're right. We must close the JSON array and add a test case for this :(. Will send v3. Stefan
Am 16.10.2012 16:42, schrieb Stefan Hajnoczi: > On Tue, Oct 16, 2012 at 12:11:44PM +0200, Kevin Wolf wrote: >> Am 15.10.2012 14:44, schrieb Stefan Hajnoczi: >>> A malicious or corruption image can contain an infinite loop of backing >>> files. The qemu-img info --backing-chain command must not hang when >>> such files are encountered. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> >> This seems to do what is intended, but I think rather than fixing the >> 'qemu-img info' special case I'd have fixed bdrv_open() to detect the >> situation. > > When bdrv_open() on the chain fails we'll still need to iterate each > image (with infinite loop detection) in qemu-img info. This allows > qemu-img info to print out details of the chain and where the chain goes > wrong. Depends on the expected output. If you don't print the start of chain but only an error message with the file name of the image that creates the loop, you could generate the error in bdrv_open(). But yes, we can have both. It's just that having nothing in bdrv_open() isn't really satisfying. Kevin
diff --git a/qemu-img.c b/qemu-img.c index c717f3e..60a5cc8 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1259,6 +1259,11 @@ typedef enum OutputFormat { OFORMAT_HUMAN, } OutputFormat; +static gboolean str_equal_func(gconstpointer a, gconstpointer b) +{ + return strcmp(a, b) == 0; +} + static int img_info(int argc, char **argv) { int c; @@ -1268,6 +1273,7 @@ static int img_info(int argc, char **argv) char *filename, *fmt; BlockDriverState *bs; ImageInfo *info; + GHashTable *filenames; fmt = NULL; output = NULL; @@ -1306,6 +1312,9 @@ static int img_info(int argc, char **argv) } filename = g_strdup(argv[optind++]); + filenames = g_hash_table_new_full(g_str_hash, str_equal_func, + g_free, NULL); + if (output && !strcmp(output, "json")) { output_format = OFORMAT_JSON; } else if (output && !strcmp(output, "human")) { @@ -1320,6 +1329,12 @@ static int img_info(int argc, char **argv) } do { + if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) { + error_report("Aborting due to backing file chain infinite loop."); + goto err; + } + g_hash_table_insert(filenames, g_strdup(filename), NULL); + bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, false); if (!bs) { @@ -1376,11 +1391,13 @@ static int img_info(int argc, char **argv) if (chain && output_format == OFORMAT_JSON) { printf("]\n"); } + g_hash_table_destroy(filenames); return 0; err: g_free(filename); g_free(fmt); + g_hash_table_destroy(filenames); return 1; }
A malicious or corruption image can contain an infinite loop of backing files. The qemu-img info --backing-chain command must not hang when such files are encountered. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- qemu-img.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)