Re: use of varargs in open and various functions

[Rich Felker <dalias@libc.org>]

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.

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.

> 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).

>     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. :-)

What benefit are you trying to get from this? Just some size
reduction? 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.)

> [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