From owner-freebsd-hackers@FreeBSD.ORG Sun Mar 22 11:38:30 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 442994B2 for ; Sun, 22 Mar 2015 11:38:30 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id C08073C5 for ; Sun, 22 Mar 2015 11:38:29 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id t2MBcMv7046313 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Sun, 22 Mar 2015 13:38:22 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua t2MBcMv7046313 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id t2MBcMth046312; Sun, 22 Mar 2015 13:38:22 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 22 Mar 2015 13:38:22 +0200 From: Konstantin Belousov To: Tiwei Bie Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150322112555.GA44277@freebsd> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: freebsd-hackers@freebsd.org, Mateusz Guzik X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 22 Mar 2015 11:38:30 -0000 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