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 <[email protected]> > > > > > > > > ---------------------------------- 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
signature.asc
Description: PGP signature
