Re: use of varargs in open and various functions

[Norbert Lange <nolange79@gmail.com>]

>
> On Thu, Apr 11, 2019 at 04:25:50PM +0200, Norbert Lange wrote:
> > Hello,
> >
> > I had some dealings with software that forwards arguments from vararg functions,
> > the kind with optional but known fixed type.
> > open, fcntl, ioctl are some examples.
> >
> > What happens in musl is that the optional argument is optionally or
> > always retrieved (fcntl).
> > I think this is pretty much pointless, the only reason to use varags would be:
> >
> > 1.   varying argument types (plainly not possibly to replace with
> > direct arguments)
> > 2.   different parameter passing than direct arguments (ABI compatibility)
> > 3.   accessing parameters the caller has not provided.
> >
> > 1. disqualifies functions like printf which I am not planning to touch.
> > 2. would need more tests, but I expect this to be true for all modern
> > architectures,
> It's not true. Not even for x86_64. If you declared open or fcntl
> wrong, the caller would not set rax indicating lack of sse args, and
> when calling into libc.so built with the correct variadic definitions,
> bogus valid in rax could crash or cause runaway wrong code execution.
> It's also not true on powerpc, at least for aggregate types -- see
> commit 2b47a7aff24bbfbe7ba89fc6d542acc9f5493ae2.

declaration would stay the same, on x86_64 the caller will set rax.
The implementation of open/fcntl would have a different declaration.
if ppc does something similar, would this matter if the caller calls
a vararg function with a implementation not using varargs?

For now consider that callers always use the vararg declaration.

> Perhaps more importantly, it's completely untrue for the abstract
> machine and advanced linking and symbolic execution models (LTO, tools
> like Frama-C) where mismatched function type would just be a hard
> linking error.

That's more of an argument, the patch I added is just one example,
you could use some barriers like implementing an
another_open, and then substitute this as symbol for "open" (alias
attribute or linker magic).

Or just disable LTO for these functions.

>
> > 3. thats a thing for open where the 3rd argument is only accessed if
> > certain flags are set,
> >     but most other functions in musl seem to always access "optional" arguments.
> There are only a few which do this, and it's a bug, but possibly an
> unfixable bug. For instance syscall() can't really know how many to
> access without a huge table, and even then it's sometimes "valid" to
> call a syscall with fewer args depending on flags or context. So we
> really probably need a stupid asm wrapper-barrier here to "launder"
> the mismatched state (converting missing args abstractly into args
> with indeterminate value).

at which point LTO and analysis tools will run against an barrier aswell
(not that much lost at syscall wrappers IMHO).

> >     reading theses non-existing arguments could trigger tools like
> > valgrind, but other
> >     than that it should not be an issue aslong a couple bytes of stack
> > is available.
> >
> > I tested code calling the functions, I believe calls are identical for
> > varags and normal functions,
> > for all architectures that are supported by musl. [1]
> > (sidenote: clang generates absolutely awful code for those varag functions).
> >
> > So, is there any reason not to make this the default for musl (adding
> > a fallback option via a define)? I am running a few tools (bash, nano)
> > compiled with the applied patch and so far I have no adverse effects,
> > it should not affect ABI for machines satisfying 2. and it gets rid of
> > some wacky code that in the end just passes in another register (like
> > open).
> >
> > Further I would like to add that Torvalds shares a similar view [2].
> A lot of effort was spent making this right; it's not going to be
> reversed. Torvalds is wrong about lots of things; a large part of
> what's nontrivial in musl has been working around things he's wrong
> about. :-)

I am not implying he is always right.

> What benefit are you trying to get from this? Just some size
> reduction?

I ran into this problem with Xenomai Userspace, which "wraps" a
subset of libc and posix functions to allow using a hard realtime
subsystem with "normal" posix code (and transparent fallback
to the regular system).
Except the wrapper for open missed the special case of O_TMPFILE,
hence me fixing that and looking at "prior art".

open could have been simple function with 3 parameters, just doing a syscall,
instead the delicate vararg hacks spreading everywhere.

So the benefit I would get from that is getting rid of some weirdness
very few people know about. And curiosity why it has to be this way
in case that's not possible.

> I assumed GCC would be non-idiotic and generate exactly the
> same code for the correct version with conditional va_arg and the
> incorrect non-variadic version, but apparently it's doing gratuitous
> store/load to/from stack, at least on the versions I've tested. If
> this bothers you, please pester the GCC folks to fix it. (Note: GCC
> can and does optimize this out when inlining variadic functions, so
> the high-level machinery is there.)

Yeah, thats a sidenote. I added a comment on the bugtracker for clang/llvm
which seems to have a crude bug there.
Its still better to keep things as simple as reasonable.

> > [1] https://godbolt.org/z/asBT92
> > [2] https://lkml.org/lkml/2014/10/30/812
> >
> > PS. why is this a thing in open:
> >   int fd = __sys_open_cp(filename, flags, mode);
> >   if (fd>=0 && (flags & O_CLOEXEC))
> >   __syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC);
> >
> > Is this to support old kernels?
> > I thought O_CLOEXEC is used to fight races during a fork,
> > so if its not supported by the kernel it wont do alot.
> There were a few kernel versions, probably no longer relevant because
> it was quickly fixed IIRC and they weren't LTS versions, where
> O_CLOEXEC was ignored completely. The result was that even in
> single-threaded programs where there was no race, you had a huge fd
> leak. The workaround here was to mitigate the badness of that. If
> someone wants to spend a little effort researching this, I think we'd
> find that we could remove the workaround now.
> Rich

To me that sounds like some min required kernel version, would make
a useful configure option.

Norbert