Re: [PATCH] Add renameat2 function [BZ #17662]

[Carlos O'Donell]

On 07/04/2018 03:35 PM, Paul Eggert wrote:
> Carlos O'Donell wrote:
> 
>> If we really need another non-race-free API then gnulib can provide
>> that.
> 
> We do really need it. All existing uses of renameat2-like
> functionality in GNU applications already use the non-race-free API.
> But as I see it, the consensus is to banish this functionality to
> Gnulib. Since it will longer matter to Gnulib whether Glibc
> introduces renameat2, I withdraw my objection to the patch.
> 
> To help forestall likely bugs in code using Glibc renameat2, it would
> be helpful for the Glibc manual to document this issue, at least for
> RENAME_NOREPLACE which is the only reason I know anybody is calling
> renameat2 anyway. That is, if we're stuck with this API, at least we
> should warn users about its shortcomings. Perhaps the Glibc manual
> could point to Gnulib, as a suggestion for users who prefer the
> non-race-free functionality.

This is a good suggestion, and I think Florian should work on something
going into the manual to document the behaviour.

>> We are not "fighting" against GNU applications
> As an application writer it sure looks like a fight to me. Glibc is
> supplying an API that doesn't do what GNU apps need. So I plan to
> change the name of the Gnulib function to avoid namespace collisions,
> and as a result GnU apps via Gnulib will not use Glibc renameat2 at
> all. (Why should they bother? The Gnulib function already renames
> atomically using code that works on more platforms than Glibc does,
> and the Gnulib code is a tiny bit more efficient even when calling
> Glibc renameat2 would work.)

You position Gnulib's implementation as having no drawbacks, but this
is not true. The API has a race, and it is something which along with
other similar racy APIs has caused difficult to solve problems a the
distribution level.

I think it's a bad design choice to wrap renameat2 in a wrapper
that falls back to a racy solution. It can only cause us problems
because of the naming, in that renameat2 is not supposed to be racy.
However, I have no objection to a racy API with another name.

> I understand the desire to support an API that guarantees atomicity,
> and I'm not arguing against that. All I'm saying is that Glibc should
> also support use cases that merely prefer instead of requiring
> atomicity, as these are so common in practice that we still haven't
> run into a GNU app that actually *requires* atomicity.

coreutils *requires* it to solve the race, it says so in their NEWS:
~~~
  'mv -n A B' no longer suffers from a race condition that can
  overwrite a simultaneously-created B.  This bug fix requires
                                                      ^^^^^^^^
  platform support for the renameat2 or renameatx_np syscalls, found
  in recent Linux and macOS kernels.  As a side effect, ‘mv -n A A’
  now silently does nothing if A exists.
~~~
without it the fix regresses.

I don't object to a *distinct* API which implements the non-atomic variants.

I'm happy to review such new API additions, but that's not what is being
proposed in this thread by Florian.

In coreutils-8.30 lib/renameat2.c calls out at least 2 races which affect
correct operation. I would like *coretuils* itself to make the decision
in their sources, with full disclosure, to call the non-racy API first,
see if it works, and then if it doesn't work, attempt a racy API fallback.
This clearly documents the intended behaviour and is still easy for
application developers to use.

I don't think it's a good design choice to put both of those things together
into one API call. The semantics are sufficiently different that mistakes
will get made.

I want glibc to make a clear design decision here that these two things
should not be mixed in a single API, there is no strong reason to do so,
and in the past it has caused hard to find bugs.

-- 
Cheers,
Carlos.