[PATCH] Finish the task 'Fix corefilename race'

[Mateusz Guzik]

On Fri, Mar 20, 2015 at 02:36:04PM +0800, Tiwei Bie wrote:
> On Fri, Mar 20, 2015 at 11:18:27AM +0530, Prasad Joshi wrote:
> > On Fri, Mar 20, 2015 at 5:34 AM, Tiwei Bie <btw at mail.ustc.edu.cn> wrote:
> > > On Thu, Mar 19, 2015 at 04:40:04PM +0200, Konstantin Belousov wrote:
> > >> On Thu, Mar 19, 2015 at 07:35:30PM +0800, Tiwei Bie wrote:
> > >> > On Thu, Mar 19, 2015 at 12:10:19PM +0200, Konstantin Belousov wrote:
> > >> > > On Thu, Mar 19, 2015 at 03:13:43PM +0800, Tiwei Bie wrote:
> > >> > > > Hi, Mateusz!
> > >> > > >
> > >> > > > I have finished the task: Fix corefilename race [1].
> > >> > > >
> > >> > > > Following is my patch:
> > >> > > >
> > >> > > > ---
> > >> > > >  sys/kern/kern_sig.c | 22 ++++++++++++++++++++--
> > >> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > >> > > >
> > >> > > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> > >> > > > index 58d9707..a1421cb 100644
> > >> > > > --- a/sys/kern/kern_sig.c
> > >> > > > +++ b/sys/kern/kern_sig.c
> > >> > > > @@ -3090,8 +3090,24 @@ static int compress_user_cores = 0;
> > >> > > >  #endif
> > >> > > >
> > >> > > >  static char corefilename[MAXPATHLEN] = {"%N.core"};
> > >> > > > -SYSCTL_STRING(_kern, OID_AUTO, corefile, CTLFLAG_RWTUN, corefilename,
> > >> > > > -    sizeof(corefilename), "Process corefile name format string");
> > >> > > > +
> > >> > > > +static struct sx corefilename_lock;
> > >> > > > +SX_SYSINIT(corefilename_init, &corefilename_lock, "corefilename lock");
> > >> > > > +
> > >> > > > +static int
> > >> > > > +sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
> > >> > > > +{
> > >> > > > +       int error;
> > >> > > > +
> > >> > > > +       sx_xlock(&corefilename_lock);
> > >> > > > +       error = sysctl_handle_string(oidp, corefilename, MAXPATHLEN, req);
> > 
> > Hello,
> > 
> > Though I am not an expert FreeBSD developer. In my humble opinion,
> > MAXPATHLEN must be replaced with sizeof(corefilename). For example,
> > the SYSCTL_STRING line (above) removed in this patch, preferred to use
> > sizeof(corefilename) instead of MAXPATHLEN.
> > 
> 
> Because corefilename is defined as `char corefilename[MAXPATHLEN]',
> sizeof(corefilename) equals MAXPATHLEN. So, technically speaking,
> both of them are right. But one of them may be preferred to keep the
> coding style consistent. I also think sizeof(corefilename) is a
> better choice. Thanks for pointing it out, I updated my patch:
> 
> ---
>  sys/kern/kern_sig.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 58d9707..8410d9d 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -3089,9 +3089,28 @@ SYSCTL_INT(_kern, OID_AUTO, compress_user_cores_gzlevel, CTLFLAG_RWTUN,
>  static int compress_user_cores = 0;
>  #endif
>  
> +/*
> + * Protect the access to corefilename[] by allproc_lock.
> + */
> +#define	corefilename_lock	allproc_lock
> +
>  static char corefilename[MAXPATHLEN] = {"%N.core"};
> -SYSCTL_STRING(_kern, OID_AUTO, corefile, CTLFLAG_RWTUN, corefilename,
> -    sizeof(corefilename), "Process corefile name format string");
> +
> +static int
> +sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
> +{
> +	int error;
> +
> +	sx_xlock(&corefilename_lock);
> +	error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename),
> +	    req);
> +	sx_xunlock(&corefilename_lock);
> +
> +	return (error);
> +}
> +SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN |
> +    CTLFLAG_MPSAFE, 0, 0, sysctl_kern_corefile, "A",
> +    "Process corefile name format string");
>  
>  /*
>   * corefile_open(comm, uid, pid, td, compress, vpp, namep)
> @@ -3120,6 +3139,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td,
>  	name = malloc(MAXPATHLEN, M_TEMP, M_WAITOK | M_ZERO);
>  	indexpos = -1;
>  	(void)sbuf_new(&sb, name, MAXPATHLEN, SBUF_FIXEDLEN);
> +	sx_slock(&corefilename_lock);
>  	for (i = 0; format[i] != '\0'; i++) {
>  		switch (format[i]) {
>  		case '%':	/* Format character */
> @@ -3162,6 +3182,7 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td,
>  			break;
>  		}
>  	}
> +	sx_sunlock(&corefilename_lock);
>  	free(hostname, M_TEMP);
>  	if (compress)
>  		sbuf_printf(&sb, GZ_SUFFIX);
> 

Thanks, committed as r280312.

-- 
Mateusz Guzik <mjguzik gmail.com>