> Date: Sat, 17 Feb 2018 15:17:11 +0100
> From: Martin Pieuchot <m...@openbsd.org>
>
> 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?
Guess guenther@ is still busy.
ok kettenis@
> > 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;
> > }
> >
> >
>
>
