Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 22 Mar 2015 13:38:22 +0200
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Tiwei Bie <btw@mail.ustc.edu.cn>
Cc:        freebsd-hackers@freebsd.org, Mateusz Guzik <mjguzik@gmail.com>
Subject:   Re: [PATCH] Finish the task 'Validate coredump format string'
Message-ID:  <20150322113822.GB2379@kib.kiev.ua>
In-Reply-To: <20150322112555.GA44277@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>

next in thread | previous in thread | raw e-mail | index | archive | help
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:
> [..]
> > 
> > A dedicated sysinit func could fetch and validate the tunable, if any.
> > If no tunable was provided it would alloc memory for the default.
> > 
> 
> My new patch, that uses a dedicated sysinit func to fetch and validate
> the tunable:
> 
> ---
>  sys/kern/kern_sig.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 8410d9d..e2a00ba 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -3096,19 +3096,77 @@ static int compress_user_cores = 0;
>  
>  static char corefilename[MAXPATHLEN] = {"%N.core"};
Are {} around string literal needed ?

>  
> +static bool
> +corefilename_check(const char *format)
> +{
> +	int i, counti;
> +
> +	counti = 0;
> +	for (i = 0; format[i] != '\0'; i++) {
> +		if (format[i] == '%') {
> +			i++;
> +			switch (format[i]) {
> +			case 'I':
> +				counti++;
> +				if (counti > 1) {
> +					printf("Too many index flags specified "
> +					    "in corename `%s'\n", format);
I very much dislike the kernel lecturing the user about things.
Kernel must silently return an error code, with man pages providing
explanation of errors.

> +					return (false);
> +				}
> +			case '%':
> +			case 'H':
> +			case 'N':
> +			case 'P':
> +			case 'U':
> +				break;
> +			default:
> +				printf("Unknown format character %c in "
> +				    "corename `%s'\n", format[i], format);
Same there.

> +				return (false);
> +			}
> +		}
> +	}
> +
> +	return (true);
> +}
> +
> +static void
> +corefilename_init(void *arg)
> +{
> +	char *format = kern_getenv("kern.corefile");
Do not use initialization at the local variable declaration.  It is
a requirement of style(9).

> +
> +	if (format != NULL && corefilename_check(format))
> +		strncpy(corefilename, format, sizeof(corefilename));
Use of strncpy() is almost always an indication of the bug.  Is it acceptable
for corefilename be non-null terminated ?

> +}
> +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);
> +	strncpy(format, corefilename, MAXPATHLEN);
> +	sx_sunlock(&corefilename_lock);
> +
> +	error = sysctl_handle_string(oidp, format, MAXPATHLEN, req);
> +	if (error != 0 || strcmp(format, corefilename) == 0 ||
> +	    !corefilename_check(format))
> +		goto out;
This code somehow misses the check for req->newptr.  Did you verified
that simply fetching current value for kern.corefile works after the
patch ?

> +
>  	sx_xlock(&corefilename_lock);
>  	error = sysctl_handle_string(oidp, corefilename, sizeof(corefilename),
>  	    req);
>  	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");
>  
> -- 
> 2.1.2
> 
> Best regards,
> Tiwei Bie



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