[musl] Re: Tweaking the program name for <err.h> functions

[musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

[TO += musl]

Hi!

On Fri, Mar 08, 2024 at 01:30:51AM +0100, Guillem Jover wrote:
> That is not portable because the BSDs do not support that variable.
> But all BSDs for which I've got a checkout (FreeBSD, NetBSD, OpenBSD,
> DragonflyBSD) support changing it via setprogname(), but that's not
> available in glibc (libbsd provides it though).
> 
> libbsd and cosmopolitan libc support setting it via setprogname() or
> program_invocation_short_name.
> 
> musl libc supports setting it via program_invocation_short_name.
> 
> uclibc might support __progname and/or program_invocation_short_name
> depending on how it has been configured, but it does not support
> setting the err(3) program name via those.

Hmmm.

	$ cat err.c 
	#define _GNU_SOURCE
	#include <bsd/stdlib.h>
	#include <err.h>
	#include <errno.h>
	#include <error.h>


	int
	main(void)
	{
		program_invocation_name = "foo";
		program_invocation_short_name = "bar";
		setprogname("baz");

		error(0, errno, "fmt string");
		err(1, "fmt2");
	}
	$ cc -Wall -Wextra err.c -lbsd
	$ ./a.out 
	foo: fmt string
	baz: fmt2: Success

This would already be portable enough for what I want, except that
libbsd isn't very welcome in some OSes as a core library.  I guess I'll
need libc support.

> 
> > Maybe it would be interesting to get the BSDs to support these pointers?
> 
> They already have a way to control the program name though. (And
> it seems to me that using functions instead of global variables is
> superior. :)

Indeed.  I didn't know about such function.  I'll reformulate my
original suggestion to this other one:

How about adding setprogname(3) (and getprogname(3)) to GNU and musl
libc?

> 
> Thanks,
> Guillem

Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Fri, Mar 08, 2024 at 01:52:28AM +0100, Alejandro Colomar wrote:
> [TO += musl]
> 
> Hi!
> 
> On Fri, Mar 08, 2024 at 01:30:51AM +0100, Guillem Jover wrote:
> > That is not portable because the BSDs do not support that variable.
> > But all BSDs for which I've got a checkout (FreeBSD, NetBSD, OpenBSD,
> > DragonflyBSD) support changing it via setprogname(), but that's not
> > available in glibc (libbsd provides it though).
> > 
> > libbsd and cosmopolitan libc support setting it via setprogname() or
> > program_invocation_short_name.
> > 
> > musl libc supports setting it via program_invocation_short_name.
> > 
> > uclibc might support __progname and/or program_invocation_short_name
> > depending on how it has been configured, but it does not support
> > setting the err(3) program name via those.
> 
> Hmmm.
> 
> 	$ cat err.c 
> 	#define _GNU_SOURCE
> 	#include <bsd/stdlib.h>
> 	#include <err.h>
> 	#include <errno.h>
> 	#include <error.h>
> 
> 
> 	int
> 	main(void)
> 	{
> 		program_invocation_name = "foo";
> 		program_invocation_short_name = "bar";
> 		setprogname("baz");
> 
> 		error(0, errno, "fmt string");
> 		err(1, "fmt2");
> 	}
> 	$ cc -Wall -Wextra err.c -lbsd
> 	$ ./a.out 
> 	foo: fmt string
> 	baz: fmt2: Success
> 
> This would already be portable enough for what I want, except that
> libbsd isn't very welcome in some OSes as a core library.  I guess I'll
> need libc support.
> 
> > 
> > > Maybe it would be interesting to get the BSDs to support these pointers?
> > 
> > They already have a way to control the program name though. (And
> > it seems to me that using functions instead of global variables is
> > superior. :)
> 
> Indeed.  I didn't know about such function.  I'll reformulate my
> original suggestion to this other one:
> 
> How about adding setprogname(3) (and getprogname(3)) to GNU and musl
> libc?

I really don't think they meet the criteria for inclusion. They don't
have historical cross platform precedent, they're not something we've
hit existing applications failing to build for lack of, they don't let
you do anything you couldn't already do, and the semantics would be
unclear (do they just configure the legacy err.h functions? set
__progname/program_invocation_short_name? modify argv[0]?)

The entire err.h function set in musl is 67 lines that compile down to
less than 500 bytes of machine code. If there's not a portable way to
configure them the way you want, and you refuse to run a configure
script of some sort to determine if the setprogname function some
systems need exists, the simpler solution, rather than trying to get
new contracts into libc and wait for them to be widely available, is
to copy-and-paste those 67 lines and customize them as needed in your
program, no?

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

Hi Rich,

On Sat, Mar 09, 2024 at 10:02:58AM -0500, Rich Felker wrote:
> > How about adding setprogname(3) (and getprogname(3)) to GNU and musl
> > libc?
> 
> I really don't think they meet the criteria for inclusion. They don't
> have historical cross platform precedent, they're not something we've
> hit existing applications failing to build for lack of, they don't let
> you do anything you couldn't already do, and the semantics would be
> unclear

I would ask the BSD authors of the API to learn all the implications of
the pair of functions.  I don't know them.  Maybe Guillem knows.

> (do they just configure the

> legacy err.h functions?

Why would you call err.h legacy?  What do you offer to use instead?
snprintf(3) + perror(3) + exit(3)?

How about bugs in such NIH cases?  You'd fix them in every project where
you've written the same bug?  That's why we have libraries.  I don't
mind if the entirety of err.h was moved to a liberr standalone library,
but you have kidnapped those APIs in libc.  Would you release them?

> set
> __progname/program_invocation_short_name? modify argv[0]?)

Would you promise to keep __progname/program_invocation_short_name as a
way to configure err.h in musl/glibc?  If so, then maybe one can write a
portable library that wraps the unportable set of libc's.  But since you
own err.h, _you_ must be the one making a contract.  It's up to you to
decide which contract you want to offer.  I suggest making the same
contract present in the BSDs.

> 
> The entire err.h function set in musl is 67 lines that compile down to
> less than 500 bytes of machine code. If there's not a portable way to
> configure them the way you want,

There's not a portable way to configure them, AFAIK.  You could say it's
glibc and musl's fault, for importing the err.h functions without
importing setprogname(3).  In the BSD world, where these APIs
originated, they seem to be configurable portably (within BSDs).  And
with libbsd, you can get relative portability to other systems.

You have a problem when you want to call these functions in a core
package, like shadow, because some distributions refuse to include
libbsd in their core packages.

> and you refuse to run a configure
> script of some sort to determine if the setprogname function some
> systems need exists, the simpler solution,

I don't refuse to do that, but it means I also need to write code to
handle the case where it's not available, and do some non-portable stuff
in that case.

In fact, I'll need to do that anyway, at least for some years until all
systems I care about have a version of libc that has the functions.

But, I'd like to be able to remove that code in, say, 5 or 10 years from
now.

> rather than trying to get
> new contracts into libc and wait for them to be widely available, is
> to copy-and-paste those 67 lines and customize them as needed in your
> program, no?

I partially agree with you, in that I don't like adding new contracts
into libc.  But that contract was already added broken into glibc and
musl.  I suggest you remove err.h from libc, and let a standalone
library to implement them separately, allowing to configure them.

I don't agree with your suggestion of going the NIH way.

Maybe I'll push a bit harder for inclusion of libbsd in distributions.
Then it'll be the problem of distributors to either package libbsd, or
do a lot of work to patch projects to not rely on libbsd.  It's not a
bad idea, actually.

> 
> Rich

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[musl] Re: Tweaking the program name for <err.h> functions

[Andreas Schwab]

On Mär 09 2024, Alejandro Colomar wrote:

> There's not a portable way to configure them, AFAIK.  You could say it's
> glibc and musl's fault, for importing the err.h functions without
> importing setprogname(3).

When glibc imported err, setprogname didn't exist yet.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

Hi Andreas,

On Sat, Mar 09, 2024 at 07:35:27PM +0100, Andreas Schwab wrote:
> On Mär 09 2024, Alejandro Colomar wrote:
> 
> > There's not a portable way to configure them, AFAIK.  You could say it's
> > glibc and musl's fault, for importing the err.h functions without
> > importing setprogname(3).
> 
> When glibc imported err, setprogname didn't exist yet.

Thanks.  Then BSD extended the contract.  That's still a problem of musl
and glibc.  The API is deficient without setprogname(3), and should be
fixed.  I think libc should either drop err.h and let another library
take ownership of the API, or add a way to configure it, hopefully being
compatible with the BSDs.  No?

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Markus Wichmann]

Am Sat, Mar 09, 2024 at 07:46:38PM +0100 schrieb Alejandro Colomar:
> Thanks.  Then BSD extended the contract.  That's still a problem of musl
> and glibc.  The API is deficient without setprogname(3), and should be
> fixed.  I think libc should either drop err.h and let another library
> take ownership of the API, or add a way to configure it, hopefully being
> compatible with the BSDs.  No?
>

Well, that's the problem with library code. musl will never drop
existing functionality for ABI stability alone. The most that was ever
done was dropping the LFS64 symbols, and there the symbols are only
hidden from the linker, but the dynamic linker can still find them.

So no, musl will not be dropping the err.h functions. Of course, you can
install a library that overrides these symbols. libc is always linked in
last, and UNIX has a long and storied history of superseding symbols.

In this particular case, though, if BSD extended the contract, should
the onus of checking not be on the application? I think the application
should check whether the extended or basic contract is in effect.

Ciao,
Markus

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Sat, Mar 09, 2024 at 07:46:38PM +0100, Alejandro Colomar wrote:
> Hi Andreas,
> 
> On Sat, Mar 09, 2024 at 07:35:27PM +0100, Andreas Schwab wrote:
> > On Mär 09 2024, Alejandro Colomar wrote:
> > 
> > > There's not a portable way to configure them, AFAIK.  You could say it's
> > > glibc and musl's fault, for importing the err.h functions without
> > > importing setprogname(3).
> > 
> > When glibc imported err, setprogname didn't exist yet.
> 
> Thanks.  Then BSD extended the contract.  That's still a problem of musl
> and glibc.  The API is deficient without setprogname(3), and should be
> fixed.  I think libc should either drop err.h and let another library
> take ownership of the API, or add a way to configure it, hopefully being
> compatible with the BSDs.  No?

libc can't drop anything at the binary level because that's breaking
existing dynamic linked programs. It could remove the header, but that
has its own problems.

The err.h interfaces are called "legacy" in musl (appearing in
src/legacy) because they are not part of any standard and are just
something a bunch of ancient crufty code used and assumed was there.
They were added to musl early on (back in 2011) because they were
very-low-cost to add, no interaction with anything else (they didn't
even support progname at the time), and made it easier for people who
were getting musl-based systems up for the first time. They don't do
anything significant you can't do yourself with fprintf/perror, the
err forms exit without giving you any opportunity for clean exit
(except installing atexit handlers, which are their own mess of global
state), and as implmented aren't really multithread-friendly (they
don't lock around the multiple stdio calls to make the error output
atomic). Basically, they just go with a really, really antiquated
programming model, and do not seem like the sort of thing anyone
should be investing development time into improving *or* intentionally
breaking.

I agree there are sometimes hazards to copy-and-paste, but in the case
of these, a copy-and-paste version (or just a rewrite) where you can
wire up your own better behaviors more suited to your application is
really *better* than using them as-is. If you have your own header and
definition for these functions, the ones in libc will not even get
used. There is no "ownership" conflict.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Thorsten Glaser]

Alejandro Colomar dixit:

>You could say it's
>glibc and musl's fault, for importing the err.h functions without
>importing setprogname(3).  In the BSD world, where these APIs
>originated, they seem to be configurable portably (within BSDs).  And

Nope. They are rather new, there are BSDs without them
(where “extern const char *__progname;” is still used).

bye,
//mirabilos
-- 
FWIW, I'm quite impressed with mksh interactively. I thought it was much
*much* more bare bones. But it turns out it beats the living hell out of
ksh93 in that respect. I'd even consider it for my daily use if I hadn't
wasted half my life on my zsh setup. :-) -- Frank Terbeck in #!/bin/mksh

Re: [musl] Re: Tweaking the program name for <err.h> functions

[NRK]

> What do you offer to use instead? snprintf(3) + perror(3) + exit(3)?

No need to have an intermediate buffer with snprintf when you can just
use vfprintf directly.

> I suggest you remove err.h from libc, and let a standalone library to
> implement them separately, allowing to configure them.

These are not mutually exclusive. You can have err.h in libc while also
having them as separate library.

And besides, these are not good interfaces anyways. Aside from what Rich
already said, you'll realize this soon when you need to use various
posix_* or pthread_* functions which don't set the errno and instead
return an error code.

Also, I don't think your fear of "NIH bug" is well grounded. This is not
some highly complicated error-prone code, it's just simple logging
facility.

I am aware that there exists certain programming cultures [*] where
having to write code instead of "import leftpad" is seen as taboo of the
highest order. But in this case you don't even *have to* write anything
when you can just copy err.c from musl and customize it.

(* Counterculture to this also exists, such as Go's "A little copying
is better than a little dependency" proverb for example.)

- NRK

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

Hi NRK,

On Sun, Mar 10, 2024 at 06:01:14AM +0000, NRK wrote:
> > What do you offer to use instead? snprintf(3) + perror(3) + exit(3)?
> 
> No need to have an intermediate buffer with snprintf when you can just
> use vfprintf directly.

For an interface that prefixes the program name, I need to either call
[v]fprintf(3) twice, or add locks; that is:

	lock()
	fprintf("%s: ", __progname);
	vfprintf(...);
	unlock();

or prepare the buffer and call it just once.

Because I want to call it as foo("reason");, not having to use
__progname all the time.

> 
> > I suggest you remove err.h from libc, and let a standalone library to
> > implement them separately, allowing to configure them.
> 
> These are not mutually exclusive. You can have err.h in libc while also
> having them as separate library.

If I #include <err.h>, glibc considers those as reserved names.

<https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html>

| All other library names are reserved if your program explicitly
| includes the header file that defines or declares them.

I guess it would work, because of weak symbols, but the manual
explicitly says don't do this.

> And besides, these are not good interfaces anyways. Aside from what Rich
> already said, you'll realize this soon when you need to use various
> posix_* or pthread_* functions which don't set the errno and instead
> return an error code.

There's errc(3), and also warnc() (Guillem, libbsd is missing the link
pages for warn*() variants!), to which you can pass an errno-like code.
This isn't supported by glibc, but is available in libbsd (and of
course, in the BSDs; at least I checked OpenBSD).

And for the finctions that don't report an errno-like, like the
gai_error(3) ones, you can use errx(3), and also warnx(3).  These are
supported in glibc.

So these are actually quite well suited for different kinds of error-
reporting functions.

Regarding multi-threaded programs:

If you're recommending writing a version because it's easy, but then
you say it needs locking, well, I wouldn't say writing a locking version
is easy.

> 
> Also, I don't think your fear of "NIH bug" is well grounded. This is not
> some highly complicated error-prone code, it's just simple logging
> facility.

locking code is error-prone, I'd say.

> 
> I am aware that there exists certain programming cultures [*] where
> having to write code instead of "import leftpad" is seen as taboo of the
> highest order. But in this case you don't even *have to* write anything
> when you can just copy err.c from musl and customize it.

I think the BSD err.h functions are actually quite well designed for
single-threaded programs (still the vast majority of software).

And for multi-threaded versions, we just need to convince the BSD
maintainers that they need to add locking.  If glibc and musl don't
consider these APIs as useless and legacy, and fix them, then maybe the
BSDs follow.

Again, is there anything better in glibc or musl?
Something that prefixes "$progname: " and appends the errno message?

Yeah, you could write fprintf("%s: ...", program_invocation_short_name,
...) all the time?  or write a wrapper that calls
fprintf("%s: ", program_invocation_short_name), but then you need to add
locking?  and then you need to set program_invocation_short_name = "su"?
And then add *c() for functions that return an errno-like code?  And
then add *x() variants for functions that don't use errno-like codes?
Oh well, you're basically writing the err.h interface.  The interface is
good.  It's the implementation that has small problems, but I say let's
fix them.  This is certainly not what I want to be doing for every
project.

And as said above, having an overlay library that defines err.h is a
violation of glibc's reserved names.  Not too bad if done in a library,
but certainly not that I'd do in every project.

Have a lovely day!
Alex

> 
> (* Counterculture to this also exists, such as Go's "A little copying
> is better than a little dependency" proverb for example.)
> 
> - NRK

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[NRK]

>  or add locks; that is:
> 
> 	lock()
> 	fprintf("%s: ", __progname);
> 	vfprintf(...);
> 	unlock();
>
> [...]
>
> locking code is error-prone, I'd say.

These interfaces do not guarantee the output to be atomic. If you were
expecting it to be atomic then that's just *another* reason to roll it
yourself because a good ton of existing implementation doesn't lock.

https://github.com/bminor/musl/blob/master/src/legacy/err.c
https://github.com/freebsd/freebsd-src/blob/main/lib/libc/gen/err.c
https://github.com/openbsd/src/blob/master/lib/libc/gen/verr.c
https://cgit.freedesktop.org/libbsd/tree/src/err.c

musl doesn't, freebsd doesn't, openbsd doesn't, libbsd doesn't. Out of
the 5 implementations I checked, only glibc seems to lock.

> There's errc(3)

Which doesn't exist on musl, I don't think it exists on glibc either. So
you're back to "DIY or depend on libbsd" land if you use this function.

> Again, is there anything better in glibc or musl?
> Something that prefixes "$progname: " and appends the errno message?
> [...]
> And then add *c() for functions that return an errno-like code? And
> then add *x() variants for functions that don't use errno-like codes?

glibc has error(3), and program_invocation_name(3) to customize $progname.
Interface wise, I find it more pleasant than the err.h gang. Having an
explicit `errnum` argument serves all 3 usecases (no errno, errno,
errno-like return code) without having multiple functions with
x/y/z/c suffix.

(One issue I have with glibc's error() interface is that doing both
warning and fatal error through same function weakens static analyzers.
I'd split up the two and mark the fatal version with _Noreturn for
better warnings/static-analysis.)

But this function is even less portable (no musl or *BSD support last I
checked). So you're back to square one.

- NRK

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Sun, Mar 10, 2024 at 02:01:18PM +0000, NRK wrote:
> >  or add locks; that is:
> > 
> > 	lock()
> > 	fprintf("%s: ", __progname);
> > 	vfprintf(...);
> > 	unlock();
> >
> > [...]
> >
> > locking code is error-prone, I'd say.
> 
> These interfaces do not guarantee the output to be atomic. If you were
> expecting it to be atomic then that's just *another* reason to roll it
> yourself because a good ton of existing implementation doesn't lock.

Also, the whole reason this comes up is gratuitous impedance mismatch
bringing in the need for a separate fprintf call to do the prefix (and
possibly newline suffix, if you want that). They could have been
designed to be one-line macros, ala...

#define warn(f,...) fprintf(stderr, "%s: " f, __progname, __VA_ARGS__)

or similar. I really see no justifiable reason for people writing new
software to want to enhance the err.h functions rather than just
rolling a one-line macro that can be better tailored to their specific
needs.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

Hi Rich,

On Sun, Mar 10, 2024 at 03:39:56PM -0400, Rich Felker wrote:
> Also, the whole reason this comes up is gratuitous impedance mismatch
> bringing in the need for a separate fprintf call to do the prefix (and
> possibly newline suffix, if you want that). They could have been
> designed to be one-line macros, ala...
> 
> #define warn(f,...) fprintf(stderr, "%s: " f, __progname, __VA_ARGS__)
> 
> or similar.

Hmmm, it's an interesting definition.  That's actually warnx(3), but you
can write the others in terms of that:


#define setprogname(n)    do { program_invocation_short_name = n; } while (0)
#define getprogname()     (program_invocation_short_name)

#define warnx(f, ...)     fprintf(stderr, "%s: " f "\n", getprogname(), ##__VA_ARGS__)
#define warnc(c, f, ...)  warnx(f ": %s", ##__VA_ARGS__, strerror(c))
#define warn(f, ...)      warnc(errno, f, ##__VA_ARGS__)

#define errx(x, ...)      do { warnx(__VA_ARGS__); exit(x); } while (0)
#define errc(x, ...)      do { warnc(__VA_ARGS__); exit(x); } while (0)
#define err(x, ...)       do { warn(__VA_ARGS__);  exit(x); } while (0)


The main problem is still portability.  But I guess with some cpp(1)
I can get it to work in the systems I care about.

libc couldn't implement it this way, because complex things like "%2ms"
wouldn't work.  But it could be interesting in a multi-threaded program,
where such complex conversion specifications don't occur.

Still, that's not my case.  I guess I'll do this:

	#if (!WITH_LIBBSD)
	inline void
	setprogname(const char *progname)
	{
		program_invocation_short_name = Basename(name);
	}

	inline const char *
	getprogname(void)
	{
		return program_invocation_short_name;
	}
	#endif

> I really see no justifiable reason for people writing new
> software to want to enhance the err.h functions rather than just

I don't really need to enhance them.  Just a statement that the current
behavior will hold would be good enough.  I would need to know that
setting 'program_invocation_short_name' will set the prefix of these
functions.  That's already the current behavior, and I guess that you'll
keep it, even if just for backwards compatibility; more so, considering
your aversion to fix or break these APIs, or to change them at all.

> rolling a one-line macro that can be better tailored to their specific
> needs.
> 
> Rich

Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Thorsten Glaser]

Rich Felker dixit:

>possibly newline suffix, if you want that). They could have been
>designed to be one-line macros, ala...

Nope.

No __VA_ARGS__ and especially no ##__VA_ARGS__ in C89, back then.

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Sun, Mar 10, 2024 at 11:22:51PM +0000, Thorsten Glaser wrote:
> Rich Felker dixit:
> 
> >possibly newline suffix, if you want that). They could have been
> >designed to be one-line macros, ala...
> 
> Nope.
> 
> No __VA_ARGS__ and especially no ##__VA_ARGS__ in C89, back then.

Well yeah, "could have been" was poor wording by me. At the time, the
options didn't exist. But someone doing that kind of interface now can
use these tools, or just design the interface differently.

With that said, while I'll grant that there are exceptions, it's
generally my impression that if you're pasting in the name of the
program from argv[0] programmatically because it can't just be part of
the string literal, because the string literal appears in modular
library code that gets called from multiple utilities, then printing
an error message (and even worse, exiting, if you do that too), rather
than returning meaningful error information up to the caller for it to
handle/display, is just really sloppy, low-quality programming.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Thorsten Glaser]

Rich Felker dixit:

>the string literal, because the string literal appears in modular
>library code that gets called from multiple utilities, then printing
>an error message (and even worse, exiting, if you do that too), rather
>than returning meaningful error information up to the caller for it to
>handle/display, is just really sloppy, low-quality programming.

Libraries totally should not call exit and thus not err/errx,
and warn/warnx is… also questionable at best.

But modularised code that builds a shared object and a few
binaries using it? Why not.

The thing I don’t get is why changing __progname is desired,
but I guess everyone has use cases for something.

bye,
//mirabilos
-- 
(gnutls can also be used, but if you are compiling lynx for your own use,
there is no reason to consider using that package)
	-- Thomas E. Dickey on the Lynx mailing list, about OpenSSL

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Alejandro Colomar]

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

Hi Thorsten,

On Mon, Mar 11, 2024 at 12:19:27AM +0000, Thorsten Glaser wrote:
> Rich Felker dixit:
> 
> >the string literal, because the string literal appears in modular
> >library code that gets called from multiple utilities, then printing
> >an error message (and even worse, exiting, if you do that too), rather
> >than returning meaningful error information up to the caller for it to
> >handle/display, is just really sloppy, low-quality programming.
> 
> Libraries totally should not call exit and thus not err/errx,
> and warn/warnx is… also questionable at best.
> 
> But modularised code that builds a shared object and a few
> binaries using it? Why not.
> 
> The thing I don’t get is why changing __progname is desired,
> but I guess everyone has use cases for something.

setuid programs.  Consider that a setuid program accidentally opens a
privileged file in fd 2.  Now what happens if a random user can trigger
that accident, and write arbitrary text to a privileged file, just by
calling that setuid program with execlp("su", "inject this stuff", ...)?

Bad stuff.

Have a lovely night!
Alex

> 
> bye,
> //mirabilos
> -- 
> (gnutls can also be used, but if you are compiling lynx for your own use,
> there is no reason to consider using that package)
> 	-- Thomas E. Dickey on the Lynx mailing list, about OpenSSL

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Skyler Ferrante (RIT Student)]

Hi,

"Consider that a setuid program accidentally opens a privileged file in fd 2."

It seems like this is the main thing shadow-utils (and other projects)
should be concerned about. Every setuid/setgid program should check
for fd 0,1,2 being open at the start of execution, and either abort or
open new fds to /dev/null to prevent file descriptor omission attacks.
Any defenses used to prevent exploitation when a setuid/setgid program
does not do this, seems unlikely to succeed.

All an attacker would need would be an attacker defined string going
to stdout/stderr. Argv[0] is useful for this, but it is not the only
option. Usernames/group names/etc. are chosen by attackers. Preventing
these from being printed might increase security a bit, but they would
make error messages worse. That's just my two cents.

Skyler

On Sun, Mar 10, 2024 at 8:46 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Thorsten,
>
> On Mon, Mar 11, 2024 at 12:19:27AM +0000, Thorsten Glaser wrote:
> > Rich Felker dixit:
> >
> > >the string literal, because the string literal appears in modular
> > >library code that gets called from multiple utilities, then printing
> > >an error message (and even worse, exiting, if you do that too), rather
> > >than returning meaningful error information up to the caller for it to
> > >handle/display, is just really sloppy, low-quality programming.
> >
> > Libraries totally should not call exit and thus not err/errx,
> > and warn/warnx is… also questionable at best.
> >
> > But modularised code that builds a shared object and a few
> > binaries using it? Why not.
> >
> > The thing I don’t get is why changing __progname is desired,
> > but I guess everyone has use cases for something.
>
> setuid programs.  Consider that a setuid program accidentally opens a
> privileged file in fd 2.  Now what happens if a random user can trigger
> that accident, and write arbitrary text to a privileged file, just by
> calling that setuid program with execlp("su", "inject this stuff", ...)?
>
> Bad stuff.
>
> Have a lovely night!
> Alex
>
> >
> > bye,
> > //mirabilos
> > --
> > (gnutls can also be used, but if you are compiling lynx for your own use,
> > there is no reason to consider using that package)
> >       -- Thomas E. Dickey on the Lynx mailing list, about OpenSSL
>
> --
> <https://www.alejandro-colomar.es/>

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Andreas Schwab]

On Mär 11 2024, Skyler Ferrante (RIT Student) wrote:

> It seems like this is the main thing shadow-utils (and other projects)
> should be concerned about. Every setuid/setgid program should check
> for fd 0,1,2 being open at the start of execution, and either abort or
> open new fds to /dev/null to prevent file descriptor omission attacks.

That's what glibc already does.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Skyler Ferrante (RIT Student)]

Hmm, maybe I'm missing something, but it seems you can close(fd) for
the standard fds and then call execve, and the new process image will
have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
This seems to affect shadow-utils and other setuid/setgid binaries.

Here is a repo I built for testing,
https://github.com/skyler-ferrante/fd_omission/. What is the correct
glibc behavior? Am I misunderstanding something?

Skyler

On Mon, Mar 11, 2024 at 11:09 AM Andreas Schwab <schwab@suse.de> wrote:
>
> On Mär 11 2024, Skyler Ferrante (RIT Student) wrote:
>
> > It seems like this is the main thing shadow-utils (and other projects)
> > should be concerned about. Every setuid/setgid program should check
> > for fd 0,1,2 being open at the start of execution, and either abort or
> > open new fds to /dev/null to prevent file descriptor omission attacks.
>
> That's what glibc already does.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Florian Weimer]

* Skyler Ferrante:

> Hmm, maybe I'm missing something, but it seems you can close(fd) for
> the standard fds and then call execve, and the new process image will
> have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> This seems to affect shadow-utils and other setuid/setgid binaries.
>
> Here is a repo I built for testing,
> https://github.com/skyler-ferrante/fd_omission/. What is the correct
> glibc behavior? Am I misunderstanding something?

If you run it under strace, it's not running SUID (in AT_SECURE mode).
I'm not saying we don't have bugs (although we do have some end-to-end
AT_SECURE tests in the testsuite, but probably not for this legacy
behavior), just that this approach to testing is questionable.

Thanks,
Florian

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Skyler Ferrante (RIT Student)]

Hi Florian,

> it's not running SUID (in AT_SECURE mode)

I see. I didn't realize that it had different behavior for setuid/not
setuid. That makes sense though, sorry for the confusion.

Skyler


On Mon, Mar 11, 2024 at 2:23 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Skyler Ferrante:
>
> > Hmm, maybe I'm missing something, but it seems you can close(fd) for
> > the standard fds and then call execve, and the new process image will
> > have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> > This seems to affect shadow-utils and other setuid/setgid binaries.
> >
> > Here is a repo I built for testing,
> > https://github.com/skyler-ferrante/fd_omission/. What is the correct
> > glibc behavior? Am I misunderstanding something?
>
> If you run it under strace, it's not running SUID (in AT_SECURE mode).
> I'm not saying we don't have bugs (although we do have some end-to-end
> AT_SECURE tests in the testsuite, but probably not for this legacy
> behavior), just that this approach to testing is questionable.
>
> Thanks,
> Florian
>

Re: [musl] Re: Tweaking the program name for <err.h> functions

[enh]

Android's libc actually does do this for everything except for
first-stage `init`, the one process that doesn't have a /dev/null
equivalent available yet:
https://android.googlesource.com/platform/bionic/+/master/libc/bionic/libc_init_common.cpp#358

On Mon, Mar 11, 2024 at 11:49 AM Skyler Ferrante (RIT Student)
<sjf5462@rit.edu> wrote:
>
> Hi Florian,
>
> > it's not running SUID (in AT_SECURE mode)
>
> I see. I didn't realize that it had different behavior for setuid/not
> setuid. That makes sense though, sorry for the confusion.
>
> Skyler
>
>
> On Mon, Mar 11, 2024 at 2:23 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Skyler Ferrante:
> >
> > > Hmm, maybe I'm missing something, but it seems you can close(fd) for
> > > the standard fds and then call execve, and the new process image will
> > > have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> > > This seems to affect shadow-utils and other setuid/setgid binaries.
> > >
> > > Here is a repo I built for testing,
> > > https://github.com/skyler-ferrante/fd_omission/. What is the correct
> > > glibc behavior? Am I misunderstanding something?
> >
> > If you run it under strace, it's not running SUID (in AT_SECURE mode).
> > I'm not saying we don't have bugs (although we do have some end-to-end
> > AT_SECURE tests in the testsuite, but probably not for this legacy
> > behavior), just that this approach to testing is questionable.
> >
> > Thanks,
> > Florian
> >

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Mon, Mar 11, 2024 at 12:05:11PM -0700, enh wrote:
> Android's libc actually does do this for everything except for
> first-stage `init`, the one process that doesn't have a /dev/null
> equivalent available yet:
> https://android.googlesource.com/platform/bionic/+/master/libc/bionic/libc_init_common.cpp#358

In the absence of /dev/null, you could probably call pipe() and close
the unwanted end. This works with no fs available, and has the "bonus"
that you'll get a nice SIGPIPE crash if you accidentally try to write
anything to it.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Mon, Mar 11, 2024 at 11:30:04AM -0400, Skyler Ferrante (RIT Student) wrote:
> Hmm, maybe I'm missing something, but it seems you can close(fd) for
> the standard fds and then call execve, and the new process image will
> have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> This seems to affect shadow-utils and other setuid/setgid binaries.
> 
> Here is a repo I built for testing,
> https://github.com/skyler-ferrante/fd_omission/. What is the correct
> glibc behavior? Am I misunderstanding something?

As Florian noted, you're missing that strace cannot invoke it suid.
POSIX explicitly permits the implementation to open these fds if they
started closed in suid execs, and IIRC indicates as a future direction
that it might be permitted for all execs. We do the same in musl in
the suid case. So really the only way that "writing attacker
controlled prefix strings to fd 2" becomes an issue is if the
application erroneously closes fd 2 and lets something else get opened
on it.

(Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
interesting idea... :)

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Skyler Ferrante (RIT Student)]

Yup, I agree. My confusion was from an incorrect assumption that
non-suid / suid programs would be handled the same way. I knew that
strace wouldn't keep it setuid by I didn't realize glibc only checked
closed fds for suid programs (which makes sense, this doesn't matter
for non-privileged programs).

> application erroneously closes fd 2

And hopefully no program does that, and if it does, that's their fault :)

Skyler

Re: [musl] Re: Tweaking the program name for <err.h> functions

[enh]

On Mon, Mar 11, 2024 at 12:44 PM Rich Felker <dalias@libc.org> wrote:
>
> On Mon, Mar 11, 2024 at 12:05:11PM -0700, enh wrote:
> > Android's libc actually does do this for everything except for
> > first-stage `init`, the one process that doesn't have a /dev/null
> > equivalent available yet:
> > https://android.googlesource.com/platform/bionic/+/master/libc/bionic/libc_init_common.cpp#358
>
> In the absence of /dev/null, you could probably call pipe() and close
> the unwanted end. This works with no fs available, and has the "bonus"
> that you'll get a nice SIGPIPE crash if you accidentally try to write
> anything to it.

(that's an interesting idea. i'll have to think about that...)

> Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[enh]

On Mon, Mar 11, 2024 at 1:09 PM Skyler Ferrante (RIT Student)
<sjf5462@rit.edu> wrote:
>
> Yup, I agree. My confusion was from an incorrect assumption that
> non-suid / suid programs would be handled the same way. I knew that
> strace wouldn't keep it setuid by I didn't realize glibc only checked
> closed fds for suid programs (which makes sense, this doesn't matter
> for non-privileged programs).
>
> > application erroneously closes fd 2
>
> And hopefully no program does that, and if it does, that's their fault :)

programs get confused about fds and close the wrong ones all the time.
the fd equivalent of a malloc() double-free especially. bionic has a
fairly general protection against this class of error:
https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md

(fork() children do it on purpose all the time too :-) )

> Skyler

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Laurent Bercot]

>(Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
>interesting idea... :)

  Please don't. close(0); if (open("something", O_RDONLY)) fail();
(only when single-threaded, obviously) is a valid pattern, and uses
one fewer descriptor than fd=open("something", O_RDONLY); dup2(fd, 0);

  Also, running with stdin or stdout closed (or even stderr but what kind
of monster would do that) is fine for a process that is internal to a
project, that isn't user-facing; it's on the project to know the fd
states of its various components. Private APIs should not have the same
constraints as public ones.

  (And yes, for user-facing programs, i.e. 99% of them, it is a good
idea to always sanitize around 0, 1 and 2.)

--
  Laurent

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Thorsten Glaser]

Rich Felker dixit:

>POSIX explicitly permits the implementation to open these fds if they
>started closed in suid execs, and IIRC indicates as a future direction

AFAIR, POSIX recently clarified that when a utility isn’t invoked
with fd#0, #1 and #2 open and suitable, the caller’s behaviour is
nōn-conforming, and so the callee can probably do what it wants in
that case as it’s left POSIX land.

bye,
//mirabilos
-- 
“It is inappropriate to require that a time represented as
 seconds since the Epoch precisely represent the number of
 seconds between the referenced time and the Epoch.”
	-- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Gabriel Ravier]

On 3/11/24 19:47, Rich Felker wrote:
> On Mon, Mar 11, 2024 at 11:30:04AM -0400, Skyler Ferrante (RIT Student) wrote:
>> Hmm, maybe I'm missing something, but it seems you can close(fd) for
>> the standard fds and then call execve, and the new process image will
>> have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
>> This seems to affect shadow-utils and other setuid/setgid binaries.
>>
>> Here is a repo I built for testing,
>> https://github.com/skyler-ferrante/fd_omission/. What is the correct
>> glibc behavior? Am I misunderstanding something?
> As Florian noted, you're missing that strace cannot invoke it suid.
> POSIX explicitly permits the implementation to open these fds if they
> started closed in suid execs, and IIRC indicates as a future direction
> that it might be permitted for all execs. We do the same in musl in
> the suid case. So really the only way that "writing attacker
> controlled prefix strings to fd 2" becomes an issue is if the
> application erroneously closes fd 2 and lets something else get opened
> on it.
>
> (Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
> interesting idea... :)
>
> Rich


Doing this would break many programs, such as:
- most of coreutils, e.g. programs like ls, cat or head, since they 
always `close` their input and output descriptors (when they've written 
or read something) to make sure to diagnose all errors
- grep
- xargs
- find
- strace, which (using the half-closed self-pipe trick mentioned earlier 
in this thread to avoid reusing them later btw) closes the standard 
descriptors, to avoid changing the behavior of programs calling it if 
e.g. its input is a pipe (where if it left the fds open that'd mean the 
writer would get SIGPIPE later than if the program was ran without strace)
- tcsh, which deliberately does `close(n)` with `n < 3` to make it so 
all the standard FDs point to `/dev/null`
- troff and groff (and thus man)
- git
- many more... I have found these by simply stracing random programs as 
found on my system with `ls /bin/ | shuf -n1`

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Tue, Mar 12, 2024 at 12:18:24AM +0000, Gabriel Ravier wrote:
> On 3/11/24 19:47, Rich Felker wrote:
> >On Mon, Mar 11, 2024 at 11:30:04AM -0400, Skyler Ferrante (RIT Student) wrote:
> >>Hmm, maybe I'm missing something, but it seems you can close(fd) for
> >>the standard fds and then call execve, and the new process image will
> >>have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> >>This seems to affect shadow-utils and other setuid/setgid binaries.
> >>
> >>Here is a repo I built for testing,
> >>https://github.com/skyler-ferrante/fd_omission/. What is the correct
> >>glibc behavior? Am I misunderstanding something?
> >As Florian noted, you're missing that strace cannot invoke it suid.
> >POSIX explicitly permits the implementation to open these fds if they
> >started closed in suid execs, and IIRC indicates as a future direction
> >that it might be permitted for all execs. We do the same in musl in
> >the suid case. So really the only way that "writing attacker
> >controlled prefix strings to fd 2" becomes an issue is if the
> >application erroneously closes fd 2 and lets something else get opened
> >on it.
> >
> >(Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
> >interesting idea... :)
> 
> Doing this would break many programs, such as:
> - most of coreutils, e.g. programs like ls, cat or head, since they
> always `close` their input and output descriptors (when they've
> written or read something) to make sure to diagnose all errors
> - grep
> - xargs
> - find

This makes it so they can malfunction during exit when it
flushes/closes the corresponding stdio FILEs. If nothing else has been
opened in the mean time, under typical implementations it should be
safe, but I think per 2.5.1 Interaction of File Descriptors and
Standard I/O Streams:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05_01

it's undefined.

The safe way to do what they want is to dup the fd they want to
close-and-check-for-errors, open /dev/null, dup2 that over the
original fd, then close the first dup.

Or, don't exit()/return-from-main, but instead _exit, so there's no
subsequent access to the FILE.

> - strace, which (using the half-closed self-pipe trick mentioned
> earlier in this thread to avoid reusing them later btw) closes the
> standard descriptors, to avoid changing the behavior of programs
> calling it if e.g. its input is a pipe (where if it left the fds
> open that'd mean the writer would get SIGPIPE later than if the
> program was ran without strace)
> - tcsh, which deliberately does `close(n)` with `n < 3` to make it
> so all the standard FDs point to `/dev/null`
> - troff and groff (and thus man)
> - git
> - many more... I have found these by simply stracing random programs
> as found on my system with `ls /bin/ | shuf -n1`

Yes, I'm quite aware it's commonplace, but it would be something nice
to get cleaned up...

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Gabriel Ravier]

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

On 3/12/24 00:43, Rich Felker wrote:
> On Tue, Mar 12, 2024 at 12:18:24AM +0000, Gabriel Ravier wrote:
>> On 3/11/24 19:47, Rich Felker wrote:
>>> On Mon, Mar 11, 2024 at 11:30:04AM -0400, Skyler Ferrante (RIT Student) wrote:
>>>> Hmm, maybe I'm missing something, but it seems you can close(fd) for
>>>> the standard fds and then call execve, and the new process image will
>>>> have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
>>>> This seems to affect shadow-utils and other setuid/setgid binaries.
>>>>
>>>> Here is a repo I built for testing,
>>>> https://github.com/skyler-ferrante/fd_omission/. What is the correct
>>>> glibc behavior? Am I misunderstanding something?
>>> As Florian noted, you're missing that strace cannot invoke it suid.
>>> POSIX explicitly permits the implementation to open these fds if they
>>> started closed in suid execs, and IIRC indicates as a future direction
>>> that it might be permitted for all execs. We do the same in musl in
>>> the suid case. So really the only way that "writing attacker
>>> controlled prefix strings to fd 2" becomes an issue is if the
>>> application erroneously closes fd 2 and lets something else get opened
>>> on it.
>>>
>>> (Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
>>> interesting idea... :)
>> Doing this would break many programs, such as:
>> - most of coreutils, e.g. programs like ls, cat or head, since they
>> always `close` their input and output descriptors (when they've
>> written or read something) to make sure to diagnose all errors
>> - grep
>> - xargs
>> - find
> This makes it so they can malfunction during exit when it
> flushes/closes the corresponding stdio FILEs. If nothing else has been
> opened in the mean time, under typical implementations it should be
> safe, but I think per 2.5.1 Interaction of File Descriptors and
> Standard I/O Streams:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05_01
>
> it's undefined.
>
> The safe way to do what they want is to dup the fd they want to
> close-and-check-for-errors, open /dev/null, dup2 that over the
> original fd, then close the first dup.
>
> Or, don't exit()/return-from-main, but instead _exit, so there's no
> subsequent access to the FILE.


Those applications above (though some of those below appear to do raw 
/close/ calls) all circumvent your objection by calling /fclose/ on the 
standard streams rather than /close/-ing the file descriptors directly, 
which seems legal according to POSIX given otherwise the following quote 
would make no sense:

 > Since after the call to /fclose/( ) any use of stream results in 
undefined behavior, /fclose/( ) should not be used on /stdin/, /stdout/, 
or /stderr/ except immediately before process termination (see XBD 
Section 3.287, on page 73), so as to avoid triggering undefined behavior 
in other standard interfaces that rely on these streams. If there are 
any /atexit/( ) handlers registered by the application, such a call to 
/fclose/() should not occur until the last handler is finishing.
- POSIX, /System Interfaces/, *fclose( )*, *APPLICATION USAGE*

and all of those applications listed above do the /fclose/ at the end of 
their /atexit/ handlers - the wording implies the fact they return from 
that /atexit/ handler when the /fclose/ succeeds is fine too since it's 
done when "the last handler is finishing" (i.e. after all other usage of 
standard interfaces which may use the standard streams)), which seems to 
imply calling /exit/ after /fclose/-ing one of the standard streams 
should be legal.


Furthermore, the description of /close/ also states:

 > Usage of /close/( ) on file descriptors STDIN_FILENO, STDOUT_FILENO, 
or STDERR_FILENO should immediately be followed by an operation to 
reopen these file descriptors. Unexpected behavior will result if any of 
these file descriptors is left in a closed state (for example, an 
[EBADF] error from /perror/( )) or if an unrelated /open/( ) or similar 
call later in the application accidentally allocates a file to one of 
these well-known file descriptors. Furthermore, a /close/( ) followed by 
a reopen operation (e.g., /open/( ), /dup/( ), etc.) is not atomic; 
/dup2/( ) should be used to change standard file descriptors.
- POSIX, /System Interfaces/, *fclose( )*, *APPLICATION USAGE*

which simply points out "unexpected" (which does not appear to mean 
"undefined") behavior along with examples of some of the potential 
consequences and points out replacing those descriptors after the 
/close/ is not atomic and that /dup2/ should be used instead - but 
"should" is not the same word as "must", so this seems to implicitly 
allow not using /dup2/ or even not reopening the file descriptors at all 
- which seems accurate to me, I can't see anything in the standard, 
including in that *Interaction of File Descriptors and Standard I/O 
Streams* section you mentioned earlier, that would result in undefined 
behavior upon /close/-ing the file descriptors before /fclose/, except 
that that section seems to indicate the application must not leave data 
buffered in the stream (which does not seem to be something any of the 
applications I've mentioned before or after this do - though it's not 
like I've engaged in exhaustive program analysis to ensure this is the 
case either, so that's more of an assumption than anything else, but the 
scenarios in which they close these streams (near the beginning or 
termination of the application) seem like they should make it quite 
likely this is not the case).

It seems to me as though further use of a /FILE/ after its file 
descriptor was /close/-ed would simply result in [EBADF] errors 
(...which could obviously also easily lead to issues involving having 
multiple active handles on the same file if someone else was to call 
/open/ afterwards, but that's a separate issue).


>
>> - strace, which (using the half-closed self-pipe trick mentioned
>> earlier in this thread to avoid reusing them later btw) closes the
>> standard descriptors, to avoid changing the behavior of programs
>> calling it if e.g. its input is a pipe (where if it left the fds
>> open that'd mean the writer would get SIGPIPE later than if the
>> program was ran without strace)
>> - tcsh, which deliberately does `close(n)` with `n < 3` to make it
>> so all the standard FDs point to `/dev/null`
>> - troff and groff (and thus man)
>> - git
>> - many more... I have found these by simply stracing random programs
>> as found on my system with `ls /bin/ | shuf -n1`
> Yes, I'm quite aware it's commonplace, but it would be something nice
> to get cleaned up...
>
> Rich

[-- Attachment #2: Type: text/html, Size: 8506 bytes --]

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Florian Weimer]

* Gabriel Ravier:

> Doing this would break many programs, such as:
> - most of coreutils, e.g. programs like ls, cat or head, since they
>   always `close` their input and output descriptors (when they've
>   written or read something) to make sure to diagnose all errors

A slightly better way to do this is to do fflush (stdout) followed by
error checking on close (dup (fileno (stdout))).  We can't do this
implicitly as part of fflush because it potentially breaks legacy
(non-OFD) POSIX file locking, at least not without parsing /proc and
whatnot.  The close system call is how the Linux NFS client reports
ENOSPC errors without performing a costly fsync on the server.  We don't
have a better interface for this.

Thanks,
Florian

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Zack Weinberg]

On Tue, Mar 12, 2024, at 9:54 AM, Florian Weimer wrote:
>> Doing this would break many programs, such as:
>> - most of coreutils, e.g. programs like ls, cat or head, since they
>>   always `close` their input and output descriptors (when they've
>>   written or read something) to make sure to diagnose all errors
>
> A slightly better way to do this is to do fflush (stdout) followed by
> error checking on close (dup (fileno (stdout))).

Does that actually report delayed write errors?  As you have it,
the close() just drops the fd created by the dup(), the OFD is
still referenced by fd 1 and therefore remains open.  I would
expect scrupulously correct and thread-safe code for detecting
write errors on stdout without fd 1 ever becoming invalid to
look something like this:

int stub_stdout = open("/dev/null", O_WRONLY|O_CLOEXEC);
if (stub_stdout < 0)
  perror_exit("/dev/null");

flockfile(stdout);
if (ferror(stdout) || fflush(stdout))
  perror_exit("stdout: write error");

int original_stdout = fcntl(fileno(stdout), F_DUPFD_CLOEXEC, 3);
if (original_stdout < 0)
  perror_exit("dup(stdout)");  // e.g. ENFILE

dup2(stub_stdout, fileno(stdout)); // failure here should be impossible
close(stub_stdout); // ditto

funlockfile(stdout);
if (close(original_stdout) < 0)
  perror_exit("stdout: write error");

... Which is enough of a pain in the neck that I can't blame people
for wanting to just do close(1), especially during process termination.

(I thought Linux had a "swap these two fds" mode for dup3(), which would
simplify the above a bunch and get rid of one of the two places where you
can hit the file descriptor limit, but it's not mentioned in my copy of
the manpage. Did I just dream it?)

zw

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Florian Weimer]

* Zack Weinberg:

> On Tue, Mar 12, 2024, at 9:54 AM, Florian Weimer wrote:
>>> Doing this would break many programs, such as:
>>> - most of coreutils, e.g. programs like ls, cat or head, since they
>>>   always `close` their input and output descriptors (when they've
>>>   written or read something) to make sure to diagnose all errors
>>
>> A slightly better way to do this is to do fflush (stdout) followed by
>> error checking on close (dup (fileno (stdout))).
>
> Does that actually report delayed write errors?  As you have it,
> the close() just drops the fd created by the dup(), the OFD is
> still referenced by fd 1 and therefore remains open.

I don't think the VFS close action is subject to reference counting.
Otherwise the current coreutils error checking wouldn't work because in
many cases, another process retains a reference to the OFD.

Thanks,
Florian

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Tue, Mar 12, 2024 at 03:31:04PM +0100, Florian Weimer wrote:
> * Zack Weinberg:
> 
> > On Tue, Mar 12, 2024, at 9:54 AM, Florian Weimer wrote:
> >>> Doing this would break many programs, such as:
> >>> - most of coreutils, e.g. programs like ls, cat or head, since they
> >>>   always `close` their input and output descriptors (when they've
> >>>   written or read something) to make sure to diagnose all errors
> >>
> >> A slightly better way to do this is to do fflush (stdout) followed by
> >> error checking on close (dup (fileno (stdout))).
> >
> > Does that actually report delayed write errors?  As you have it,
> > the close() just drops the fd created by the dup(), the OFD is
> > still referenced by fd 1 and therefore remains open.
> 
> I don't think the VFS close action is subject to reference counting.
> Otherwise the current coreutils error checking wouldn't work because in
> many cases, another process retains a reference to the OFD.

It is. close only reports errors if it's the last fd referring to the
ofd. It's an incredibly stupid design choice by NFS that mismatches
expected fd behavior. This is why my alternate proposal for doing it
used dup2 to remove the original reference on fd<3 without closing it,
so that the close of the dup would have a chance to be the last close.
But indeed none of these ways help if some other process still has a
reference.

The right thing to do here IMO has always been to ignore this and let
people who configure their NFS setups not to synchronously report
errors deal with the resulting data loss. (And on a deeper level, the
right thing is not to use NFS, print out the NFS source code and burn
it, etc. :) But if folks insist on trying to handle it more
gracefully, I'm not stopping them.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Tue, Mar 12, 2024 at 03:23:32AM +0000, Gabriel Ravier wrote:
> On 3/12/24 00:43, Rich Felker wrote:
> >On Tue, Mar 12, 2024 at 12:18:24AM +0000, Gabriel Ravier wrote:
> >>On 3/11/24 19:47, Rich Felker wrote:
> >>>On Mon, Mar 11, 2024 at 11:30:04AM -0400, Skyler Ferrante (RIT Student) wrote:
> >>>>Hmm, maybe I'm missing something, but it seems you can close(fd) for
> >>>>the standard fds and then call execve, and the new process image will
> >>>>have no fd 0,1,2. I've tried this on a default Ubuntu 22.04 system.
> >>>>This seems to affect shadow-utils and other setuid/setgid binaries.
> >>>>
> >>>>Here is a repo I built for testing,
> >>>>https://github.com/skyler-ferrante/fd_omission/. What is the correct
> >>>>glibc behavior? Am I misunderstanding something?
> >>>As Florian noted, you're missing that strace cannot invoke it suid.
> >>>POSIX explicitly permits the implementation to open these fds if they
> >>>started closed in suid execs, and IIRC indicates as a future direction
> >>>that it might be permitted for all execs. We do the same in musl in
> >>>the suid case. So really the only way that "writing attacker
> >>>controlled prefix strings to fd 2" becomes an issue is if the
> >>>application erroneously closes fd 2 and lets something else get opened
> >>>on it.
> >>>
> >>>(Aside: making _FORTIFY_SOURCE>1 trap close(n) with n<3 would be an
> >>>interesting idea... :)
> >>Doing this would break many programs, such as:
> >>- most of coreutils, e.g. programs like ls, cat or head, since they
> >>always `close` their input and output descriptors (when they've
> >>written or read something) to make sure to diagnose all errors
> >>- grep
> >>- xargs
> >>- find
> >This makes it so they can malfunction during exit when it
> >flushes/closes the corresponding stdio FILEs. If nothing else has been
> >opened in the mean time, under typical implementations it should be
> >safe, but I think per 2.5.1 Interaction of File Descriptors and
> >Standard I/O Streams:
> >
> >https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05_01
> >
> >it's undefined.
> >
> >The safe way to do what they want is to dup the fd they want to
> >close-and-check-for-errors, open /dev/null, dup2 that over the
> >original fd, then close the first dup.
> >
> >Or, don't exit()/return-from-main, but instead _exit, so there's no
> >subsequent access to the FILE.
> 
> 
> Those applications above (though some of those below appear to do
> raw /close/ calls) all circumvent your objection by calling /fclose/
> on the standard streams rather than /close/-ing the file descriptors
> directly, which seems legal according to POSIX given otherwise the
> following quote would make no sense:

OK, in that case, _FORTIFY_SOURCE>1 trapping close(n) for n<3 would
not affect them, since they're calling fclose not close...

None of this is particularly intended as a serious proposal, but it
could be interesting to experiment with and catch programs with
dubious behavior that might be a bug.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Zack Weinberg]

On Tue, Mar 12, 2024, at 10:42 AM, Rich Felker wrote:
> On Tue, Mar 12, 2024 at 03:31:04PM +0100, Florian Weimer wrote:
>> * Zack Weinberg:
>> 
>> > On Tue, Mar 12, 2024, at 9:54 AM, Florian Weimer wrote:
>> >>> Doing this would break many programs, such as:
>> >>> - most of coreutils, e.g. programs like ls, cat or head, since they
>> >>>   always `close` their input and output descriptors (when they've
>> >>>   written or read something) to make sure to diagnose all errors
>> >>
>> >> A slightly better way to do this is to do fflush (stdout) followed by
>> >> error checking on close (dup (fileno (stdout))).
>> >
>> > Does that actually report delayed write errors?  As you have it,
>> > the close() just drops the fd created by the dup(), the OFD is
>> > still referenced by fd 1 and therefore remains open.
>> 
>> I don't think the VFS close action is subject to reference counting.
>> Otherwise the current coreutils error checking wouldn't work because in
>> many cases, another process retains a reference to the OFD.
>
> It is. close only reports errors if it's the last fd referring to the
> ofd. It's an incredibly stupid design choice by NFS that mismatches
> expected fd behavior. 

I do fully agree that this is a design error in NFS and in close(2) more generally [like all destructors, it should be _impossible_ for close to fail], but there is no realistic prospect of changing it, and I've been burned a few times by programs that didn't notice delayed write errors. The risk of missing an error because another process still has a ref to the OFD is real, but it comes up rarely enough that I don't think it should deter us from trying to get the "only one process has this file open" case right. (Think shell `>` redirection.)

Also, I have written programs that expected fds 0 and/or 1 to be pipes, and closed them well before exiting as a cheap way to send one-shot events to the process on the other end of the pipe. I think that's a legitimate way to use inherited fds, and it would be significantly more difficult to do in some contexts if you had to use a fd > 2 to do it (e.g. the process on the other end used popen() to start things).

If there's something we can do to make it easier for programmers to do the Right Thing with closing standard fds, IMHO we should. For stdio, we perfectly well _could_ special case fclose(), when the fileno is 0/1/2, to do the dup/close dance and leave both the FILE and the fd in a valid, stubbed state. (For stdin, open("/dev/null", O_RDONLY) is clearly an appropriate stub semantic; for stdout and stderr I am unsure whether "discard all writes silently" or "fail all writes" would be better.) I don't think it's safe to make close() special case 0/1/2, though—not least because it's supposed to be atomic and async signal safe. And we should maybe give some thought to runtimes for languages other than C, which probably have their own buffering wrappers for fds 0/1/2 and might care about cross-language interop.

zw

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Rich Felker]

On Tue, Mar 12, 2024 at 03:25:31PM -0400, Zack Weinberg wrote:
> 
> 
> On Tue, Mar 12, 2024, at 10:42 AM, Rich Felker wrote:
> > On Tue, Mar 12, 2024 at 03:31:04PM +0100, Florian Weimer wrote:
> >> * Zack Weinberg:
> >> 
> >> > On Tue, Mar 12, 2024, at 9:54 AM, Florian Weimer wrote:
> >> >>> Doing this would break many programs, such as:
> >> >>> - most of coreutils, e.g. programs like ls, cat or head, since they
> >> >>>   always `close` their input and output descriptors (when they've
> >> >>>   written or read something) to make sure to diagnose all errors
> >> >>
> >> >> A slightly better way to do this is to do fflush (stdout) followed by
> >> >> error checking on close (dup (fileno (stdout))).
> >> >
> >> > Does that actually report delayed write errors?  As you have it,
> >> > the close() just drops the fd created by the dup(), the OFD is
> >> > still referenced by fd 1 and therefore remains open.
> >> 
> >> I don't think the VFS close action is subject to reference counting.
> >> Otherwise the current coreutils error checking wouldn't work because in
> >> many cases, another process retains a reference to the OFD.
> >
> > It is. close only reports errors if it's the last fd referring to the
> > ofd. It's an incredibly stupid design choice by NFS that mismatches
> > expected fd behavior. 
> 
> I do fully agree that this is a design error in NFS and in close(2)
> more generally [like all destructors, it should be _impossible_ for
> close to fail], but there is no realistic prospect of changing it,
> and I've been burned a few times by programs that didn't notice
> delayed write errors. The risk of missing an error because another
> process still has a ref to the OFD is real, but it comes up rarely
> enough that I don't think it should deter us from trying to get the
> "only one process has this file open" case right. (Think shell `>`
> redirection.)
> 
> Also, I have written programs that expected fds 0 and/or 1 to be
> pipes, and closed them well before exiting as a cheap way to send
> one-shot events to the process on the other end of the pipe. I think
> that's a legitimate way to use inherited fds, and it would be
> significantly more difficult to do in some contexts if you had to
> use a fd > 2 to do it (e.g. the process on the other end used
> popen() to start things).

There's a perfectly safe way to do that: you dup2 something else over
fd 0/1/2 as needed, rather than closing it.

> If there's something we can do to make it easier for programmers to
> do the Right Thing with closing standard fds, IMHO we should. For
> stdio, we perfectly well _could_ special case fclose(), when the
> fileno is 0/1/2, to do the dup/close dance and leave both the FILE
> and the fd in a valid, stubbed state. (For stdin, open("/dev/null",
> O_RDONLY) is clearly an appropriate stub semantic; for stdout and
> stderr I am unsure whether "discard all writes silently" or "fail
> all writes" would be better.)

I don't think that's at all conforming. fclose is specified to close
the fd too, and while that's generally bad behavior, it is the
expected behavior that a program can be depending on. (A hardening
level to trap if a voluntary constraint is violated is a lot different
than making a standard function silently do the wrong thing because
something else would be "better" to do.)

FWIW, accessing any of the standard FILEs after fclose on them (e.g.
calling printf after fclose(stdout) or getchar() after fclose(stdin))
is UB, and would potentially be worth trapping too. Zero-overhead
trapping could probably be arranged just by fiddling with the buffer
pointers.

> I don't think it's safe to make
> close() special case 0/1/2, though—not least because it's supposed
> to be atomic and async signal safe. And we should maybe give some
> thought to runtimes for languages other than C, which probably have
> their own buffering wrappers for fds 0/1/2 and might care about
> cross-language interop.

Indeed, the hypothetical thing was not intended as a change of
semantics, just as a hardening mode to catch programming errors.

Rich

Re: [musl] Re: Tweaking the program name for <err.h> functions

[Florian Weimer]

* Zack Weinberg:

> I do fully agree that this is a design error in NFS and in close(2)
> more generally [like all destructors, it should be _impossible_ for
> close to fail], but there is no realistic prospect of changing it, and
> I've been burned a few times by programs that didn't notice delayed
> write errors.

There is fsync to avoid delayed write errors.  Historically, it's been
very bad for performance.  What coreutils et al. aim to do is to deal
with non-catastrophic failures (mainly out of space errors) without
incurring the fsync overhead.

Thanks,
Florian