From owner-freebsd-hackers@FreeBSD.ORG Wed Mar 25 06:00:47 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 BDCB4B68 for ; Wed, 25 Mar 2015 06:00:47 +0000 (UTC) Received: from ustc.edu.cn (email6.ustc.edu.cn [IPv6:2001:da8:d800::8]) by mx1.freebsd.org (Postfix) with ESMTP id 15D3D876 for ; Wed, 25 Mar 2015 06:00:46 +0000 (UTC) Received: from freebsd (unknown [58.211.218.74]) by newmailweb.ustc.edu.cn (Coremail) with SMTP id LkAmygCnVzwHTxJVSnfVAw--.50550S2; Wed, 25 Mar 2015 14:00:43 +0800 (CST) Date: Wed, 25 Mar 2015 14:00:12 +0800 From: Tiwei Bie To: Mateusz Guzik Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150325060012.GA75674@freebsd> References: <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> <20150323020314.GA30143@freebsd> <20150324123517.GA25678@dft-labs.eu> <20150324143709.GA54065@freebsd> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150324143709.GA54065@freebsd> User-Agent: Mutt/1.5.23 (2014-03-12) X-CM-TRANSID: LkAmygCnVzwHTxJVSnfVAw--.50550S2 X-Coremail-Antispam: 1UD129KBjvJXoWxKw18WF1Utry7ur1kKr43GFg_yoW7Ww18pF Wakr95AFs0kr43Cr9ay3yrZ34Yywn5Jay5X347Zw4akrWFgryDXr18Kw1YvFykGrWvq3s8 Ja15Wry2gryUZ3DanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUkYb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Ar0_tr1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwV C2z280aVCY1x0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC 0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUGVWUXwAv7VC2z280aVAFwI0_Cr0_Gr 1UMcvjeVCFs4IE7xkEbVWUJVW8JwACjcxG0xvEwIxGrwCY02Avz4vE14v_GF1l42xK82IY c2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGwC20s 026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1Y6r17MIIYrxkI7VAKI48JMIIF 0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMIIF0x vE42xK8VAvwI8IcIk0rVWrZr1j6s0DMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2 jsIEc7CjxVAFwI0_Jr0_GrUvcSsGvfC2KfnxnUUI43ZEXa7IU59SdJUUUUU== X-CM-SenderInfo: xewzqzxdloh3xvwfhvlgxou0/1tbiAQUTAVQhl-tn9QAAsd 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: Wed, 25 Mar 2015 06:00:47 -0000 On Tue, Mar 24, 2015 at 10:37:09PM +0800, Tiwei Bie wrote: > On Tue, Mar 24, 2015 at 01:35:17PM +0100, Mateusz Guzik wrote: > > On Mon, Mar 23, 2015 at 10:03:14AM +0800, Tiwei Bie wrote: > > > 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: > > > [..] > +static void > +corefilename_init(void *arg) > +{ > + char *format; > + > + format = kern_getenv("kern.corefile"); > + if (format == NULL) > + return; > + > + if (corefilename_check(format)) > + strcpy(corefilename, format); > + else > + printf("Invalid format character specified in " > + "corename `%s'\n", format); > + The message printed in corefilename_init() is not proper, it doesn't cover the case that the length is too long, nor the case that %I is specified more than once. I extended the corefilename_check() to be able to return the reason (error code) if it fails. But I'm not sure whether it is worthwhile, cause it seems currently there is no way to specified a string longer than MAXPATHLEN via sysctl or kenv. I also removed the '\n' character from KASSERT() in corefile_open(), as what the other codes do. Here is my new patch: --- share/man/man5/core.5 | 7 ++++ sys/kern/kern_sig.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/share/man/man5/core.5 b/share/man/man5/core.5 index 3f54f89..3047e0a 100644 --- a/share/man/man5/core.5 +++ b/share/man/man5/core.5 @@ -78,6 +78,7 @@ An index starting at zero until the sysctl is reached. This can be useful for limiting the number of corefiles generated by a particular process. +This specifier can only be specified at most once. .It Em \&%N process name. .It Em \&%P @@ -91,6 +92,12 @@ The name defaults to yielding the traditional .Fx behaviour. +When changing the name via the +.Va kern.corefile +sysctl, it will fail if the new name contains +unknown format specifiers, or +.Em \&%I +is specified more than once, or its length is too long. .Pp By default, a process that changes user or group credentials whether real or effective will not create a corefile. diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 154c250..7fdb079 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -3094,21 +3094,101 @@ static int compress_user_cores = 0; */ #define corefilename_lock allproc_lock -static char corefilename[MAXPATHLEN] = {"%N.core"}; +static char corefilename[MAXPATHLEN] = "%N.core"; + +static int +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 (EINVAL); + case '%': + case 'H': + case 'N': + case 'P': + case 'U': + break; + default: + return (EINVAL); + } + } + } + + if (i == MAXPATHLEN) + return (ENAMETOOLONG); + + return (0); +} + +static void +corefilename_init(void *arg) +{ + char *format; + int error; + + format = kern_getenv("kern.corefile"); + if (format == NULL) + return; + + error = corefilename_check(format); + + switch (error) { + case 0: + strcpy(corefilename, format); + break; + case EINVAL: + printf("Invalid format specified for corename `%s'\n", format); + break; + case ENAMETOOLONG: + printf("The format specified for corename is too long\n"); + break; + default: + KASSERT(0, ("%s: unknown return value %d from " + "corefilename_check", __func__, error)); + break; + } + + 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 != 0 || req->newptr == NULL) + goto out; + + error = corefilename_check(format); + if (error != 0) + 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 +3251,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'", format[i], format)); break; } break; -- 2.1.2 Best regards, Tiwei Bie