Date: Thu, 11 Apr 2019 18:03:16 +0200 From: Norbert Lange <nolange79@...il.com> To: musl@...ts.openwall.com Subject: Re: use of varargs in open and various functions > > 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.  > > (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 . > 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. > >  https://godbolt.org/z/asBT92 > >  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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.