Date: Sun, 6 Dec 2009 05:02:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Stefan Farfeleder <stefanf@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r200120 - head/usr.bin/make Message-ID: <20091206034620.C26414@delplex.bde.org> In-Reply-To: <200912051312.nB5DC4kQ088533@svn.freebsd.org> References: <200912051312.nB5DC4kQ088533@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 5 Dec 2009, Stefan Farfeleder wrote:
> Log:
> Add a missing space to the error message when execvp() failed.
>
> Modified:
> head/usr.bin/make/proc.c
>
> Modified: head/usr.bin/make/proc.c
> ==============================================================================
> --- head/usr.bin/make/proc.c Sat Dec 5 12:51:51 2009 (r200119)
> +++ head/usr.bin/make/proc.c Sat Dec 5 13:12:04 2009 (r200120)
> @@ -116,7 +116,7 @@ Proc_Exec(const ProcStuff *ps)
> execvp(ps->argv[0], ps->argv);
>
Someone broke the formatting by adding this extra blank line.
> write(STDERR_FILENO, ps->argv[0], strlen(ps->argv[0]));
> - write(STDERR_FILENO, ":", 1);
> + write(STDERR_FILENO, ": ", 2);
> write(STDERR_FILENO, strerror(errno), strlen(strerror(errno)));
> write(STDERR_FILENO, "\n", 1);
> } else {
>
It would be nice if the err() family were not so broken in design and
implementation so as to be unusable in signal handlers and other delicate
contexts. Then it could just be used here (here we are in vfork context
after a failed exec). The err() family uses stdio, so it might flush
output stream(s), which would be bad as documented in vfork(2), and
probably other things that might be done by stdio would be even worse
in vfork() context. (The restructuring that added the above style bug
also moved the execvp() far away from the vfork(), so it is unclear
even that most things cannot be done in code near here.)
Brokenness in the err() family includes not really documenting where the
output goes. stdio is not mentioned. [stdio] streams are mentioned,
but only in connection with the FreeBSD extension of err_set_file()
(without this extension, the err() family could and should always write
to the STDERR_FILENO non-stream using code like the above). Output is
documented to go "on the standard error output", which should mean to
STDERR_FILENO, but output actually goes to err_file (default stderr)
using fprintf().
Corresponding brokenness in perror() is to be expected, since perror()
is declared in <stdio.h>, but perror() is documented to only write to
"the standard output file descriptor", and it used to do this very
carefully (using local variables), so it was usuable in signal handlers
and other delicate contexts including vfork(). But it never documented
this safeness, and FreeBSD turned its careful code into nonsense:
% RCS file: /home/ncvs/src/lib/libc/stdio/perror.c,v
% Working file: perror.c
% head: 1.8
% ...
% ----------------------------
% revision 1.8
% date: 2002/12/19 09:53:26; author: tjr; state: Exp; lines: +7 -1
% Write the message to stderr, not file descriptor 2, so that perror()
% writes to the correct stream if stderr has been redirected with freopen().
The man page still documents writing to fd 2, and writing to stderr defeats
not using stdio. The log message and man page are missing mention of the
new flushing feature.
% =============================================================================
% Index: perror.c
% ===================================================================
% RCS file: /home/ncvs/src/lib/libc/stdio/perror.c,v
% retrieving revision 1.7
% retrieving revision 1.8
% diff -u -2 -r1.7 -r1.8
% --- perror.c 19 Dec 2002 09:50:10 -0000 1.7
% +++ perror.c 19 Dec 2002 09:53:26 -0000 1.8
% @@ -36,5 +36,5 @@
% #endif /* LIBC_SCCS and not lint */
% #include <sys/cdefs.h>
% -__FBSDID("$FreeBSD: src/lib/libc/stdio/perror.c,v 1.7 2002/12/19 09:50:10 tjr Exp $");
% +__FBSDID("$FreeBSD: src/lib/libc/stdio/perror.c,v 1.8 2002/12/19 09:53:26 tjr Exp $");
%
% #include "namespace.h"
% @@ -47,4 +47,6 @@
% #include <string.h>
% #include "un-namespace.h"
% +#include "libc_private.h"
% +#include "local.h"
%
% void
% @@ -71,4 +73,8 @@
% v->iov_base = "\n";
% v->iov_len = 1;
% - (void)_writev(STDERR_FILENO, iov, (v - iov) + 1);
% + FLOCKFILE(stderr);
Just locking stderr breaks signal-safeness.
% + __sflush(stderr);
Flushing also breaks use in [v]fork() after failed exec. Pre-flushing
like this might be useful, but the err() family doesn't do any. Here
the pre-flush is needed to synchronize with stderr, since the output
here (now pointlessly) uses a direct writev. The err() family uses
stderr, so it doesn't need to do anything to synchronize with stderr,
but it could do more to sync with other streams and it should do more
to ensure that its output actually goes out in case stderr is unbuffered
(especially since its use of stdio is undocumented, its callers cannot
know what synchronization is necessary or possible).
% + (void)_writev(stderr->_file, iov, (v - iov) + 1);
Maybe stderr->_file can be type-stable or something, so that it can be
accessed not too unsafely without locking in signal handlers. The change
should have been limited to something like that.
% + stderr->_flags &= ~__SOFF;
Another state change which must be avoided in delicate contexts.
% + FUNLOCKFILE(stderr);
The locking is hopefully idempotent, so it is safe in vfork context.
% }
Both Standard C and POSIX specify perror() to be perfectly broken.
They specify it to write to the "standard error stream" (why not just
stderr?). Standard C doesn't have STDERR_FILENO, so it cannot do the
right thing here except by not really specifying where the output goes.
Neither specifies any special synchronization. POSIX (and newer C99?)
also specifies that perror() shall not change the orientation of the
standard error stream.
A non-broken err() would look like the non-broken perror(), except it
would have to snprintf() to a local buffer before writev() and thus it
could no longer support very long messages.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091206034620.C26414>