Re: [ptrace] please review follow fork/exec changes

On Mon, Feb 06, 2012 at 01:19:30PM -0800, Dmitry Mikulin wrote:
> 
> >I see what is going on. The wait loop for P_PPWAIT in do_fork() simply
> >do not allow the ptracestop() in the syscall return path to be reached.
> >There seems to be more problems. In particular, I do not see anything
> >which would prevent the child from being reapped while the loop is
> >executing (assume that the parent is multithreaded and other thread
> >processed SIGCHLD and called wait).
> >
> >Lets deal with these bugs after your proposal for interface changes is
> >dealt with.
> 
> OK.
> 
> >
> >Yes, I agree with the proposal to add flag to the child lwp info.
> >I think it will be easier if the flag is different from PL_FLAG_FORKED.
> >I named it PL_FLAG_CHILD.
> >
> >PT_FOLLOW_EXEC is easy to implement, but my question is, how can debugger
> >operate (correctly) if it ignores exec events ? After exec, the whole
> >cached state of the debuggee must be invalidated, and since debugger
> >ignores the notification when the invalidation shall be done, it probably
> >gets very confused.
> 
> You're right, the debugger needs to handle exec() events implicitly when it 
> starts up executables. The problem is that there is OS-independent 
> machinery in gdb which handles statup fork-exec sequence differently from 
> when the debuggee itself does an exec(). Basically in the event handling 
> code I need to be able to distinguish app startup by gdb from an exec done 
> by the app. Other OS-es have flags like PL_FLAG_EXEC set on demand: they 
> have an equivalent of PT_FOLLOW_EXEC. I attached a modified patch that 
> solves the problem. It tries to separate the always-on TDB_EXEC from the 
> on-demand TDB_FOLLOWEXEC without changing existing functionality. Let me 
> know if it's acceptable.

So, do you in fact need to distinguish exec stops from syscall exit
against exec stops from PT_FOLLOW_EXEC, or do you need to only get stops
at exec returns from PT_CONTINUE when explicitely requested them ?

I would prefer to not introduce another PL_FLAG_<something>EXEC with the
same semantic as PL_FLAG_EXEC. Instead, would the following patch be fine
for your purposes ? With it, stop on exec should only occur if PT_SCX
is requested, or PT_CONTINUE and PT_FOLLOW_EXEC.

[I am unable to fully test this until tomorrow].

diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index 135f798..67cb1b2 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
_at__at_ -56,6 +56,7 _at__at_ __FBSDID("$FreeBSD$");
 #include <sys/proc.h>
 #include <sys/pioctl.h>
 #include <sys/namei.h>
+#include <sys/ptrace.h>
 #include <sys/resourcevar.h>
 #include <sys/sdt.h>
 #include <sys/sf_buf.h>
_at__at_ -889,7 +890,9 _at__at_ exec_fail_dealloc:
 
 	if (error == 0) {
 		PROC_LOCK(p);
-		td->td_dbgflags |= TDB_EXEC;
+		if ((p->p_flag & P_TRACED) != 0 &&
+		    ((P_FOLLOWEXEC) != 0 || (p->p_stops & S_PT_SCX) != 0))
+			td->td_dbgflags |= TDB_EXEC;
 		PROC_UNLOCK(p);
 
 		/*
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 60639c9..e447c93 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
_at__at_ -1035,7 +1035,9 _at__at_ fork_return(struct thread *td, struct trapframe *frame)
 			p->p_oppid = p->p_pptr->p_pid;
 			proc_reparent(p, dbg);
 			sx_xunlock(&proctree_lock);
+			td->td_dbgflags |= TDB_CHILD;
 			ptracestop(td, SIGSTOP);
+			td->td_dbgflags &= ~TDB_CHILD;
 		} else {
 			/*
 			 * ... otherwise clear the request.
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 4510380..79bbaed 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
_at__at_ -660,6 +660,7 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 	case PT_TO_SCX:
 	case PT_SYSCALL:
 	case PT_FOLLOW_FORK:
+	case PT_FOLLOW_EXEC:
 	case PT_DETACH:
 		sx_xlock(&proctree_lock);
 		proctree_locked = 1;
_at__at_ -873,6 +874,12 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 		else
 			p->p_flag &= ~P_FOLLOWFORK;
 		break;
+	case PT_FOLLOW_EXEC:
+		if (data)
+			p->p_flag |= P_FOLLOWEXEC;
+		else
+			p->p_flag &= ~P_FOLLOWEXEC;
+		break;
 
 	case PT_STEP:
 	case PT_CONTINUE:
_at__at_ -936,7 +943,8 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 					p->p_sigparent = SIGCHLD;
 			}
 			p->p_oppid = 0;
-			p->p_flag &= ~(P_TRACED | P_WAITED | P_FOLLOWFORK);
+			p->p_flag &= ~(P_TRACED | P_WAITED | P_FOLLOWFORK |
+			    P_FOLLOWEXEC);
 
 			/* should we send SIGCHLD? */
 			/* childproc_continued(p); */
_at__at_ -1145,6 +1153,8 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 			pl->pl_flags |= PL_FLAG_FORKED;
 			pl->pl_child_pid = td2->td_dbg_forked;
 		}
+		if (td2->td_dbgflags & TDB_CHILD)
+			pl->pl_flags |= PL_FLAG_CHILD;
 		pl->pl_sigmask = td2->td_sigmask;
 		pl->pl_siglist = td2->td_siglist;
 		strcpy(pl->pl_tdname, td2->td_name);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 9ebfe83..bec7223 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
_at__at_ -384,6 +384,7 _at__at_ do {									\
 				      process */
 #define	TDB_STOPATFORK	0x00000080 /* Stop at the return from fork (child
 				      only) */
+#define	TDB_CHILD	0x00000100 /* New child indicator for ptrace() */
 
 /*
  * "Private" flags kept in td_pflags:
_at__at_ -613,6 +614,7 _at__at_ struct proc {
 #define	P_HWPMC		0x800000 /* Process is using HWPMCs */
 
 #define	P_JAILED	0x1000000 /* Process is in jail. */
+#define	P_FOLLOWEXEC	0x2000000 /* Report execs with ptrace. */
 #define	P_INEXEC	0x4000000 /* Process is in execve(). */
 #define	P_STATCHILD	0x8000000 /* Child process stopped or exited. */
 #define	P_INMEM		0x10000000 /* Loaded into memory. */
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index 2583d59..05c758c 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
_at__at_ -64,6 +64,7 _at__at_
 #define	PT_SYSCALL	22
 
 #define	PT_FOLLOW_FORK	23
+#define	PT_FOLLOW_EXEC	24
 
 #define PT_GETREGS      33	/* get general-purpose registers */
 #define PT_SETREGS      34	/* set general-purpose registers */
_at__at_ -106,7 +107,8 _at__at_ struct ptrace_lwpinfo {
 #define	PL_FLAG_SCX	0x08	/* syscall leave point */
 #define	PL_FLAG_EXEC	0x10	/* exec(2) succeeded */
 #define	PL_FLAG_SI	0x20	/* siginfo is valid */
-#define	PL_FLAG_FORKED	0x40	/* new child */
+#define	PL_FLAG_FORKED	0x40	/* child born */
+#define	PL_FLAG_CHILD	0x80	/* I am from child */
 	sigset_t	pl_sigmask;	/* LWP signal mask */
 	sigset_t	pl_siglist;	/* LWP pending signal */
 	struct __siginfo pl_siginfo;	/* siginfo for signal */