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

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

[River Chiang]

    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;
     }

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

[Eric Blake]

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

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Eric Blake]

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

On 01/23/2018 08:48 AM, 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

Also, is this something that our image locking patches should be able
prevent automatically, without having to special case whether the
command line arguments refer to the same file, by the two different
command line arguments triggering conflicting locks?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Stefan Hajnoczi]

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

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.

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).

Stefan

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

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 <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.

> 
> 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 :|

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

[Stefan Hajnoczi]

[-- 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 --]