Date: Mon, 23 Mar 2015 10:03:14 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, freebsd-hackers@freebsd.org Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150323020314.GA30143@freebsd> In-Reply-To: <20150323005852.GB6798@dft-labs.eu> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150323020314.GA30143>