[PATCH] Fix ignorance of pipe symbol(|) in dumping core under linux

[Michael Adam]

Hi Anoop,

This looks like a good finding, thanks for the patch!

Comments inline below:

On 2016-11-16 at 15:06 +0530, Anoop C S wrote:
> Hi all,
> 
> Please see the attached patch which enables smbd/nmbd/winbindd to honour pipe symbol (|) if
> specified inside /proc/sys/kernel/core_pattern under linux. Commit message from the patch explains
> this in detail.
> 
> Reviews are appreciated.
> 
> Thanks,
> Anoop C S.

> From de0e4e0395bf69ecf2e853d1844c89a9718f3a3f Mon Sep 17 00:00:00 2001
> From: Anoop C S <anoopcs at redhat.com>
> Date: Wed, 16 Nov 2016 08:55:13 +0000
> Subject: [PATCH] s3/dump_core: Honour pipe symbol (|) in system-wide
>  core_pattern under linux
> 
> From man core(5):
> 
> "Since  kernel  2.6.19, Linux supports an alternate syntax for the
> /proc/sys/kernel/core_pattern file.  If the first character of this
> file is a pipe symbol (|), then the remainder of the line is interpreted
> as a user-space program to be executed."
> 
> Discarding this symbol would result in misleading logs for smbd/nmbd/winbindd.
> For example even if core_pattern contains '|', smbd logs would suggest the
> following:
> 
> ...
> dumping core in /var/log/samba/cores/smbd
> ...
> 
> and coredump may or may not get created at that location depending on which
> helper binary is being used for handling coredumps.
> 
> Signed-off-by: Anoop C S <anoopcs at redhat.com>
> ---
>  source3/lib/dumpcore.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/source3/lib/dumpcore.c b/source3/lib/dumpcore.c
> index c72aa88..c9b3394 100644
> --- a/source3/lib/dumpcore.c
> +++ b/source3/lib/dumpcore.c
> @@ -37,6 +37,7 @@
>  #endif
>  
>  static char *corepath;
> +static bool helper_binary;

Even though I know it's not needed, i would
like to see explicit initialization to "false"
just for clarity.

Also, wouldn't a name like "using_helper_binary"
be more natural for a bool?

Otherwise the patch looks good to me.
(I.e. RB+ barring changes / comments to the above.)

Michael


>  /**
>   * Build up the default corepath as "<logbase>/cores/<progname>"
> @@ -169,6 +170,12 @@ static char *get_linux_corepath(void)
>  		/*
>  		 * No absolute path, use the default (cwd)
>  		 */
> +		if (result[0] == '|') {
> +			/*
> +			* Core dump handled by helper binaries
> +			*/
> +			helper_binary = true;
> +		}
>  		TALLOC_FREE(result);
>  		return NULL;
>  	}
> @@ -291,16 +298,25 @@ void dump_core_setup(const char *progname, const char *log_file)
>  	}
>  
>  	if (*corepath != '\0') {
> -		/* The chdir might fail if we dump core before we finish
> -		 * processing the config file.
> +		/*
> +		 * Check whether coredump is handled by helper binaries or not.
> +		 * If so skip chdir().
>  		 */
> -		if (chdir(corepath) != 0) {
> -			DEBUG(0, ("unable to change to %s\n", corepath));
> -			DEBUGADD(0, ("refusing to dump core\n"));
> -			exit(1);
> -		}
> +		if (!helper_binary) {
> +			/* The chdir might fail if we dump core before we finish
> +			 * processing the config file.
> +			 */
> +			if (chdir(corepath) != 0) {
> +				DEBUG(0, ("unable to change to %s\n", corepath));
> +				DEBUGADD(0, ("refusing to dump core\n"));
> +				exit(1);
> +			}
>  
> -		DEBUG(0,("dumping core in %s\n", corepath));
> +			DEBUG(0,("dumping core in %s\n", corepath));
> +		} else {
> +			DEBUG(0,("coredump is handled by helper binary "
> +				 "specified at /proc/sys/kernel/core_pattern"));
> +		}
>  	}
>  
>  	umask(~(0700));
> -- 
> 2.7.4
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161116/2ce487a7/signature.sig>