[PATCH-V4] xserver: Optional backtrace handler

[Jeremy Huddleston]

I thought I already said this, but:

Reviewed-by: Jeremy Huddleston <jeremyhu at apple.com>

Just send the final version with the accumulated tags to keithp for merging and CC the list.

On Oct 6, 2011, at 5:19 AM, Barry Scott wrote:

> Is there anything else I need to do to make this patch acceptable
> for inclusion in Xorg?
> 
> Barry
> 
> 
> On Thursday 29 September 2011 11:21:08 Barry Scott wrote:
> > It is useful to be able to run an external program to analyse
> > a crashed server process. The server will run a user supplied
> > program in a subprocess of the server.
> >
> > The subprocess is created when the server starts up so that all
> > resources needed to create the subprocess are available. It is not
> > always possible to start up a process from within a crashed server.
> > For example if the malloc heap is corrupt fork will not work.
> >
> > If the server crashes a "crash" message is sent to the handler on
> > its stdin. The handler can then respond in whatever way it wishes.
> > If the handler wants the server to continue, return from
> > xorg_backtrace, it must write a line to its stdout.
> >
> > When then server exits normally the handler is sent the "exit"
> > message just before the server exits.
> >
> > Signed-off-by: Barry Scott <barry.scott at onelan.co.uk>
> > Reviewed-by: Simon Farnsworth <simon.farnsworth at onelan.co.uk>
> > ---
> > change from V3:
> > * close backtrace_handler_stdin in child process
> >
> > change from V2:
> > * restore trailing NL in main.c
> > * beef up comments to make it clea how loops are exited
> > when the child process dies
> > * Use _X_HIDDEN for new symbols that are not to be exported
> >
> > change from V1:
> >
> > * check for EAGAIN and EINTR on calls to read(), write() and waitpid()
> > * put -backtrace into lexical position in the command parser and usage
> > * fix coding style white space and condition ordering
> > * update man page with more details
> > * call xorg_cleanup_backtrace() from FatalError()
> >
> > dix/main.c | 5 ++
> > include/os.h | 3 +
> > man/Xserver.man | 20 +++++++++
> > os/backtrace.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > os/log.c | 3 +
> > os/utils.c | 8 ++++
> > 6 files changed, 159 insertions(+), 3 deletions(-)
> >
> > diff --git a/dix/main.c b/dix/main.c
> > index 16575ce..3326e78 100644
> > --- a/dix/main.c
> > +++ b/dix/main.c
> > @@ -147,6 +147,8 @@ int main(int argc, char *argv[], char *envp[])
> >
> > ProcessCommandLine(argc, argv);
> >
> > + xorg_init_backtrace();
> > +
> > alwaysCheckForInput[0] = 0;
> > alwaysCheckForInput[1] = 1;
> > while(1)
> > @@ -354,6 +356,9 @@ int main(int argc, char *argv[], char *envp[])
> > free(ConnectionInfo);
> > ConnectionInfo = NULL;
> > }
> > +
> > + xorg_cleanup_backtrace();
> > +
> > return 0;
> > }
> >
> > diff --git a/include/os.h b/include/os.h
> > index a553f57..00336e5 100644
> > --- a/include/os.h
> > +++ b/include/os.h
> > @@ -541,6 +541,9 @@ extern _X_EXPORT void ErrorF(const char *f, ...) _X_ATTRIBUTE_PRINTF(1,2);
> > extern _X_EXPORT void Error(const char *str);
> > extern _X_EXPORT void LogPrintMarkers(void);
> >
> > +extern _X_HIDDEN void xorg_init_backtrace(void);
> > extern _X_EXPORT void xorg_backtrace(void);
> > +extern _X_HIDDEN void xorg_cleanup_backtrace(void);
> > +extern _X_HIDDEN const char *BacktraceHandlerPath;
> >
> > #endif /* OS_H */
> > diff --git a/man/Xserver.man b/man/Xserver.man
> > index f743912..9def022 100644
> > --- a/man/Xserver.man
> > +++ b/man/Xserver.man
> > @@ -100,6 +100,9 @@ specifies a file which contains a collection of authorization records used
> > to authenticate access. See also the \fIxdm\fP(1) and
> > \fIXsecurity\fP(__miscmansuffix__) manual pages.
> > .TP 8
> > +.B \-backtrace \fIbacktrace-handler-path\fP
> > +specifies the path to the optional backtrace handler program. See \fBBACKTRACE HANDLER\fP below.
> > +.TP 8
> > .B \-br
> > sets the default root window to solid black instead of the standard root weave
> > pattern. This is the default unless -retro or -wr is specified.
> > @@ -530,6 +533,23 @@ the following font path:
> > /usr/share/fonts/default/ghostscript
> > .fi
> >
> > +.SH BACKTRACE HANDLER
> > +
> > +The optional backtrace handler program will be run in a subprocess when
> > +the server starts. The handler is invoked with argv[1] set to the PID
> > +of the server.
> > +.PP
> > +The handler is expected to read a line from stdin that will tell the
> > +handler what has happened to the server.
> > +.PP
> > +When the server is about to exit normally "exit\\n" is sent to the handler.
> > +.PP
> > +If the server crashes "crash\n" is sent to the handler and the server will
> > +wait for a line to be written out on the handler's stdout. This causes
> > +the server to return from the xorg_backtrace function. Typically after calling
> > +xorg_backtrace additional error messages are logged and the server shuts down.
> > +If you do not want the server to continue then kill it.
> > +
> > .SH FILES
> > .TP 30
> > .I /etc/X\fBn\fP.hosts
> > diff --git a/os/backtrace.c b/os/backtrace.c
> > index 7ca6dab..bcdcbf6 100644
> > --- a/os/backtrace.c
> > +++ b/os/backtrace.c
> > @@ -28,6 +28,11 @@
> > #include "os.h"
> > #include "misc.h"
> >
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <sys/types.h>
> > +#include <sys/wait.h>
> > +
> > #ifdef HAVE_BACKTRACE
> > #ifndef _GNU_SOURCE
> > #define _GNU_SOURCE
> > @@ -35,7 +40,7 @@
> > #include <dlfcn.h>
> > #include <execinfo.h>
> >
> > -void xorg_backtrace(void)
> > +void xorg_builtin_backtrace(void)
> > {
> > void *array[64];
> > const char *mod;
> > @@ -180,7 +185,7 @@ static int xorg_backtrace_pstack(void) {
> >
> > # if defined(HAVE_PSTACK) || defined(HAVE_WALKCONTEXT)
> >
> > -void xorg_backtrace(void) {
> > +void xorg_builtin_backtrace(void) {
> >
> > ErrorF("\nBacktrace:\n");
> >
> > @@ -207,7 +212,119 @@ void xorg_backtrace(void) {
> > # else
> >
> > /* Default fallback if we can't find any way to get a backtrace */
> > -void xorg_backtrace(void) { return; }
> > +void xorg_builtin_backtrace(void) { return; }
> >
> > # endif
> > #endif
> > +
> > +const char *BacktraceHandlerPath = NULL;
> > +
> > +static int backtrace_handler_stdin;
> > +static int backtrace_handler_stdout;
> > +static pid_t child_pid;
> > +
> > +static const char cmd_crash[] = "crash\n";
> > +static const char cmd_exit[] = "exit\n";
> > +
> > +static void report_init_error(const char *msg) {
> > + /* prevent any further use of the handler */
> > + BacktraceHandlerPath = NULL;
> > + FatalError("%s - errno %d", msg, errno);
> > +}
> > +
> > +void xorg_init_backtrace(void) {
> > + char pid_buffer[32];
> > + int pipe_fds[2];
> > + int child_stdin;
> > + int child_stdout;
> > +
> > + if (!BacktraceHandlerPath)
> > + return;
> > +
> > + if (pipe(pipe_fds) != 0)
> > + report_init_error("failed to create pipe");
> > + backtrace_handler_stdin = pipe_fds[1];
> > + child_stdin = pipe_fds[0];
> > +
> > + if (pipe(pipe_fds) != 0)
> > + report_init_error("failed to create pipe");
> > + backtrace_handler_stdout = pipe_fds[0];
> > + child_stdout = pipe_fds[1];
> > +
> > + /* server pid will be passed in argv[1] */
> > + snprintf( pid_buffer, sizeof(pid_buffer), "%d", getpid());
> > +
> > + child_pid = fork();
> > + if (child_pid < 0) {
> > + report_init_error("failed to fork");
> > + } else {
> > + if (child_pid == 0) {
> > + /* setup stdin, stdout to point to the pipes */
> > + dup2(child_stdin, STDIN_FILENO);
> > + dup2(child_stdout, STDOUT_FILENO);
> > +
> > + /* close all other fds to the pipes */
> > + close(child_stdin);
> > + close(child_stdout);
> > + close(backtrace_handler_stdin);
> > + close(backtrace_handler_stdout);
> > +
> > + execl(BacktraceHandlerPath, BacktraceHandlerPath, pid_buffer, NULL);
> > + exit(1);
> > + } else {
> > + if (close(child_stdin) != 0)
> > + report_init_error("failed to close pipe fd");
> > + if (close(child_stdout) != 0)
> > + report_init_error("failed to close pipe fd");
> > + }
> > + }
> > +}
> > +
> > +void xorg_backtrace(void) {
> > + if (BacktraceHandlerPath) {
> > + int status;
> > +
> > + /* check that the handler process is still running */
> > + if (waitpid(child_pid, &status, WNOHANG) == 0) {
> > + char buffer[1];
> > +
> > + /* tell backtrace handler xorg has crashed */
> > + while (write(backtrace_handler_stdin, cmd_crash, sizeof(cmd_crash ) - 1) < 0)
> > + if (errno != EAGAIN && errno != EINTR)
> > + break;
> > +
> > + /* exit to caller after backtrace handler writes anything to stdout */
> > + while (read(backtrace_handler_stdout, buffer, sizeof(buffer)) < 0)
> > + if (errno != EAGAIN && errno != EINTR)
> > + break;
> > +
> > + } else {
> > + /* handler process has quit? run builtin handler */
> > + xorg_builtin_backtrace();
> > + }
> > + } else {
> > + xorg_builtin_backtrace();
> > + }
> > +}
> > +
> > +void xorg_cleanup_backtrace(void) {
> > + if (BacktraceHandlerPath) {
> > + int status;
> > +
> > + BacktraceHandlerPath = NULL;
> > +
> > + /* tell backtrace handler xorg is about to exit cleanly
> > + EPIPE is returned if the child process closes the pipe
> > + (which happens when the process exits) and the loop will exit */
> > + while (write(backtrace_handler_stdin, cmd_exit, sizeof(cmd_exit ) - 1) < 0)
> > + if (errno != EAGAIN && errno != EINTR)
> > + break;
> > +
> > + /* If the child has already exited waitpid return with ECHILD
> > + and the loop will exit */
> > + while (waitpid(child_pid, &status, 0) < 0)
> > + if (errno != EINTR)
> > + break;
> > +
> > + }
> > +}
> > diff --git a/os/log.c b/os/log.c
> > index f519762..7b3409a 100644
> > --- a/os/log.c
> > +++ b/os/log.c
> > @@ -523,6 +523,9 @@ FatalError(const char *f, ...)
> > va_list args;
> > static Bool beenhere = FALSE;
> >
> > + /* make sure any backtrace handler process is told to exit */
> > + xorg_cleanup_backtrace();
> > +
> > if (beenhere)
> > ErrorF("\nFatalError re-entered, aborting\n");
> > else
> > diff --git a/os/utils.c b/os/utils.c
> > index 36cb46f..78c39e6 100644
> > --- a/os/utils.c
> > +++ b/os/utils.c
> > @@ -470,6 +470,7 @@ void UseMsg(void)
> > ErrorF("-ac disable access control restrictions\n");
> > ErrorF("-audit int set audit trail level\n");
> > ErrorF("-auth file select authorization file\n");
> > + ErrorF("-backtrace string path of program to run to handle a server crash\n");
> > ErrorF("-br create root window with black background\n");
> > ErrorF("+bs enable any backing store support\n");
> > ErrorF("-bs disable any backing store support\n");
> > @@ -616,6 +617,13 @@ ProcessCommandLine(int argc, char *argv[])
> > else
> > UseMsg();
> > }
> > + else if ( strcmp( argv[i], "-backtrace") == 0)
> > + {
> > +	 if(++i < argc)
> > + BacktraceHandlerPath = argv[i];
> > +	 else
> > +	 UseMsg();
> > + }
> > else if ( strcmp( argv[i], "-br") == 0) ; /* default */
> > else if ( strcmp( argv[i], "+bs") == 0)
> > enableBackingStore = TRUE;
> >
> 

---
Jeremy Huddleston

Rebuild Sudan
 - Board of Directors
 - http://www.rebuildsudan.org

Berkeley Foundation for Opportunities in Information Technology
 - Advisory Board
 - http://www.bfoit.org