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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

On Thu, Jan 25, 2018 at 11:02:08AM +0000, Daniel P. Berrangé wrote:
> 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 <riverchiang@gmail.com>
> > > > 
> > > > ---------------------------------- 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.

I don't think a partial solution to protecting the user is worthwhile.
It gives a false impression.

If we do decide to add the strcmp(3) check, then please add it to all
sub-commmands where it's needed.  qemu-img dd comes to mind and there
are probably others.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]