Re: use of varargs in open and various functions

Re: use of varargs in open and various functions

[Norbert Lange]

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

Re: Re: use of varargs in open and various functions

[Rich Felker]

On Thu, Apr 11, 2019 at 06:03:16PM +0200, Norbert Lange wrote:
> >
> > 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.

Indeed, that detail is not relevant to your specific changes, but the
point was to highlight that there *are* ABI subtleties here and doing
it wrong for the sake of liking the wrong way of doing it is asking
for trouble.

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

That's not how it works. Symbolic tools do not have "disable LTO for
these functions". See also things like wasm.

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

Basically nobody uses syscall(), except some very low-level software
or software using hacks to get access to bleeding-edge functionality.
It's a junk interface that shouldn't exist publicly. If it can't be
supported in some environments, that's largely okay. On the other hand
everything uses open.

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

It has to be that way because that's the public interface definition,
and the language does not make the two forms equivalent.

> > > [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.
> 
> To me that sounds like some min required kernel version, would make
> a useful configure option.

We don't have configure options like that, and don't have any hard min
kernel version. Back to 2.5.x "fully works" with the functionality
limitations (lack of *at functions, etc.) and bugs (especially lots of
signal-related bugs) those versions imply. Back to 2.4.0 or earlier
works on some archs, without threads (pthread_create will fail). The
buggy versions are somewhere in the late 2.6.x or 3.x range IIRC, way
above the "fully works" threshold. So the question is whether they
were every widely distributed without being fixed in environments
someone might encounter in the wild where fd leakage would matter.

Rich

Re: use of varargs in open and various functions

[Norbert Lange]

> 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,
> Tough. The interfaces are just defined this way. In case of fcntl, it
> is merely a convenience of circumstance that allows musl to do what it
> does. fcntl()'s third argument can be a pointer to various structures,
> or an unsigned long. Linux requires a C implementation where pointers
> can be losslessly converted to long and back, but that is the only
> reason this call to va_arg() works.

Those are C interfaces, they can be implemented in any language you
you see fit.
if you write asm, you can just pick the registers or pull the values from
the stack.
declaration can and should stay the same, no point stressing this further.

> > the only reason to use varags would be:
> >
> > 1.   varying argument types (plainly not possibly to replace with
> > direct arguments)
> That is done in fcntl(). That last arg can be a pointer to struct flock,
> or it can be an int, or it can be nothing.
> Ooh, Rich, someone found UB! If fcntl() is called with one of the codes
> requiring no argument (such as F_GETFD), then the va_arg() in there
> tries to pop an argument off an empty argument list. Which is UB, if I'm
> not mistaken. In fact, the patch for open() to avoid exactly this
> problem happened not so long ago, right?

UB can still be deterministic. Just as longs have the same size in linux,
some assumptions could be made for a c library targeting linux.
dlsym further assumes void * and void (*)() have the same size.

>
> Now for the master class: Are there ioctl() codes that also require no
> argument?
> > 2.   different parameter passing than direct arguments (ABI compatibility)
> I have worked with ABIs like this before. Granted, not on Linux, but
> they do exist. I don't think anyone here wants to introduce a dependency
> on an ABI definition.

Would be debatable to me (I know I have no say in the decission),
it certainly would restrict the scope of the project.

> > 3.   accessing parameters the caller has not provided.
> >
> Well, that is probably UB, but maybe you just worded it badly.

yes, just as accessing that same parameter via a vararg,
which musl does in several occasions. the manpage of the functions
might not define a call without argument, but the C declaration certainly does.
(practically it will be garbage from a register or stack.)

>
> > 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,
> >     atleast its true for most of them.
> What is true of modern architectures? Also, no testing required,
> parameter passing is defined in the relevant ABI documents.
> > 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.
> >     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.
> >
> In practice, I would expect such a va_arg() call to return an
> arbitrary value. But yeah, it is UB, so anything *might* happen.
> > 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]
> Ah, "I tested it and it works for me". There are a few people here for
> whom that will not be good enough.

Shees. means I did not dig through ABI docs, but looked at
what compilers are spitting out.

> > (sidenote: clang generates absolutely awful code for those varag functions).
> >
> Well, then maybe they need to tighten up their vararg codegen.
> > 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).
> >
> Yes, and a very simple one: UB. open() has been declared with an
> ellipsis for ages now, and in fact this has made it into POSIX[1]. So we
> can't change the declaration in the header file. Defining the function
> differently from its declaration will result in a compiler error. And
> even if we got past that (by preventing the compiler from seeing the
> public declaration of the function when compiling it), it would invoke
> UB to declare a function differently from its definition, and then call
> that function.

since the function might be, and mostly is, compiled in a different translation
unit or sitting in a DSO here is where ABIs do matter.

> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> > Further I would like to add that Torvalds shares a similar view [2].
> >
> When has an argument from authority last worked for you? Does he have
> anything to say in that mail?

It was pointless to do that obviously, it was not my intent to diverse from what
I wrote/asked.

> [One read later]
> Well, if you look carefully, between the derision he actually managed to
> plug in the argument that glibc's open() implementation is (was) buggy,
> because it fails to support a use case devised nineteen years after the
> code was initially written.
> This seems to stem from the viewpoint that the open() libc function
> should be a thin wrapper around the SYS_open system call. And that is a
> fundamental misunderstanding. No, the libc's job is to provide an open()
> function that fulfills the contract set out in POSIX, and it might use
> the SYS_open system call to do it, or might not. That is an
> implementation detail.
> (Generalization of the above argument to _all_ system calls is left as
> an excercise to the reader).
> > Kind regards,
> > Norbert
> >
> > [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.
>
> Well, it'll make the window smaller. Also, if an application uses
> O_CLOEXEC, that means it expects FD_CLOEXEC to be set on the returned
> FD. Which has some detectable meaning, even if there is a window when
> the FD can leak to children of other threads.
> > diff -burN musl-1.1.21.org/src/fcntl/fcntl.c musl-1.1.21/src/fcntl/fcntl.c
> > --- musl-1.1.21.org/src/fcntl/fcntl.c 2019-04-10 13:35:43.589279396 +0200
> > +++ musl-1.1.21/src/fcntl/fcntl.c 2019-04-10 13:53:46.979931879 +0200
> > @@ -1,16 +1,12 @@
> >  #define _GNU_SOURCE
> > +#define fcntl undef_fcntl
> >  #include <fcntl.h>
> > -#include <stdarg.h>
> >  #include <errno.h>
> >  #include "syscall.h"
> > +#undef fcntl
> >
> My toenails are curling up at this...

Yeah, you could have taken from the example that I don't
want to touch declarations in header files.

Regards,
Norbert

Re: use of varargs in open and various functions

[Markus Wichmann]

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,

Tough. The interfaces are just defined this way. In case of fcntl, it
is merely a convenience of circumstance that allows musl to do what it
does. fcntl()'s third argument can be a pointer to various structures,
or an unsigned long. Linux requires a C implementation where pointers
can be losslessly converted to long and back, but that is the only
reason this call to va_arg() works.

> the only reason to use varags would be:
>
> 1.   varying argument types (plainly not possibly to replace with
> direct arguments)

That is done in fcntl(). That last arg can be a pointer to struct flock,
or it can be an int, or it can be nothing.

Ooh, Rich, someone found UB! If fcntl() is called with one of the codes
requiring no argument (such as F_GETFD), then the va_arg() in there
tries to pop an argument off an empty argument list. Which is UB, if I'm
not mistaken. In fact, the patch for open() to avoid exactly this
problem happened not so long ago, right?

Now for the master class: Are there ioctl() codes that also require no
argument?

> 2.   different parameter passing than direct arguments (ABI compatibility)

I have worked with ABIs like this before. Granted, not on Linux, but
they do exist. I don't think anyone here wants to introduce a dependency
on an ABI definition.

> 3.   accessing parameters the caller has not provided.
>

Well, that is probably UB, but maybe you just worded it badly.

> 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,
>     atleast its true for most of them.

What is true of modern architectures? Also, no testing required,
parameter passing is defined in the relevant ABI documents.

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

In practice, I would expect such a va_arg() call to return an
arbitrary value. But yeah, it is UB, so anything *might* happen.

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

Ah, "I tested it and it works for me". There are a few people here for
whom that will not be good enough.

> (sidenote: clang generates absolutely awful code for those varag functions).
>

Well, then maybe they need to tighten up their vararg codegen.

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

Yes, and a very simple one: UB. open() has been declared with an
ellipsis for ages now, and in fact this has made it into POSIX[1]. So we
can't change the declaration in the header file. Defining the function
differently from its declaration will result in a compiler error. And
even if we got past that (by preventing the compiler from seeing the
public declaration of the function when compiling it), it would invoke
UB to declare a function differently from its definition, and then call
that function.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

> Further I would like to add that Torvalds shares a similar view [2].
>

When has an argument from authority last worked for you? Does he have
anything to say in that mail?

[One read later]

Well, if you look carefully, between the derision he actually managed to
plug in the argument that glibc's open() implementation is (was) buggy,
because it fails to support a use case devised nineteen years after the
code was initially written.

This seems to stem from the viewpoint that the open() libc function
should be a thin wrapper around the SYS_open system call. And that is a
fundamental misunderstanding. No, the libc's job is to provide an open()
function that fulfills the contract set out in POSIX, and it might use
the SYS_open system call to do it, or might not. That is an
implementation detail.

(Generalization of the above argument to _all_ system calls is left as
an excercise to the reader).

> Kind regards,
> Norbert
>
> [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.


Well, it'll make the window smaller. Also, if an application uses
O_CLOEXEC, that means it expects FD_CLOEXEC to be set on the returned
FD. Which has some detectable meaning, even if there is a window when
the FD can leak to children of other threads.

> diff -burN musl-1.1.21.org/src/fcntl/fcntl.c musl-1.1.21/src/fcntl/fcntl.c
> --- musl-1.1.21.org/src/fcntl/fcntl.c	2019-04-10 13:35:43.589279396 +0200
> +++ musl-1.1.21/src/fcntl/fcntl.c	2019-04-10 13:53:46.979931879 +0200
> @@ -1,16 +1,12 @@
>  #define _GNU_SOURCE
> +#define fcntl undef_fcntl
>  #include <fcntl.h>
> -#include <stdarg.h>
>  #include <errno.h>
>  #include "syscall.h"
> +#undef fcntl
>

My toenails are curling up at this...

Ciao,
Markus

Re: use of varargs in open and various functions

[Rich Felker]

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

use of varargs in open and various functions

[Norbert Lange]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

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,
    atleast its true for most of them.
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.
    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].

Kind regards,
Norbert

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

[-- Attachment #2: lessvarargs.patch --]
[-- Type: text/x-patch, Size: 5933 bytes --]

diff -burN musl-1.1.21.org/src/fcntl/fcntl.c musl-1.1.21/src/fcntl/fcntl.c
--- musl-1.1.21.org/src/fcntl/fcntl.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/fcntl.c	2019-04-10 13:53:46.979931879 +0200
@@ -1,16 +1,12 @@
 #define _GNU_SOURCE
+#define fcntl undef_fcntl
 #include <fcntl.h>
-#include <stdarg.h>
 #include <errno.h>
 #include "syscall.h"
+#undef fcntl
 
-int fcntl(int fd, int cmd, ...)
+int fcntl(int fd, int cmd, unsigned long arg)
 {
-	unsigned long arg;
-	va_list ap;
-	va_start(ap, cmd);
-	arg = va_arg(ap, unsigned long);
-	va_end(ap);
 	if (cmd == F_SETFL) arg |= O_LARGEFILE;
 	if (cmd == F_SETLKW) return syscall_cp(SYS_fcntl, fd, cmd, (void *)arg);
 	if (cmd == F_GETOWN) {
diff -burN musl-1.1.21.org/src/fcntl/openat.c musl-1.1.21/src/fcntl/openat.c
--- musl-1.1.21.org/src/fcntl/openat.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/openat.c	2019-04-10 13:53:58.088037210 +0200
@@ -1,18 +1,10 @@
+#define openat undef_openat
 #include <fcntl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef openat
 
-int openat(int fd, const char *filename, int flags, ...)
+int openat(int fd, const char *filename, int flags, mode_t mode)
 {
-	mode_t mode = 0;
-
-	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
-		va_list ap;
-		va_start(ap, flags);
-		mode = va_arg(ap, mode_t);
-		va_end(ap);
-	}
-
 	return syscall_cp(SYS_openat, fd, filename, flags|O_LARGEFILE, mode);
 }
 
diff -burN musl-1.1.21.org/src/fcntl/open.c musl-1.1.21/src/fcntl/open.c
--- musl-1.1.21.org/src/fcntl/open.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/fcntl/open.c	2019-04-10 13:53:53.939997879 +0200
@@ -1,18 +1,10 @@
+#define open undef_open
 #include <fcntl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef open
 
-int open(const char *filename, int flags, ...)
+int open(const char *filename, int flags, mode_t mode)
 {
-	mode_t mode = 0;
-
-	if ((flags & O_CREAT) || (flags & O_TMPFILE) == O_TMPFILE) {
-		va_list ap;
-		va_start(ap, flags);
-		mode = va_arg(ap, mode_t);
-		va_end(ap);
-	}
-
 	int fd = __sys_open_cp(filename, flags, mode);
 	if (fd>=0 && (flags & O_CLOEXEC))
 		__syscall(SYS_fcntl, fd, F_SETFD, FD_CLOEXEC);
diff -burN musl-1.1.21.org/src/legacy/ulimit.c musl-1.1.21/src/legacy/ulimit.c
--- musl-1.1.21.org/src/legacy/ulimit.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/legacy/ulimit.c	2019-04-10 14:03:23.113362412 +0200
@@ -1,17 +1,13 @@
+#define ulimit undef_ulimit
 #include <sys/resource.h>
 #include <ulimit.h>
-#include <stdarg.h>
+#undef ulimit
 
-long ulimit(int cmd, ...)
+long ulimit(int cmd, long val)
 {
 	struct rlimit rl;
 	getrlimit(RLIMIT_FSIZE, &rl);
 	if (cmd == UL_SETFSIZE) {
-		long val;
-		va_list ap;
-		va_start(ap, cmd);
-		val = va_arg(ap, long);
-		va_end(ap);
 		rl.rlim_cur = 512ULL * val;
 		if (setrlimit(RLIMIT_FSIZE, &rl)) return -1;
 	}
diff -burN musl-1.1.21.org/src/linux/clone.c musl-1.1.21/src/linux/clone.c
--- musl-1.1.21.org/src/linux/clone.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/clone.c	2019-04-10 14:00:41.015839619 +0200
@@ -1,21 +1,13 @@
 #define _GNU_SOURCE
-#include <stdarg.h>
+#define clone undef_clone
 #include <unistd.h>
 #include <sched.h>
 #include "pthread_impl.h"
 #include "syscall.h"
+#undef clone
 
-int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
+int clone(int (*func)(void *), void *stack, int flags, void *arg,
+	pid_t *ptid, void *tls, pid_t *ctid)
 {
-	va_list ap;
-	pid_t *ptid, *ctid;
-	void  *tls;
-
-	va_start(ap, arg);
-	ptid = va_arg(ap, pid_t *);
-	tls  = va_arg(ap, void *);
-	ctid = va_arg(ap, pid_t *);
-	va_end(ap);
-
 	return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
 }
diff -burN musl-1.1.21.org/src/linux/prctl.c musl-1.1.21/src/linux/prctl.c
--- musl-1.1.21.org/src/linux/prctl.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/prctl.c	2019-04-10 13:57:26.570008404 +0200
@@ -1,14 +1,9 @@
+#define prctl undef_prctl
 #include <sys/prctl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef prctl
 
-int prctl(int op, ...)
+int prctl(int op, unsigned long x0, unsigned long x1, unsigned long x2, unsigned long x3)
 {
-	unsigned long x[4];
-	int i;
-	va_list ap;
-	va_start(ap, op);
-	for (i=0; i<4; i++) x[i] = va_arg(ap, unsigned long);
-	va_end(ap);
-	return syscall(SYS_prctl, op, x[0], x[1], x[2], x[3]);
+	return syscall(SYS_prctl, op, x0, x1, x2, x3);
 }
diff -burN musl-1.1.21.org/src/linux/ptrace.c musl-1.1.21/src/linux/ptrace.c
--- musl-1.1.21.org/src/linux/ptrace.c	2019-04-10 13:35:43.589279396 +0200
+++ musl-1.1.21/src/linux/ptrace.c	2019-04-10 14:01:09.280105349 +0200
@@ -1,26 +1,13 @@
+#define ptrace undef_ptrace
 #include <sys/ptrace.h>
-#include <stdarg.h>
 #include <unistd.h>
 #include "syscall.h"
+#undef ptrace
 
-long ptrace(int req, ...)
+long ptrace(int req, pid_t pid, void *addr, void *data, void *addr2)
 {
-	va_list ap;
-	pid_t pid;
-	void *addr, *data, *addr2 = 0;
 	long ret, result;
 
-	va_start(ap, req);
-	pid = va_arg(ap, pid_t);
-	addr = va_arg(ap, void *);
-	data = va_arg(ap, void *);
-	/* PTRACE_{READ,WRITE}{DATA,TEXT} (16...19) are specific to SPARC. */
-#ifdef PTRACE_READDATA
-	if ((unsigned)req - PTRACE_READDATA < 4)
-		addr2 = va_arg(ap, void *);
-#endif
-	va_end(ap);
-
 	if (req-1U < 3) data = &result;
 	ret = syscall(SYS_ptrace, req, pid, addr, data, addr2);
 
diff -burN musl-1.1.21.org/src/misc/ioctl.c musl-1.1.21/src/misc/ioctl.c
--- musl-1.1.21.org/src/misc/ioctl.c	2019-04-10 13:35:43.605279563 +0200
+++ musl-1.1.21/src/misc/ioctl.c	2019-04-10 14:01:52.920515470 +0200
@@ -1,13 +1,9 @@
+#define ioctl undef_ioctl
 #include <sys/ioctl.h>
-#include <stdarg.h>
 #include "syscall.h"
+#undef ioctl
 
-int ioctl(int fd, int req, ...)
+int ioctl(int fd, int req, void *arg)
 {
-	void *arg;
-	va_list ap;
-	va_start(ap, req);
-	arg = va_arg(ap, void *);
-	va_end(ap);
 	return syscall(SYS_ioctl, fd, req, arg);
 }