[PATCH] Provide a simple backtrace in the samba AD DC also

Jeremy Allison jra at samba.org
Tue Apr 10 20:59:43 UTC 2018


On Tue, Apr 10, 2018 at 05:00:32PM +1200, Andrew Bartlett via samba-technical wrote:
> smbd has for a long time been able to produce a simple internal
> backtrace.
> 
> This patch allows 'samba' to do the same, by bringing that code into
> lib/util.  An un-used IRIX mode, lost since the move to waf, was
> removed to allow this to happen.  (That mode used become_root()). 
> 
> This is important because gdb_backtrace as a panic action is not
> compatible with gitlab CI, so without this we have no way to tell where
> the fault is. 
> 
> The cstyle.py failures are all in the moved code:
> 
> ad84ecfd836 s3-lib: Remove support for libexc for IRIX backtraces
> 2469ab654a3 lib/util: Log PANIC before calling pacic action just like
> s3
> 212ae4db4bb lib/util: Move log_stack_trace() to common code
>   :lib/util/fault.c:
>       178  Comment line does not have * aligned with top
>       179  Comment line does not have * aligned with top
>       180  Comment line does not have * aligned with top
>       181  Garbage before * in comment line
>       181  Comment is 3+ lines but is not formatted as block comment
>       197  Preprocessor statement in function body
>       201  Comment is 3+ lines but is not formatted as block comment
>       227  Space before close parenthesis
>       234  Space after cast operator (or inline if/while body)
>       242  Space after cast operator (or inline if/while body)
>       252  Preprocessor statement in function body
>       273  Preprocessor statement in function body
>       275  Preprocessor statement in function body
> 19dba766ad6 lib/util: Call log_stack_trace() in smb_panic_default()
> 
> Please review and push,

LGTM - RB+ and pushed with the minor change of DEBUG(0, -> DBG_ERR(
to match updated coding standards.

Cheers,

Jeremy

> Andrew Bartlett
> https://samba.org/~abartlet/
> Authentication Developer, Samba Team         https://samba.org
> Samba Development and Support, Catalyst IT   
> https://catalyst.net.nz/services/samba
> 
> 
> 

> From ad84ecfd836f07bbfe050fe203d352490c85decc Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 10 Apr 2018 15:54:10 +1200
> Subject: [PATCH 1/4] s3-lib: Remove support for libexc for IRIX backtraces
> 
> IRIX is long dead, and this code needs become_root() which is not in
> the top level code.
> 
> Additionally, the check for libexc never made it into waf, so this
> has been dead code since Samba 4.1.
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  source3/lib/util.c | 42 ------------------------------------------
>  source3/wscript    |  2 +-
>  2 files changed, 1 insertion(+), 43 deletions(-)
> 
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index ae9fe71c974..98c47b3df9f 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -852,10 +852,6 @@ void smb_panic_s3(const char *why)
>  #include <execinfo.h>
>  #endif
>  
> -#ifdef HAVE_LIBEXC_H
> -#include <libexc.h>
> -#endif
> -
>  void log_stack_trace(void)
>  {
>  #ifdef HAVE_LIBUNWIND
> @@ -934,44 +930,6 @@ libunwind_failed:
>  		/* Leak the backtrace_strings, rather than risk what free() might do */
>  	}
>  
> -#elif HAVE_LIBEXC
> -
> -	/* The IRIX libexc library provides an API for unwinding the stack. See
> -	 * libexc(3) for details. Apparantly trace_back_stack leaks memory, but
> -	 * since we are about to abort anyway, it hardly matters.
> -	 */
> -
> -#define NAMESIZE 32 /* Arbitrary */
> -
> -	__uint64_t	addrs[BACKTRACE_STACK_SIZE];
> -	char *      	names[BACKTRACE_STACK_SIZE];
> -	char		namebuf[BACKTRACE_STACK_SIZE * NAMESIZE];
> -
> -	int		i;
> -	int		levels;
> -
> -	ZERO_ARRAY(addrs);
> -	ZERO_ARRAY(names);
> -	ZERO_ARRAY(namebuf);
> -
> -	/* We need to be root so we can open our /proc entry to walk
> -	 * our stack. It also helps when we want to dump core.
> -	 */
> -	become_root();
> -
> -	for (i = 0; i < BACKTRACE_STACK_SIZE; i++) {
> -		names[i] = namebuf + (i * NAMESIZE);
> -	}
> -
> -	levels = trace_back_stack(0, addrs, names,
> -			BACKTRACE_STACK_SIZE, NAMESIZE - 1);
> -
> -	DEBUG(0, ("BACKTRACE: %d stack frames:\n", levels));
> -	for (i = 0; i < levels; i++) {
> -		DEBUGADD(0, (" #%d 0x%llx %s\n", i, addrs[i], names[i]));
> -	}
> -#undef NAMESIZE
> -
>  #else
>  	DEBUG(0, ("unable to produce a stack trace on this platform\n"));
>  #endif
> diff --git a/source3/wscript b/source3/wscript
> index 826877d3dfd..8d658b2f53e 100644
> --- a/source3/wscript
> +++ b/source3/wscript
> @@ -96,7 +96,7 @@ def configure(conf):
>      # We crash without vfs_default
>      required_static_modules.extend(TO_LIST('vfs_default'))
>  
> -    conf.CHECK_HEADERS('execinfo.h libexc.h libunwind.h netdb.h')
> +    conf.CHECK_HEADERS('execinfo.h libunwind.h netdb.h')
>      conf.CHECK_HEADERS('linux/falloc.h linux/ioctl.h')
>  
>      conf.CHECK_FUNCS('getcwd fchown chmod fchmod mknod')
> -- 
> 2.11.0
> 
> 
> From 2469ab654a3c39099ec6423acb5927182849a3b1 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 10 Apr 2018 16:06:12 +1200
> Subject: [PATCH 2/4] lib/util: Log PANIC before calling pacic action just like
>  s3
> 
> This is like the changes made in s3 by
> 4fa555980070d78b39711ef21d77628d26055bc2
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/util/fault.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/util/fault.c b/lib/util/fault.c
> index 54d84711742..887c7d6c238 100644
> --- a/lib/util/fault.c
> +++ b/lib/util/fault.c
> @@ -123,6 +123,9 @@ _PUBLIC_ const char *panic_action = NULL;
>  static void smb_panic_default(const char *why) _NORETURN_;
>  static void smb_panic_default(const char *why)
>  {
> +	DEBUG(0,("PANIC (pid %llu): %s\n",
> +		    (unsigned long long)getpid(), why));
> +
>  #if defined(HAVE_PRCTL) && defined(PR_SET_PTRACER)
>  	/*
>  	 * Make sure all children can attach a debugger.
> @@ -148,7 +151,6 @@ static void smb_panic_default(const char *why)
>  					  WEXITSTATUS(result)));
>  		}
>  	}
> -	DEBUG(0,("PANIC: %s\n", why));
>  
>  #ifdef SIGABRT
>  	CatchSignal(SIGABRT, SIG_DFL);
> -- 
> 2.11.0
> 
> 
> From 212ae4db4bb433de5e3a70fcb797c6dc21f4d795 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 10 Apr 2018 16:35:07 +1200
> Subject: [PATCH 3/4] lib/util: Move log_stack_trace() to common code
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/util/fault.c           | 104 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/util/fault.h           |   1 +
>  lib/util/wscript_configure |   1 +
>  source3/include/local.h    |   3 --
>  source3/include/proto.h    |   1 -
>  source3/lib/util.c         |  97 ------------------------------------------
>  source3/wscript            |   2 +-
>  7 files changed, 107 insertions(+), 102 deletions(-)
> 
> diff --git a/lib/util/fault.c b/lib/util/fault.c
> index 887c7d6c238..af06c9484db 100644
> --- a/lib/util/fault.c
> +++ b/lib/util/fault.c
> @@ -3,6 +3,7 @@
>     Critical Fault handling
>     Copyright (C) Andrew Tridgell 1992-1998
>     Copyright (C) Tim Prouty 2009
> +   Copyright (C) James Peach 2006
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -170,3 +171,106 @@ _PUBLIC_ void smb_panic(const char *why)
>  	}
>  	smb_panic_default(why);
>  }
> +
> +
> +
> +/*******************************************************************
> + Print a backtrace of the stack to the debug log. This function
> + DELIBERATELY LEAKS MEMORY. The expectation is that you should
> + exit shortly after calling it.
> +********************************************************************/
> +
> +/* Buffer size to use when printing backtraces */
> +#define BACKTRACE_STACK_SIZE 64
> +
> +
> +#ifdef HAVE_LIBUNWIND_H
> +#include <libunwind.h>
> +#endif
> +
> +#ifdef HAVE_EXECINFO_H
> +#include <execinfo.h>
> +#endif
> +
> +void log_stack_trace(void)
> +{
> +#ifdef HAVE_LIBUNWIND
> +	/* Try to use libunwind before any other technique since on ia64
> +	 * libunwind correctly walks the stack in more circumstances than
> +	 * backtrace.
> +	 */
> +	unw_cursor_t cursor;
> +	unw_context_t uc;
> +	unsigned i = 0;
> +
> +	char procname[256];
> +	unw_word_t ip, sp, off;
> +
> +	procname[sizeof(procname) - 1] = '\0';
> +
> +	if (unw_getcontext(&uc) != 0) {
> +		goto libunwind_failed;
> +	}
> +
> +	if (unw_init_local(&cursor, &uc) != 0) {
> +		goto libunwind_failed;
> +	}
> +
> +	DEBUG(0, ("BACKTRACE:\n"));
> +
> +	do {
> +	    ip = sp = 0;
> +	    unw_get_reg(&cursor, UNW_REG_IP, &ip);
> +	    unw_get_reg(&cursor, UNW_REG_SP, &sp);
> +
> +	    switch (unw_get_proc_name(&cursor,
> +			procname, sizeof(procname) - 1, &off) ) {
> +	    case 0:
> +		    /* Name found. */
> +	    case -UNW_ENOMEM:
> +		    /* Name truncated. */
> +		    DEBUGADD(0, (" #%u %s + %#llx [ip=%#llx] [sp=%#llx]\n",
> +			    i, procname, (long long)off,
> +			    (long long)ip, (long long) sp));
> +		    break;
> +	    default:
> +	    /* case -UNW_ENOINFO: */
> +	    /* case -UNW_EUNSPEC: */
> +		    /* No symbol name found. */
> +		    DEBUGADD(0, (" #%u %s [ip=%#llx] [sp=%#llx]\n",
> +			    i, "<unknown symbol>",
> +			    (long long)ip, (long long) sp));
> +	    }
> +	    ++i;
> +	} while (unw_step(&cursor) > 0);
> +
> +	return;
> +
> +libunwind_failed:
> +	DEBUG(0, ("unable to produce a stack trace with libunwind\n"));
> +
> +#elif HAVE_BACKTRACE_SYMBOLS
> +	void *backtrace_stack[BACKTRACE_STACK_SIZE];
> +	size_t backtrace_size;
> +	char **backtrace_strings;
> +
> +	/* get the backtrace (stack frames) */
> +	backtrace_size = backtrace(backtrace_stack,BACKTRACE_STACK_SIZE);
> +	backtrace_strings = backtrace_symbols(backtrace_stack, backtrace_size);
> +
> +	DEBUG(0, ("BACKTRACE: %lu stack frames:\n",
> +		  (unsigned long)backtrace_size));
> +
> +	if (backtrace_strings) {
> +		int i;
> +
> +		for (i = 0; i < backtrace_size; i++)
> +			DEBUGADD(0, (" #%u %s\n", i, backtrace_strings[i]));
> +
> +		/* Leak the backtrace_strings, rather than risk what free() might do */
> +	}
> +
> +#else
> +	DEBUG(0, ("unable to produce a stack trace on this platform\n"));
> +#endif
> +}
> diff --git a/lib/util/fault.h b/lib/util/fault.h
> index 0ac6cb9ceb5..dfa339b7650 100644
> --- a/lib/util/fault.h
> +++ b/lib/util/fault.h
> @@ -53,5 +53,6 @@ void fault_setup(void);
>  void fault_setup_disable(void);
>  _NORETURN_ void smb_panic(const char *reason);
>  
> +void log_stack_trace(void);
>  
>  #endif /* _SAMBA_FAULT_H_ */
> diff --git a/lib/util/wscript_configure b/lib/util/wscript_configure
> index 8e5a59c8480..2a8dbef699b 100644
> --- a/lib/util/wscript_configure
> +++ b/lib/util/wscript_configure
> @@ -6,6 +6,7 @@ if Options.options.disable_fault_handling:
>  
>  # backtrace could be in libexecinfo or in libc
>  conf.CHECK_FUNCS_IN('backtrace backtrace_symbols', 'execinfo', checklibc=True, headers='execinfo.h')
> +conf.CHECK_HEADERS('execinfo.h libunwind.h')
>  
>  conf.CHECK_STRUCTURE_MEMBER('struct statvfs', 'f_frsize', define='HAVE_FRSIZE', headers='sys/statvfs.h')
>  
> diff --git a/source3/include/local.h b/source3/include/local.h
> index 7f97d4ece9c..c2be1ff3b7f 100644
> --- a/source3/include/local.h
> +++ b/source3/include/local.h
> @@ -172,9 +172,6 @@
>  /* Number in seconds for winbindd to wait for the mutex. Make this 2 * smbd wait time. */
>  #define WINBIND_SERVER_MUTEX_WAIT_TIME (( ((NUM_CLI_AUTH_CONNECT_RETRIES) * ((CLI_AUTH_TIMEOUT)/1000)) + 5)*2)
>  
> -/* Buffer size to use when printing backtraces */
> -#define BACKTRACE_STACK_SIZE 64
> -
>  /* size of listen() backlog in smbd */
>  #define SMBD_LISTEN_BACKLOG 50
>  
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index fa87407ff24..2bc5ab2f532 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -356,7 +356,6 @@ char *gidtoname(gid_t gid);
>  uid_t nametouid(const char *name);
>  gid_t nametogid(const char *name);
>  void smb_panic_s3(const char *why);
> -void log_stack_trace(void);
>  const char *readdirname(DIR *p);
>  bool is_in_path(const char *name, name_compare_entry *namelist, bool case_sensitive);
>  void set_namearray(name_compare_entry **ppname_array, const char *namelist);
> diff --git a/source3/lib/util.c b/source3/lib/util.c
> index 98c47b3df9f..5f786f95d3e 100644
> --- a/source3/lib/util.c
> +++ b/source3/lib/util.c
> @@ -839,103 +839,6 @@ void smb_panic_s3(const char *why)
>  }
>  
>  /*******************************************************************
> - Print a backtrace of the stack to the debug log. This function
> - DELIBERATELY LEAKS MEMORY. The expectation is that you should
> - exit shortly after calling it.
> -********************************************************************/
> -
> -#ifdef HAVE_LIBUNWIND_H
> -#include <libunwind.h>
> -#endif
> -
> -#ifdef HAVE_EXECINFO_H
> -#include <execinfo.h>
> -#endif
> -
> -void log_stack_trace(void)
> -{
> -#ifdef HAVE_LIBUNWIND
> -	/* Try to use libunwind before any other technique since on ia64
> -	 * libunwind correctly walks the stack in more circumstances than
> -	 * backtrace.
> -	 */ 
> -	unw_cursor_t cursor;
> -	unw_context_t uc;
> -	unsigned i = 0;
> -
> -	char procname[256];
> -	unw_word_t ip, sp, off;
> -
> -	procname[sizeof(procname) - 1] = '\0';
> -
> -	if (unw_getcontext(&uc) != 0) {
> -		goto libunwind_failed;
> -	}
> -
> -	if (unw_init_local(&cursor, &uc) != 0) {
> -		goto libunwind_failed;
> -	}
> -
> -	DEBUG(0, ("BACKTRACE:\n"));
> -
> -	do {
> -	    ip = sp = 0;
> -	    unw_get_reg(&cursor, UNW_REG_IP, &ip);
> -	    unw_get_reg(&cursor, UNW_REG_SP, &sp);
> -
> -	    switch (unw_get_proc_name(&cursor,
> -			procname, sizeof(procname) - 1, &off) ) {
> -	    case 0:
> -		    /* Name found. */
> -	    case -UNW_ENOMEM:
> -		    /* Name truncated. */
> -		    DEBUGADD(0, (" #%u %s + %#llx [ip=%#llx] [sp=%#llx]\n",
> -			    i, procname, (long long)off,
> -			    (long long)ip, (long long) sp));
> -		    break;
> -	    default:
> -	    /* case -UNW_ENOINFO: */
> -	    /* case -UNW_EUNSPEC: */
> -		    /* No symbol name found. */
> -		    DEBUGADD(0, (" #%u %s [ip=%#llx] [sp=%#llx]\n",
> -			    i, "<unknown symbol>",
> -			    (long long)ip, (long long) sp));
> -	    }
> -	    ++i;
> -	} while (unw_step(&cursor) > 0);
> -
> -	return;
> -
> -libunwind_failed:
> -	DEBUG(0, ("unable to produce a stack trace with libunwind\n"));
> -
> -#elif HAVE_BACKTRACE_SYMBOLS
> -	void *backtrace_stack[BACKTRACE_STACK_SIZE];
> -	size_t backtrace_size;
> -	char **backtrace_strings;
> -
> -	/* get the backtrace (stack frames) */
> -	backtrace_size = backtrace(backtrace_stack,BACKTRACE_STACK_SIZE);
> -	backtrace_strings = backtrace_symbols(backtrace_stack, backtrace_size);
> -
> -	DEBUG(0, ("BACKTRACE: %lu stack frames:\n", 
> -		  (unsigned long)backtrace_size));
> -
> -	if (backtrace_strings) {
> -		int i;
> -
> -		for (i = 0; i < backtrace_size; i++)
> -			DEBUGADD(0, (" #%u %s\n", i, backtrace_strings[i]));
> -
> -		/* Leak the backtrace_strings, rather than risk what free() might do */
> -	}
> -
> -#else
> -	DEBUG(0, ("unable to produce a stack trace on this platform\n"));
> -#endif
> -}
> -
> -/*******************************************************************
>    A readdir wrapper which just returns the file name.
>   ********************************************************************/
>  
> diff --git a/source3/wscript b/source3/wscript
> index 8d658b2f53e..ab64e80214e 100644
> --- a/source3/wscript
> +++ b/source3/wscript
> @@ -96,7 +96,7 @@ def configure(conf):
>      # We crash without vfs_default
>      required_static_modules.extend(TO_LIST('vfs_default'))
>  
> -    conf.CHECK_HEADERS('execinfo.h libunwind.h netdb.h')
> +    conf.CHECK_HEADERS('netdb.h')
>      conf.CHECK_HEADERS('linux/falloc.h linux/ioctl.h')
>  
>      conf.CHECK_FUNCS('getcwd fchown chmod fchmod mknod')
> -- 
> 2.11.0
> 
> 
> From 19dba766ad65916fe329db77f6bdd8dc59d882a2 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Tue, 10 Apr 2018 16:37:45 +1200
> Subject: [PATCH 4/4] lib/util: Call log_stack_trace() in smb_panic_default()
> 
> This matches the AD DC with the behaviour in smbd.
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
>  lib/util/fault.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/util/fault.c b/lib/util/fault.c
> index af06c9484db..77415e1134d 100644
> --- a/lib/util/fault.c
> +++ b/lib/util/fault.c
> @@ -126,6 +126,7 @@ static void smb_panic_default(const char *why)
>  {
>  	DEBUG(0,("PANIC (pid %llu): %s\n",
>  		    (unsigned long long)getpid(), why));
> +	log_stack_trace();
>  
>  #if defined(HAVE_PRCTL) && defined(PR_SET_PTRACER)
>  	/*
> -- 
> 2.11.0
> 




More information about the samba-technical mailing list