Date: Wed, 25 Mar 2015 10:41:30 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Tiwei Bie <btw@mail.ustc.edu.cn> Cc: freebsd-hackers@freebsd.org, Mateusz Guzik <mjguzik@gmail.com> Subject: Re: [PATCH] Finish the task 'Validate coredump format string' Message-ID: <20150325084130.GX2379@kib.kiev.ua> In-Reply-To: <20150325060012.GA75674@freebsd> References: <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> <20150325060012.GA75674@freebsd>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> @@ -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++) {
Was it already suggested to use sizeof(corefilename) instead of MAXPATHLEN ?
> + 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);
Use strlcpy(9).
> + 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 ?
> + 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);
Again, use strlcpy. It works correctly, comparing with strncpy.
> + 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);
And there.
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150325084130.GX2379>