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

[Anoop C S]

On Wed, 2016-11-16 at 12:55 +0100, Michael Adam wrote:
> 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.
> 

Done.

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

I couldn't think of any other name suitable for this bool variable. So going with Michael's
suggestion. See attached patch.

> 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: 0001-s3-dump_core-Honour-pipe-symbol-in-system-wide-core_.patch
Type: text/x-patch
Size: 2635 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161116/f5689c30/0001-s3-dump_core-Honour-pipe-symbol-in-system-wide-core_.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 299 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161116/f5689c30/signature.sig>