svn commit: r352795 - head/lib/libc/sys

[Konstantin Belousov]

On Tue, Oct 01, 2019 at 04:23:05PM +0000, Brooks Davis wrote:
> On Sat, Sep 28, 2019 at 10:25:48AM +0300, Konstantin Belousov wrote:
> > On Fri, Sep 27, 2019 at 03:19:59PM -0600, Warner Losh wrote:
> > > On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik <mjguzik at gmail.com> wrote:
> > > 
> > > > On 9/27/19, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > > > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > > > >> On 9/27/19, Warner Losh <imp at freebsd.org> wrote:
> > > > >> >   Document varadic args as int, since you can't have short varadic
> > > > args
> > > > >> > (they are
> > > > >> >   promoted to ints).
> > > > >> >
> > > > >> >   - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
> > > > >> >   - `openat` takes variadic args
> > > > >> >   - variadic args cannot be 16-bit, and indeed the code uses int
> > > > >> >   - the manpage currently kinda implies the argument is 16-bit by
> > > > >> > saying
> > > > >> > `mode_t`
> > > > >> >
> > > > >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
> > > > >> to be changed?
> > > > >
> > > > > Yes, users must pass mode_t, and the man page is written for users.
> > > > > Implementation needs to be aware of the implicit promotion and handle
> > > > > it accordingly.
> > > > >
> > > > > In theory, mode_t might be wider than int.
> > > > >
> > > >
> > > > So I think the change should be reverted. Whatever workaround is being
> > > > in place in rust should remain for the current codebase.
> > > >
> > > 
> > > Rust needs to understand that it's not C. It's mistake was assuming it was
> > > just like C and this is a case where the languages differ because C is so
> > > quirky.
> > > 
> > > 
> > > > If anyone is to fixed the problem they should bump mode_t to uint32_t,
> > > > to match Linux. This is ABI breakage, I don't know how that's handled.
> > > >
> > > 
> > > That's not going to happen. And there's no need. It would cause more
> > > heartache than it's worth.
> > > 
> > > 
> > > > I have no interest in handling any of this, but the change committed
> > > > is definitely wrong.
> > > >
> > > 
> > > I tend to agree, but the manual was/is incomplete. The arg *IS* promoted to
> > > an int, per normal C rules, so that part is right and there's no
> > > type-checking against truncation or the wrong type being used as would be
> > > the case if it weren't varadic (so don't pass a long here).
> > > 
> > > However, type purity aside, that's not how things are implemented. Open is
> > > expecting an int (as is openat):
> > > 
> > > int
> > > open(const char *path, int flags, ...)
> > > {
> > >         va_list ap;
> > >         int mode;
> > > 
> > >         if ((flags & O_CREAT) != 0) {
> > >                 va_start(ap, flags);
> > >                 mode = va_arg(ap, int);
> > >                 va_end(ap);
> > >         } else {
> > >                 mode = 0;
> > >         }
> > >         return (((int (*)(int, const char *, int, ...))
> > >             __libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
> > > }
> > > 
> > > so the change, from that perspective, actually documents the interface (so
> > > isn't definitely wrong, and my guarded 'tend to agree'). So if you did
> > > change the type of mode_t, the above code might be wrong afterwards (hence
> > > my can of worms comment). And then we're passing it again through a varadic
> > > function pointer...
> > > 
> > > So while POSIX says one thing, we implement something else. Should we
> > > document POSIX or what we implement?
> > I do not see how did you come to this conclusion.
> > 
> > > Or do we fix our implementation to
> > > match the docs? For all programs that don't pass in a 'long' or a pointer,
> > > the difference is zero, however.
> > ... on all supported architectures.  On 32bit it actually does not matter even
> > for long or pointers.  But this is irrelevant, because correct programs
> > must only pass mode_t as the third arg, and then our libc does the right
> > thing on all currently supported platforms.  More, I do not expect that
> > this fragment would need any revisions for future architectures.
> > 
> > > 
> > > To be honest, though, quibbling over how it should be implemented aside, I
> > > think we should actually do the following:
> > > 
> > > diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
> > > index a771461e2e49..aa912b797f74 100644
> > > --- a/lib/libc/sys/open.2
> > > +++ b/lib/libc/sys/open.2
> > > @@ -61,7 +61,7 @@ In this case
> > >  and
> > >  .Fn openat
> > >  require an additional argument
> > > -.Fa "int mode" ,
> > > +.Fa "mode_t mode" ,
> > >  and the file is created with mode
> > >  .Fa mode
> > >  as described in
> > > @@ -615,3 +615,8 @@ permits searches.
> > >  The present implementation of the
> > >  .Fa openat
> > >  checks the current permissions of directory instead.
> > > +.Pp
> > > +The
> > > +.Fa mode
> > > +argument is varadic and may result in different calling conventions
> > > +than might otherwise be expected.
> > I do not see how this could be useful for a user trying to call open(2).
> > I think it would be much easier to understand and use if you simply mention
> > that 'on all supported arches, mode_t is promoted to int by C rules for
> > implicit conversions of arguments for variadic functions'.  And perhaps
> > put it somewhere else, not in the BUGS section.
> 
> I think this would be a good solution.
> 
> 
> Note that the actual ABI constraint on variadic syscalls in FreeBSD is
> that they have exactly the same calling convention as if the argument is
> explicitly declared because we have on infrastructure to handle them any
> other way.  Specifically, calling a function of type:
> 
> int open(const char *path, int flags, ...);
> 
> with a mode_t as a variadic argument must be identical to calling a
> function of type:
> 
> int open(const char *path, int flags, mode_t mode);
It is not quite true, surprisingly, on amd64. At least the ABI requires
the caller to put the number of XMM registers used for param passing
into %eax for variadic functions. I think both gcc and clang can live
without this hint in modern times.

> 
> This isn't true with CHERI and as a result I've moved the variadic
> argument handling (except for syscall() and __syscall()) into libc.
> 
> -- Brooks