From owner-freebsd-hackers@FreeBSD.ORG Mon Mar 23 02:03:46 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 84238786 for ; Mon, 23 Mar 2015 02:03:46 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id 9CD02813 for ; Mon, 23 Mar 2015 02:03:44 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygDXuRZ2dA9VPnPGAw--.50379S2; Mon, 23 Mar 2015 10:03:40 +0800 (CST) Date: Mon, 23 Mar 2015 10:03:14 +0800 From: Tiwei Bie To: Mateusz Guzik Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150323005852.GB6798@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygDXuRZ2dA9VPnPGAw--.50379S2 X-Coremail-Antispam: 1UD129KBjvJXoWxKFW3ur13Zw47Gr48Xr47Jwb_yoW3Aw13pF WrCrZ8Ars5Cr43Crsav3yDAa4Yyws5Jw4UW347X3WYkr1F9rykXF18Kr1F9Fn7GrWvgryD Za15Wr9xKry8XrJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUkYb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_tr0E3s1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIE14v26F4UJVW0owA2z4x0Y4 vEx4A2jsIEc7CjxVAFwI0_GcCE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xv F2IEw4CE5I8CrVC2j2WlYx0E2Ix0cI8IcVAFwI0_Jr0_Jr4lYx0Ex4A2jsIE14v26r4j6F 4UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCY02Avz4vE14v_Gr1l42xK82IY c2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s 026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1Y6r17MIIYrxkI7VAKI48JMIIF 0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0x vE42xK8VAvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2 jsIEc7CjxVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU5AKs5UUUUU== X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUQAVQhl-peFAAesZ Cc: Konstantin Belousov , freebsd-hackers@freebsd.org 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: Mon, 23 Mar 2015 02:03:46 -0000 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. ;-( > > 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 /* * Report or change the active event timers hardware. */ static int sysctl_kern_eventtimer_timer(SYSCTL_HANDLER_ARGS) { char buf[32]; struct eventtimer *et; int error; ET_LOCK(); et = timer; snprintf(buf, sizeof(buf), "%s", et->et_name); ET_UNLOCK(); error = sysctl_handle_string(oidp, buf, sizeof(buf), req); ET_LOCK(); et = timer; if (error != 0 || req->newptr == NULL || strcasecmp(buf, et->et_name) == 0) { ET_UNLOCK(); return (error); } et = et_find(buf, 0, 0); if (et == NULL) { ET_UNLOCK(); return (ENOENT); } configtimer(0); et_free(timer); if (et->et_flags & ET_FLAGS_C3STOP) cpu_disable_c3_sleep++; if (timer->et_flags & ET_FLAGS_C3STOP) cpu_disable_c3_sleep--; periodic = want_periodic; timer = et; et_init(timer, timercb, NULL, NULL); configtimer(1); ET_UNLOCK(); return (error); } SYSCTL_PROC(_kern_eventtimer, OID_AUTO, timer, CTLTYPE_STRING | CTLFLAG_RW | CTLFLAG_MPSAFE, 0, 0, sysctl_kern_eventtimer_timer, "A", "Chosen event timer"); 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'. > > + 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"); > > > > @@ -3170,11 +3230,6 @@ corefile_open(const char *comm, uid_t uid, pid_t pid, struct thread *td, > > case 'U': /* user id */ > > sbuf_printf(&sb, "%u", uid); > > break; > > - default: > > - log(LOG_ERR, > > - "Unknown format character %c in " > > - "corename `%s'\n", format[i], format); > > - break; > > } > > This case should be an assertion. E.g. KASSERT(0, "unknown format in > character") > I will update my patch. 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); + 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); + } +} +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; + + 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