Ping?
On Sat, Dec 12, 2015 at 8:38 PM, Maxim Pugachev <pugachev...@gmail.com> wrote:
> Hi,
>
> This patch removes copypasted code that prepares args and env in exec
> system call.
>
>
> Index: sys/kern/kern_exec.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exec.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 kern_exec.c
> --- sys/kern/kern_exec.c 5 Dec 2015 10:11:53 -0000 1.173
> +++ sys/kern/kern_exec.c 12 Dec 2015 17:34:43 -0000
> @@ -77,6 +77,12 @@ const struct kmem_va_mode kv_exec = {
> .kv_map = &exec_map
> };
>
> +struct argcontext
> +{
> + char *buffer;
> + char *bptr;
> +};
> +
> /*
> * Map the shared signal code.
> */
> @@ -231,6 +237,58 @@ bad1:
> return (error);
> }
>
> +void
> +copy_fake_args(struct exec_package *pack, struct argcontext *argctx,
> + long *count)
> +{
> + char **fake_args_vec = pack->ep_fa;
> +
> + while (*fake_args_vec != NULL) {
> + char *arg = *fake_args_vec;
> +
> + while (*arg)
> + *argctx->bptr++ = *arg++;
> +
> + *argctx->bptr++ = '\0';
> +
> + free(*fake_args_vec, M_EXEC, 0);
> +
> + fake_args_vec++;
> + (*count)++;
> + }
> +
> + free(pack->ep_fa, M_EXEC, 0);
> + pack->ep_flags &= ~EXEC_HASARGL;
> +}
> +
> +int
> +copy_strings(char * const *src, struct argcontext *argctx, long *count)
> +{
> + int error;
> + size_t len;
> + size_t maxlen = argctx->buffer + ARG_MAX - argctx->bptr;
> + char *sp;
> +
> + while (1) {
> + if ((error = copyin(src, &sp, sizeof(sp))) != 0)
> + return error;
> + if (!sp)
> + break;
> + if ((error = copyinstr(sp, argctx->bptr, maxlen, &len)) != 0)
> {
> + if (error == ENAMETOOLONG)
> + error = E2BIG;
> + return error;
> + }
> +
> + maxlen -= len;
> + argctx->bptr += len;
> + src++;
> + (*count)++;
> + }
> +
> + return 0;
> +}
> +
> /*
> * exec system call
> */
> @@ -242,13 +300,13 @@ sys_execve(struct proc *p, void *v, regi
> syscallarg(char *const *) argp;
> syscallarg(char *const *) envp;
> } */ *uap = v;
> + struct argcontext argctx;
> int error;
> struct exec_package pack;
> struct nameidata nid;
> struct vattr attr;
> struct ucred *cred = p->p_ucred;
> - char *argp;
> - char * const *cpp, *dp, *sp;
> + char * const *cpp;
> #ifdef KTRACE
> char *env_start;
> #endif
> @@ -261,7 +319,6 @@ sys_execve(struct proc *p, void *v, regi
> char *stack;
> struct ps_strings arginfo;
> struct vmspace *vm = pr->ps_vmspace;
> - char **tmpfap;
> extern struct emul emul_native;
> #if NSYSTRACE > 0
> int wassugid = ISSET(pr->ps_flags, PS_SUGID | PS_SUGIDEXEC);
> @@ -317,38 +374,23 @@ sys_execve(struct proc *p, void *v, regi
> pack.ep_flags = 0;
>
> /* see if we can run it. */
> - if ((error = check_exec(p, &pack)) != 0) {
> + if ((error = check_exec(p, &pack)) != 0)
> goto freehdr;
> - }
>
> /* XXX -- THE FOLLOWING SECTION NEEDS MAJOR CLEANUP */
> -
> +
> /* allocate an argument buffer */
> - argp = km_alloc(NCARGS, &kv_exec, &kp_pageable, &kd_waitok);
> + argctx.buffer = km_alloc(NCARGS, &kv_exec, &kp_pageable, &kd_waitok);
> #ifdef DIAGNOSTIC
> - if (argp == NULL)
> - panic("execve: argp == NULL");
> + if (argctx.buffer == NULL)
> + panic("execve: argctx.buffer == NULL");
> #endif
> - dp = argp;
> + argctx.bptr = argctx.buffer;
> argc = 0;
>
> /* copy the fake args list, if there's one, freeing it as we go */
> - if (pack.ep_flags & EXEC_HASARGL) {
> - tmpfap = pack.ep_fa;
> - while (*tmpfap != NULL) {
> - char *cp;
> -
> - cp = *tmpfap;
> - while (*cp)
> - *dp++ = *cp++;
> - *dp++ = '\0';
> -
> - free(*tmpfap, M_EXEC, 0);
> - tmpfap++; argc++;
> - }
> - free(pack.ep_fa, M_EXEC, 0);
> - pack.ep_flags &= ~EXEC_HASARGL;
> - }
> + if (pack.ep_flags & EXEC_HASARGL)
> + copy_fake_args(&pack, &argctx, &argc);
>
> /* Now get argv & environment */
> if (!(cpp = SCARG(uap, argp))) {
> @@ -359,21 +401,9 @@ sys_execve(struct proc *p, void *v, regi
> if (pack.ep_flags & EXEC_SKIPARG)
> cpp++;
>
> - while (1) {
> - len = argp + ARG_MAX - dp;
> - if ((error = copyin(cpp, &sp, sizeof(sp))) != 0)
> - goto bad;
> - if (!sp)
> - break;
> - if ((error = copyinstr(sp, dp, len, &len)) != 0) {
> - if (error == ENAMETOOLONG)
> - error = E2BIG;
> - goto bad;
> - }
> - dp += len;
> - cpp++;
> - argc++;
> - }
> + /* copy args */
> + if (copy_strings(cpp, &argctx, &argc) != 0)
> + goto bad;
>
> /* must have at least one argument */
> if (argc == 0) {
> @@ -382,39 +412,31 @@ sys_execve(struct proc *p, void *v, regi
> }
>
> #ifdef KTRACE
> - if (KTRPOINT(p, KTR_EXECARGS))
> - ktrexec(p, KTR_EXECARGS, argp, dp - argp);
> + if (KTRPOINT(p, KTR_EXECARGS)) {
> + ktrexec(p, KTR_EXECARGS, argctx.buffer,
> + argctx.bptr - argctx.buffer);
> + }
> #endif
>
> envc = 0;
> /* environment does not need to be there */
> if ((cpp = SCARG(uap, envp)) != NULL ) {
> #ifdef KTRACE
> - env_start = dp;
> + env_start = argctx.bptr;
> #endif
> - while (1) {
> - len = argp + ARG_MAX - dp;
> - if ((error = copyin(cpp, &sp, sizeof(sp))) != 0)
> - goto bad;
> - if (!sp)
> - break;
> - if ((error = copyinstr(sp, dp, len, &len)) != 0) {
> - if (error == ENAMETOOLONG)
> - error = E2BIG;
> - goto bad;
> - }
> - dp += len;
> - cpp++;
> - envc++;
> - }
> -
> + if (copy_strings(cpp, &argctx, &envc) != 0)
> + goto bad;
> #ifdef KTRACE
> - if (KTRPOINT(p, KTR_EXECENV))
> - ktrexec(p, KTR_EXECENV, env_start, dp - env_start);
> + if (KTRPOINT(p, KTR_EXECENV)) {
> + ktrexec(p, KTR_EXECENV, env_start,
> + argctx.bptr - env_start);
> + }
> #endif
> }
>
> - dp = (char *)(((long)dp + _STACKALIGNBYTES) & ~_STACKALIGNBYTES);
> + argctx.bptr =
> + (char *)(((long)argctx.bptr + _STACKALIGNBYTES) &
> + ~_STACKALIGNBYTES);
>
> sgap = STACKGAPLEN;
>
> @@ -430,7 +452,8 @@ sys_execve(struct proc *p, void *v, regi
>
> /* Now check if args & environ fit into new stack */
> len = ((argc + envc + 2 + pack.ep_emul->e_arglen) * sizeof(char *) +
> - sizeof(long) + dp + sgap + sizeof(struct ps_strings)) - argp;
> + sizeof(long) + argctx.bptr + sgap + sizeof(struct ps_strings)) -
> + argctx.buffer;
>
> len = (len + _STACKALIGNBYTES) &~ _STACKALIGNBYTES;
>
> @@ -505,7 +528,7 @@ sys_execve(struct proc *p, void *v, regi
> stack = (char *)(vm->vm_minsaddr - len);
> #endif
> /* Now copy argc, args & environ to new stack */
> - if (!(*pack.ep_emul->e_copyargs)(&pack, &arginfo, stack, argp))
> + if (!(*pack.ep_emul->e_copyargs)(&pack, &arginfo, stack,
> argctx.buffer))
> goto exec_abort;
>
> /* copy out the process's ps_strings structure */
> @@ -672,7 +695,7 @@ sys_execve(struct proc *p, void *v, regi
> timespecclear(&p->p_tu.tu_runtime);
> p->p_tu.tu_uticks = p->p_tu.tu_sticks = p->p_tu.tu_iticks = 0;
>
> - km_free(argp, NCARGS, &kv_exec, &kp_pageable);
> + km_free(argctx.buffer, NCARGS, &kv_exec, &kp_pageable);
>
> pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
> vn_close(pack.ep_vp, FREAD, cred, p);
> @@ -777,7 +800,7 @@ bad:
> /* close and put the exec'd file */
> vn_close(pack.ep_vp, FREAD, cred, p);
> pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
> - km_free(argp, NCARGS, &kv_exec, &kp_pageable);
> + km_free(argctx.buffer, NCARGS, &kv_exec, &kp_pageable);
>
> freehdr:
> free(pack.ep_hdr, M_EXEC, pack.ep_hdrlen);
> @@ -806,7 +829,7 @@ exec_abort:
> free(pack.ep_emul_arg, M_TEMP, pack.ep_emul_argsize);
> pool_put(&namei_pool, nid.ni_cnd.cn_pnbuf);
> vn_close(pack.ep_vp, FREAD, cred, p);
> - km_free(argp, NCARGS, &kv_exec, &kp_pageable);
> + km_free(argctx.buffer, NCARGS, &kv_exec, &kp_pageable);
>
> free_pack_abort:
> free(pack.ep_hdr, M_EXEC, pack.ep_hdrlen);
