From owner-freebsd-hackers@FreeBSD.ORG Tue Mar 24 12:35:23 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 A7001D74 for ; Tue, 24 Mar 2015 12:35:23 +0000 (UTC) Received: from mail-wi0-x22a.google.com (mail-wi0-x22a.google.com [IPv6:2a00:1450:400c:c05::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 305DAFBF for ; Tue, 24 Mar 2015 12:35:23 +0000 (UTC) Received: by wibdy8 with SMTP id dy8so73646486wib.0 for ; Tue, 24 Mar 2015 05:35:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=DDoKnqvkThLnADxkAJK6aVCACqKYToNyqyGigo0/48A=; b=bT9I1jG4E7ikMVO5ot62qJAmkSsiyV4m/UD0R0Jw2a9E2v3CMScgecXeG04ps6Ikqf e9T6Vq1LoyqEAiOTR9SbVCl+ZlLuw4wa62G8UMb+6goLqz8nkvjv0dpuaGxLt3GHUXg0 0vz14bDJ6TiEeBwBJ4NrELo/rgSk/rS51h1fITjdxIX5xTPB3u8lAmNzLLs6LZB0nzxX PSZpQ7GN56P1Ddo+YzbIR5TdW0r3VjSKp4hk+NnYo0h0JwYa/QZnYS/4/BMLET/UxEpn M3EG7DZozEB98kHdARb0t5UNBnmUskrsg5SUSixJW2MNFpT5ppDeLDW08fV/V0UoZ1xn WYWw== X-Received: by 10.194.24.103 with SMTP id t7mr7419906wjf.15.1427200521588; Tue, 24 Mar 2015 05:35:21 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id uo6sm5882647wjc.49.2015.03.24.05.35.19 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 24 Mar 2015 05:35:20 -0700 (PDT) Date: Tue, 24 Mar 2015 13:35:17 +0100 From: Mateusz Guzik To: Tiwei Bie Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150324123517.GA25678@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Tiwei Bie , Konstantin Belousov , freebsd-hackers@freebsd.org 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> <20150323020314.GA30143@freebsd> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150323020314.GA30143@freebsd> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Tue, 24 Mar 2015 12:35:23 -0000 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: > > > > > > > [..] > > > > 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. ;-( > core(5). yeah, would be nice to mention all this stuff. > > > 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 > [..] > 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'. > Well, I presume original author did not want to play with timers when changing to the same timer. Corefile is safe to change, so I suggest just dropping that strcmp. Also see below. > 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); countI? > + 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); > + } > +} printf something. "invalid format specified" or the like. Also the code may be nicer if you if (format == NULL) return; instead > +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; > + this should be if (error != 0 || req->newptr == NULL). strmcp was covered earlier in the mail > + 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 > -- Mateusz Guzik