Date: Tue, 1 Oct 2019 16:23:05 +0000 From: Brooks Davis <brooks@freebsd.org> To: Konstantin Belousov <kib@freebsd.org> Cc: Warner Losh <imp@bsdimp.com>, Mateusz Guzik <mjguzik@gmail.com>, Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r352795 - head/lib/libc/sys Message-ID: <20191001162305.GM93439@spindle.one-eyed-alien.net> In-Reply-To: <20190928072548.GN44691@kib.kiev.ua> References: <201909271611.x8RGBl0H036116@repo.freebsd.org> <CAGudoHF454hyHeS_HayGRJYRpSfo9B9wwU4hVBxjcmAk0HEeJg@mail.gmail.com> <20190927184623.GM44691@kib.kiev.ua> <CAGudoHFhMRFm_qvXHZ4MsHGtv74w0_CLdOG3QojeC=ZxDTYZvQ@mail.gmail.com> <CANCZdfonqN9oRnqbjktW1Kc-oKMbcus-cL3x2Nh=dZDDWYdMDw@mail.gmail.com> <20190928072548.GN44691@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--wTWi5aaYRw9ix9vO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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@gmail.com> wrote:
> >=20
> > > On 9/27/19, Konstantin Belousov <kostikbel@gmail.com> wrote:
> > > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > > >> On 9/27/19, Warner Losh <imp@freebsd.org> wrote:
> > > >> > Document varadic args as int, since you can't have short varad=
ic
> > > 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 hand=
le
> > > > 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.
> > >
> >=20
> > 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.
> >=20
> >=20
> > > 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.
> > >
> >=20
> > That's not going to happen. And there's no need. It would cause more
> > heartache than it's worth.
> >=20
> >=20
> > > I have no interest in handling any of this, but the change committed
> > > is definitely wrong.
> > >
> >=20
> > I tend to agree, but the manual was/is incomplete. The arg *IS* promote=
d 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).
> >=20
> > However, type purity aside, that's not how things are implemented. Open=
is
> > expecting an int (as is openat):
> >=20
> > int
> > open(const char *path, int flags, ...)
> > {
> > va_list ap;
> > int mode;
> >=20
> > if ((flags & O_CREAT) !=3D 0) {
> > va_start(ap, flags);
> > mode =3D va_arg(ap, int);
> > va_end(ap);
> > } else {
> > mode =3D 0;
> > }
> > return (((int (*)(int, const char *, int, ...))
> > __libc_interposing[INTERPOS_openat])(fd, path, flags, mode)=
);
> > }
> >=20
> > 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 (he=
nce
> > my can of worms comment). And then we're passing it again through a var=
adic
> > function pointer...
> >=20
> > 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.
>=20
> > Or do we fix our implementation to
> > match the docs? For all programs that don't pass in a 'long' or a point=
er,
> > 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.
>=20
> >=20
> > To be honest, though, quibbling over how it should be implemented aside=
, I
> > think we should actually do the following:
> >=20
> > 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 menti=
on
> 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);
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
--wTWi5aaYRw9ix9vO
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iQEcBAEBAgAGBQJdk31pAAoJEKzQXbSebgfArsAH/A3U2SdkTPQ8c3U5S96GTGZr
gu5WTaBA30L96cdc42c5HzAcUBNPb2iqFa4uo/7VVfQTr81pYg6igJKvor4Db5yw
BcRgruFreBl9sL559J2W0UPsu3G5UvuX7aZajMysx295LkEIf1nMg1n2wYdlL2Kf
83ZiGG3JlsYKXoLKhxiVHy0NvcUBrGC0x8s4L3diTrr0RBgKqoC3OqITXypuG9BD
/E2wKByVkhDoRB71Cb0RKuib8SGH/464fM7SPX/0c1Ydc7PBp7UByI2w+yATREH8
AznNGbdpnefxtZJbEdmxdvayLWH9Lv/Shvp6yXznf+SucehNKqM891ty7/49uY0=
=PX8t
-----END PGP SIGNATURE-----
--wTWi5aaYRw9ix9vO--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191001162305.GM93439>