[PATCH-V4] xserver: Optional backtrace handler

Barry Scott barry.scott at onelan.co.uk
Thu Oct 6 05:19:35 PDT 2011


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;
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111006/e7771d06/attachment.html>


More information about the xorg-devel mailing list