Date: Wed, 25 Mar 2015 18:47:07 +0800 From: Tiwei Bie <btw@mail.ustc.edu.cn> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-hackers@freebsd.org, Mateusz Guzik <mjguzik@gmail.com> Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150325104707.GA25729@freebsd> In-Reply-To: <20150325084130.GX2379@kib.kiev.ua> References: <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> <20150325060012.GA75674@freebsd> <20150325084130.GX2379@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Mar 25, 2015 at 10:41:30AM +0200, Konstantin Belousov wrote:
> On Wed, Mar 25, 2015 at 02:00:12PM +0800, Tiwei Bie wrote:
> > 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
> Please bump date for the man page.
>
Yeah, I will!
> > @@ -78,6 +78,7 @@ An index starting at zero until the sysctl
> > +
[..]
> > +static int
> > +corefilename_check(const char *format)
> > +{
> > + int i, countI;
> > +
> > + countI = 0;
> > + for (i = 0; i < MAXPATHLEN && format[i] != '\0'; i++) {
> Was it already suggested to use sizeof(corefilename) instead of MAXPATHLEN ?
>
Sorry, I didn't notice this.
> > + if (format[i] == '%') {
> > +
[..]
> > + format = kern_getenv("kern.corefile");
> > + if (format == NULL)
> > + return;
> > +
> > + error = corefilename_check(format);
> > +
> > + switch (error) {
> > + case 0:
> > + strcpy(corefilename, format);
> Use strlcpy(9).
>
Got it, thanks!
> > + 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;
> Sigh. Why kernel should decode the error codes ? Why should it print
> anything on the console, with non-zero chance of sysctl caller not seeing
> the text ?
>
I will remove these prints.
My new patch:
---
share/man/man5/core.5 | 9 +++++-
sys/kern/kern_sig.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 78 insertions(+), 8 deletions(-)
diff --git a/share/man/man5/core.5 b/share/man/man5/core.5
index 3f54f89..cfbcb1c 100644
--- a/share/man/man5/core.5
+++ b/share/man/man5/core.5
@@ -28,7 +28,7 @@
.\" @(#)core.5 8.3 (Berkeley) 12/11/93
.\" $FreeBSD$
.\"
-.Dd March 8, 2015
+.Dd March 25, 2015
.Dt CORE 5
.Os
.Sh NAME
@@ -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..611d138 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 int
+corefilename_check(const char *format)
+{
+ int i, countI;
+
+ countI = 0;
+ for (i = 0; i < sizeof(corefilename) && 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 == sizeof(corefilename))
+ return (ENAMETOOLONG);
+
+ return (0);
+}
+
+static void
+corefilename_init(void *arg)
+{
+ char *format;
+
+ format = kern_getenv("kern.corefile");
+ if (format == NULL)
+ return;
+
+ if (corefilename_check(format) == 0)
+ strlcpy(corefilename, format, sizeof(corefilename));
+
+ 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(sizeof(corefilename), M_TEMP, M_WAITOK);
+
+ sx_slock(&corefilename_lock);
+ strlcpy(format, corefilename, sizeof(corefilename));
+ sx_sunlock(&corefilename_lock);
+
+ error = sysctl_handle_string(oidp, format, sizeof(corefilename), 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);
+ strlcpy(corefilename, format, sizeof(corefilename));
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'", 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?20150325104707.GA25729>