On 12/02/18(Mon) 09:26, Martin Pieuchot wrote:
> Diff below introduce multiple 'goto fail' in ptrace_ctrl(). It is
> extracted from guenther@'s proctreelk diff because it doesn't introduce
> any change in behavior. I'd like to get it in to shrink the locking
> diff.
>
> ok?
Anyone?
> Index: kern/sys_process.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_process.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 sys_process.c
> --- kern/sys_process.c 14 Oct 2017 10:17:08 -0000 1.78
> +++ kern/sys_process.c 9 Feb 2018 20:37:58 -0000
> @@ -295,8 +295,10 @@ ptrace_ctrl(struct proc *p, int req, pid
> case PT_ATTACH:
> case PT_DETACH:
> /* Find the process we're supposed to be operating on. */
> - if ((tr = prfind(pid)) == NULL)
> - return (ESRCH);
> + if ((tr = prfind(pid)) == NULL) {
> + error = ESRCH;
> + goto fail;
> + }
> t = TAILQ_FIRST(&tr->ps_threads);
> break;
>
> @@ -305,20 +307,24 @@ ptrace_ctrl(struct proc *p, int req, pid
> #ifdef PT_STEP
> case PT_STEP:
> #endif
> - if ((tr = process_tprfind(pid, &t)) == NULL)
> - return ESRCH;
> + if ((tr = process_tprfind(pid, &t)) == NULL) {
> + error = ESRCH;
> + goto fail;
> + }
> break;
> }
>
> /* Check permissions/state */
> if (req != PT_ATTACH) {
> /* Check that the data is a valid signal number or zero. */
> - if (req != PT_KILL && (data < 0 || data >= NSIG))
> - return EINVAL;
> + if (req != PT_KILL && (data < 0 || data >= NSIG)) {
> + error = EINVAL;
> + goto fail;
> + }
>
> /* Most operations require the target to already be traced */
> if ((error = process_checktracestate(p->p_p, tr, t)))
> - return error;
> + goto fail;
>
> /* Do single-step fixup if needed. */
> FIX_SSTEP(t);
> @@ -327,26 +333,34 @@ ptrace_ctrl(struct proc *p, int req, pid
> * PT_ATTACH is the opposite; you can't attach to a process if:
> * (1) it's the process that's doing the attaching,
> */
> - if (tr == p->p_p)
> - return (EINVAL);
> + if (tr == p->p_p) {
> + error = EINVAL;
> + goto fail;
> + }
>
> /*
> * (2) it's a system process
> */
> - if (ISSET(tr->ps_flags, PS_SYSTEM))
> - return (EPERM);
> + if (ISSET(tr->ps_flags, PS_SYSTEM)) {
> + error = EPERM;
> + goto fail;
> + }
>
> /*
> * (3) it's already being traced, or
> */
> - if (ISSET(tr->ps_flags, PS_TRACED))
> - return (EBUSY);
> + if (ISSET(tr->ps_flags, PS_TRACED)) {
> + error = EBUSY;
> + goto fail;
> + }
>
> /*
> * (4) it's in the middle of execve(2)
> */
> - if (ISSET(tr->ps_flags, PS_INEXEC))
> - return (EAGAIN);
> + if (ISSET(tr->ps_flags, PS_INEXEC)) {
> + error = EAGAIN;
> + goto fail;
> + }
>
> /*
> * (5) it's not owned by you, or the last exec
> @@ -362,14 +376,14 @@ ptrace_ctrl(struct proc *p, int req, pid
> if ((tr->ps_ucred->cr_ruid != p->p_ucred->cr_ruid ||
> ISSET(tr->ps_flags, PS_SUGIDEXEC | PS_SUGID)) &&
> (error = suser(p, 0)) != 0)
> - return (error);
> + goto fail;
>
> /*
> * (5.5) it's not a child of the tracing process.
> */
> if (global_ptrace == 0 && !inferior(tr, p->p_p) &&
> (error = suser(p, 0)) != 0)
> - return (error);
> + goto fail;
>
> /*
> * (6) ...it's init, which controls the security level
> @@ -377,16 +391,20 @@ ptrace_ctrl(struct proc *p, int req, pid
> * compiled with permanently insecure mode turned
> * on.
> */
> - if ((tr->ps_pid == 1) && (securelevel > -1))
> - return (EPERM);
> + if ((tr->ps_pid == 1) && (securelevel > -1)) {
> + error = EPERM;
> + goto fail;
> + }
>
> /*
> * (7) it's an ancestor of the current process and
> * not init (because that would create a loop in
> * the process graph).
> */
> - if (tr->ps_pid != 1 && inferior(p->p_p, tr))
> - return (EINVAL);
> + if (tr->ps_pid != 1 && inferior(p->p_p, tr)) {
> + error = EINVAL;
> + goto fail;
> + }
> }
>
> switch (req) {
> @@ -419,7 +437,7 @@ ptrace_ctrl(struct proc *p, int req, pid
> /* If the address parameter is not (int *)1, set the pc. */
> if ((int *)addr != (int *)1)
> if ((error = process_set_pc(t, addr)) != 0)
> - return error;
> + goto fail;
>
> #ifdef PT_STEP
> /*
> @@ -427,7 +445,7 @@ ptrace_ctrl(struct proc *p, int req, pid
> */
> error = process_sstep(t, req == PT_STEP);
> if (error)
> - return error;
> + goto fail;
> #endif
> goto sendsig;
>
> @@ -453,7 +471,7 @@ ptrace_ctrl(struct proc *p, int req, pid
> */
> error = process_sstep(t, 0);
> if (error)
> - return error;
> + goto fail;
> #endif
>
> /* give process back to original parent or init */
> @@ -515,6 +533,7 @@ ptrace_ctrl(struct proc *p, int req, pid
> break;
> }
>
> +fail:
> return error;
> }
>
>
