Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 24 Mar 2015 13:35:17 +0100
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Tiwei Bie <btw@mail.ustc.edu.cn>
Cc:        Konstantin Belousov <kostikbel@gmail.com>, freebsd-hackers@freebsd.org
Subject:   Re: [PATCH] Finish the task 'Validate coredump format string'
Message-ID:  <20150324123517.GA25678@dft-labs.eu>
In-Reply-To: <20150323020314.GA30143@freebsd>
References:  <1426946345-67889-1-git-send-email-btw@mail.ustc.edu.cn> <20150321200500.GC14650@dft-labs.eu> <20150322091853.GA89976@freebsd> <20150322101401.GH14650@dft-labs.eu> <20150322112555.GA44277@freebsd> <20150322113822.GB2379@kib.kiev.ua> <20150322120655.GA59757@freebsd> <20150322131524.GA95795@freebsd> <20150323005852.GB6798@dft-labs.eu> <20150323020314.GA30143@freebsd>

next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 23, 2015 at 10:03:14AM +0800, Tiwei Bie wrote:
> On Mon, Mar 23, 2015 at 01:58:52AM +0100, Mateusz Guzik wrote:
> > On Sun, Mar 22, 2015 at 09:15:24PM +0800, Tiwei Bie wrote:
> > > On Sun, Mar 22, 2015 at 08:06:55PM +0800, Tiwei Bie wrote:
> > > > On Sun, Mar 22, 2015 at 01:38:22PM +0200, Konstantin Belousov wrote:
> > > > > On Sun, Mar 22, 2015 at 07:25:55PM +0800, Tiwei Bie wrote:
> > > > > > On Sun, Mar 22, 2015 at 11:14:01AM +0100, Mateusz Guzik wrote:
> > > > > > > On Sun, Mar 22, 2015 at 05:19:40PM +0800, Tiwei Bie wrote:
> > > > 
> > > [..]
> > > > I will submit my new patch later.
> > > > 
> > > 
> > > Here is my new patch:
> > > 
> > > ---
> > >  sys/kern/kern_sig.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 64 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> > > index 8410d9d..5162ab6 100644
> > > --- a/sys/kern/kern_sig.c
> > > +++ b/sys/kern/kern_sig.c
> > > @@ -3094,21 +3094,81 @@ static int compress_user_cores = 0;
> > >   */
> > >  #define	corefilename_lock	allproc_lock
> > >  
> > > -static char corefilename[MAXPATHLEN] = {"%N.core"};
> > > +static char corefilename[MAXPATHLEN] = "%N.core";
> > > +
> > > +static bool
> > > +corefilename_check(const char *format)
> > > +{
> > > +	int i, counti;
> > > +
> > > +	counti = 0;
> > > +	for (i = 0; i < MAXPATHLEN && format[i] != '\0'; i++) {
> > > +		if (format[i] == '%') {
> > > +			i++;
> > > +			switch (format[i]) {
> > > +			case 'I':
> > > +				counti++;
> > > +				if (counti > 1)
> > > +					return (false);
> > > +			case '%':
> > > +			case 'H':
> > > +			case 'N':
> > > +			case 'P':
> > > +			case 'U':
> > > +				break;
> > > +			default:
> > > +				return (false);
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	if (i == MAXPATHLEN)
> > > +		return (false);
> > > +
> > > +	return (true);
> > > +}
> > > +
> > > +static void
> > > +corefilename_init(void *arg)
> > > +{
> > > +	char *format;
> > > +
> > > +	format = kern_getenv("kern.corefile");
> > > +	if (format != NULL && corefilename_check(format))
> > > +		strcpy(corefilename, format);
> > > +}
> > > +SYSINIT(corefilename, SI_SUB_KMEM, SI_ORDER_FIRST, corefilename_init, 0);
> > >  
> > 
> > The user should know that the tunable got discarded (e.g. like with
> > prinf removed below).
> > 
> 
> I don't know how to do this. We need to update the man-pages?
> 
> And sorry again, maybe I was too tired yesterday. I just find a new bug in
> the codes above when reading your comments. Though it won't affect anything
> cause dynamic_kenv hasn't been enabled at this time, it is really a bug.
> 
> `format' needs to be freed, sorry... What a terrible day. ;-(
> 

core(5). yeah, would be nice to mention all this stuff.

> > >  static int
> > >  sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
> > >  {
> > > +	char *format;
> > >  	int error;
> > >  
> > > +	format = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
> > > +
> > > +	sx_slock(&corefilename_lock);
> > > +	strcpy(format, corefilename);
> > > +	sx_sunlock(&corefilename_lock);
> > > +
> > > +	error = sysctl_handle_string(oidp, format, MAXPATHLEN, req);
> > > +	if (error || !req->newptr || strcmp(format, corefilename) == 0)
> > > +		goto out;
> > > +
> > 
> > Is this strcmp useful? Normally you don't replace the pattern with
> > identical one.
> > 
> 
> I wrote these codes after consulting the codes in kern/kern_clocksource.c
> 
[..]
> It also needs to validate the user's inputs. And it uses a strcasecmp()
> to check whether a different timer has been specified by the user.
> 
> If the user is going to replace the pattern with identical one, we won't
> need to do any update, we just `goto out'.
> 

Well, I presume original author did not want to play with timers when
changing to the same timer.

Corefile is safe to change, so I suggest just dropping that strcmp.

Also see below.

> My new patch:
> 
> ---
>  sys/kern/kern_sig.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 8410d9d..0254517 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -3094,21 +3094,85 @@ static int compress_user_cores = 0;
>   */
>  #define	corefilename_lock	allproc_lock
>  
> -static char corefilename[MAXPATHLEN] = {"%N.core"};
> +static char corefilename[MAXPATHLEN] = "%N.core";
> +
> +static bool
> +corefilename_check(const char *format)
> +{
> +	int i, counti;
> +
> +	counti = 0;
> +	for (i = 0; i < MAXPATHLEN && format[i] != '\0'; i++) {
> +		if (format[i] == '%') {
> +			i++;
> +			switch (format[i]) {
> +			case 'I':
> +				counti++;
> +				if (counti > 1)
> +					return (false);

countI?

> +			case '%':
> +			case 'H':
> +			case 'N':
> +			case 'P':
> +			case 'U':
> +				break;
> +			default:
> +				return (false);
> +			}
> +		}
> +	}
> +
> +	if (i == MAXPATHLEN)
> +		return (false);
> +
> +	return (true);
> +}
> +
> +static void
> +corefilename_init(void *arg)
> +{
> +	char *format;
> +
> +	format = kern_getenv("kern.corefile");
> +
> +	if (format != NULL) {
> +		if (corefilename_check(format))
> +			strcpy(corefilename, format);
> +		freeenv(format);
> +	}
> +}

printf something. "invalid format specified" or the like.

Also the code may be nicer if you
if (format == NULL)
	return;

instead

> +SYSINIT(corefilename, SI_SUB_KMEM, SI_ORDER_FIRST, corefilename_init, 0);
>  
>  static int
>  sysctl_kern_corefile(SYSCTL_HANDLER_ARGS)
>  {
> +	char *format;
>  	int error;
>  
> +	format = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
> +
> +	sx_slock(&corefilename_lock);
> +	strcpy(format, corefilename);
> +	sx_sunlock(&corefilename_lock);
> +
> +	error = sysctl_handle_string(oidp, format, MAXPATHLEN, req);
> +	if (error || !req->newptr || strcmp(format, corefilename) == 0)
> +		goto out;
> +

this should be if (error != 0 || req->newptr == NULL).

strmcp was covered earlier in the mail

> +	if (!corefilename_check(format)) {
> +		error = EINVAL;
> +		goto out;
> +	}
> +
>  	sx_xlock(&corefilename_lock);
> -	error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename),
> -	    req);
> +	strcpy(corefilename, format);
>  	sx_xunlock(&corefilename_lock);
>  
> +out:
> +	free(format, M_TEMP);
>  	return (error);
>  }
> -SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RWTUN |
> +SYSCTL_PROC(_kern, OID_AUTO, corefile, CTLTYPE_STRING | CTLFLAG_RW |
>      CTLFLAG_MPSAFE, 0, 0, sysctl_kern_corefile, "A",
>      "Process corefile name format string");
>  
> @@ -3171,9 +3235,8 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td,
>  				sbuf_printf(&sb, "%u", uid);
>  				break;
>  			default:
> -				log(LOG_ERR,
> -				    "Unknown format character %c in "
> -				    "corename `%s'\n", format[i], format);
> +				KASSERT(0, ("Unknown format character %c in "
> +				    "corename `%s'\n", format[i], format));
>  				break;
>  			}
>  			break;
> -- 
> 2.1.2
> 
> Best regards,
> Tiwei Bie
> 

-- 
Mateusz Guzik <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150324123517.GA25678>