On 05/01/18 11:56, Kamil Dudka wrote: > On Friday, January 5, 2018 2:00:52 AM CET Paul Eggert wrote: >> On 01/04/2018 03:01 AM, Kamil Dudka wrote: >>> On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote: >>>> Kamil Dudka wrote: >>>>> - if (rename (src_name, dst_name) == 0) >>>>> + int flags = 0; >>>>> + if (x->interactive == I_ALWAYS_NO) >>>>> + /* do not replace DST_NAME if it was created since our last >>>>> check >>>>> */ + flags = RENAME_NOREPLACE; >>>> >>>> By then it's too late, as multiple decisions have been made on the basis >>>> of >>>> stat/lstat calls that are still subject to races. >>> >>> Do you mean in the case of mv -n? Which decisions exactly? >> >> Mostly mv -n, but I suspect problems also for mv without -n. > > My patch changes nothing but the 'mv -n' behavior. It could hardly break > (or even change behavior of) anything else. Your patch reworks the logic > of copy_internal(), which itself is very fragile, as you learned from the > first version of your patch. > >> It's all >> the decisions that depend on the result of lstat of dst_name, before >> abandon_move decides whether to skip the rename. > > I am only fixing the case where the destination file is created after the > lstat() call. In that case, the only result is ENOENT, which is harmless. > >> With the patch you >> proposed, mv -n could call lstat and get a failure (with errno == >> ENOENT), then (after another process creates the file) call renameat2 >> with RENAME_NOREPLACE and after this fails (with errno == EEXIST) > > Exactly. That is the only case that my patch intended to fix. > >> report an error. > > Nope. It will silently succeed with my patch. Did you try it? > >> mv -n should silently succeed in that case. > > I agree. > >>> Sounds like a corner case. Please consider writing a separate patch >>> for that. >> >> OK, that's pretty straightforward so I installed it. Please see the >> first attached patch. >> >>> I had difficulties trying to evaluate the patch. It does not compile >> >> That's what I get for sending an untested patch. Sorry. I fixed the bugs >> you mentioned and tested the result. Please see the second attached >> patch, which I have not installed. > > Thanks for the fixup! > >> There is an interesting behavior change with this second patch. >> Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the >> same file". With the patch, 'mv -n a a' silently succeeds. The coreutils >> documentation allows both behaviors. I doubt whether anyone cares, and >> doing it the new way avoids some syscalls so I left it that way. > > If you decide to apply your patch anyway, please document this change > at least in the commit message. Still my concern is that your patch also > changes the behavior of 'mv' without '-n', which is neither tested, nor > documented.
Thanks to both of you. The approaches can be summarized as: Orig: --------------------------------- stat() => ENOENT <file created externally> rename() may clobber file Kamil's: --------------------------------- stat() => ENOENT <file created externally> renameat() doesn't clobber file if -n exit early if -n && errno==EEXIST Paul's: --------------------------------- renameat2() => EEXIST -n => exit early if (renameat2_failed) unless EEXIST && -n stat() if (renameat2_failed) rename() I think both patches ensure rename() doesn't clobber when -n is specified. Paul's is more encompassing for the non -n case. For example if a directory was _created_ externally after the stat() in Kamil's logic above, it could be erroneously clobbered. Paul's also avoids a stat() in the common case where the initial renameat2() succeeds. Both versions are still susceptible to erroneous clobbering if the destination file was externally _replaced_ by a directory for example, after the stat(). Though that is less likely. Paul's fix looks good to apply here, since it's more encompassing. cheers, Pádraig
