goto in ptrace_ctrl()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

goto in ptrace_ctrl()

Martin Pieuchot
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?

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;
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: goto in ptrace_ctrl()

Martin Pieuchot
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;
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: goto in ptrace_ctrl()

Mark Kettenis
> Date: Sat, 17 Feb 2018 15:17:11 +0100
> From: Martin Pieuchot <[hidden email]>
>
> 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;
> >  }
> >  
> >
>
>