Date: Sun, 22 Mar 2015 20:06:55 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-hackers@freebsd.org, Mateusz Guzik <mjguzik@gmail.com> Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150322120655.GA59757@freebsd> In-Reply-To: <20150322113822.GB2379@kib.kiev.ua> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
> > [..]
> > >
> > > 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 ?
>
It is not needed. But I didn't modify this line.
> >
> > +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.
>
I will remove it. I was not sure about it. Thank you very much for telling
me about this!
> > + 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).
>
Sorry...
> > +
> > + 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 ?
>
No, it is NOT. Sorry...
> > +}
> > +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 ?
>
I'm so sorry, I was too hurried.
I will submit my new patch later.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150322120655.GA59757>