On Fri 09 Feb 2018 02:04:06 PM CET, Max Reitz wrote: > I don't like completely refusing to open a qcow2 image if one of the > snapshots is invalid, without giving the user any way of fixing it. > > With this patch, the final two images created in 080 cannot be opened > at all (not even with qemu-img info). Without it, you can, you just > can't load the snapshots. (Well, in one case. In the other, you can > even do that, but that's a bug, as you correctly claim.) > > More importantly, though, without this patch, you can delete the > snapshots. qemu-img will complain a bit, and you'll have leaks > afterwards, but that's nothing qemu-img check can't fix. With this > patch, you can't, because the image cannot be opened at all so it's > basically gone for good (unless you get a hex editor).
What you're saying is correct. The alternative however is to add checks everywhere the snapshot table is used. There's for example qcow2_check_metadata_overlap() that will happily allocate a buffer of (l1_size * 8) bytes, that can be up to 32 GB. qcow2_expand_zero_clusters() has the same problem but there g_realloc() is used, aborting the process on failure (we should actually use g_try_realloc() instead, I'll leave that for a different patch). If we want to allow opening images with corrupted snapshots perhaps we could still do the checks in qcow2_read_snapshots() but instead of returning an error add a 'corrupted' flag to QCowSnapshot. That solution is not without problems because we'd still need to check for that flag everytime the snapshot is used, and that's calling for errors in the future. Berto
