Re: [Qemu-devel] Prevent overriding the input file with the output file when using qemu-img

[Daniel P . Berrangé]

On Thu, Jan 25, 2018 at 10:52:57AM +0000, Stefan Hajnoczi wrote:
> On Tue, Jan 23, 2018 at 08:48:15AM -0600, Eric Blake wrote:
> > On 01/22/2018 10:40 PM, River Chiang wrote:
> > >     Signed-off-by: River Chiang <address@hidden>
> > > 
> > > ---------------------------------- qemu-img.c
> > > ----------------------------------
> > > index 68b375f998..5ce594ea00 100644
> > > @@ -2098,6 +2098,9 @@ static int img_convert(int argc, char **argv)
> > >      if (s.src_num < 1) {
> > >          error_report("Must specify image file name");
> > >          goto fail_getopt;
> > > +    } else if (!strcmp(argv[optind], out_filename)) {
> > > +        error_report("Override the input file with the output file");
> > > +        goto fail_getopt;
> > 
> > Comparing names is too prone to false negatives.  'foo' and './foo' are
> > the same file, but your test won't catch it.  Better might be checking
> > if stat() reports the same dev/inode pair for the two files.
> > 
> > By the way, your patch is not in proper 'git send-email' format, which
> > makes it hard to test whether it even applies.  More patch submission
> > hints at http://wiki.qemu.org/Contribute/SubmitAPatch
> 
> stat(2) cannot be used since the "filenames" may not be a local file,
> (nbd://, iscsi://, etc).
> 
> strcmp(3) is also not a full solution, for the reasons you mentioned.

It isn't a full solution, but I does it really need to be ? This check
is only needed to protect against user accidents. It doesn't trigger
false reports so won't block valid usage, it merely fails to report
the problem in some edge cases.  IOW, I think strcmp is good enough
in absence of any other simple solution - better than nothing IMHO.

> 
> Even file locking probably isn't a full solution.  What happens when
> input and output files are nbd:// URIs?
> 
> Attempting to prevent the user from harming themselves is very hard to
> do.  It's better not to second-guess the user than to have some magic
> that doesn't always work (the user cannot rely on it anyway).



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|