From owner-freebsd-hackers@FreeBSD.ORG Sun Mar 22 12:07:24 2015 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BA9D8C6D for ; Sun, 22 Mar 2015 12:07:24 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id DAA9D88C for ; Sun, 22 Mar 2015 12:07:23 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygCHaRZxsA5V_T7DAw--.37634S2; Sun, 22 Mar 2015 20:07:18 +0800 (CST) Date: Sun, 22 Mar 2015 20:06:55 +0800 From: Tiwei Bie To: Konstantin Belousov Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150322120655.GA59757@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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150322113822.GB2379@kib.kiev.ua> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygCHaRZxsA5V_T7DAw--.37634S2 X-Coremail-Antispam: 1UD129KBjvJXoWxWrW8WF4rGF43tF4UGF4xCrg_yoW5ZrWxpF WFkrZYyF4YyrW3Cr1av3y8Aa4Fy3s5Jr45W347JFn8KF9Y9r1kJr1Fgw1F9F1kWr1vgwnF vay5WFy7K345Z3DanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvFb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Xr0_Ar1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr0_Cr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4 A2jsIEc7CjxVAFwI0_GcCE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IE w4CE5I8CrVC2j2WlYx0E2Ix0cI8IcVAFwI0_JrI_JrylYx0Ex4A2jsIE14v26r4j6F4UMc vjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCY02Avz4vE14v_Xr1l42xK82IYc2Ij 64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1l4IxYO2xFxVAFwI0_Jrv_JF1lx2IqxVAqx4 xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1Y6r17 MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I 0E14v26r1j6r4UMIIF0xvE42xK8VAvwI8IcIk0rVWrJr0_WFyUJwCI42IY6I8E87Iv67AK xVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73UjIFyTuYvj xUy3l8UUUUU X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUQAVQhl-peFAAOsJ 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 12:07:24 -0000 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.