Re: ptrace single-stepping change breaks Wine

[Jesse Allen <the3dfxdude@gmail.com>]

On Thu, 30 Dec 2004 09:59:27 -0800 (PST), Davide Libenzi
<davidel@xmailserver.org> wrote:
> On Wed, 29 Dec 2004, Linus Torvalds wrote:
> 
> > On Wed, 29 Dec 2004, Davide Libenzi wrote:
> > >
> > > I think same. My test simply let the function processing to let thru and
> > > reach the fake signal sending point.
> >
> > No, your test-case doesn't even send a signal at all, because your
> > test-program just uses a PTRACE_SINGLESTEP without any "send signal" - so
> > "exit_code" will be zero after the debuggee gets released from the
> > "ptrace_notify()", and the suspect code will never be executed..
> 
> No no, my test case has nothing to do with the extra signal sent in
> do_syscall_trace(). But the test I put at the time:
> 
> -       if (!test_thread_flag(TIF_SYSCALL_TRACE))
> +       if (!test_thread_flag(TIF_SYSCALL_TRACE) &&
> +           !test_thread_flag(TIF_SINGLESTEP))
>                 return;
> 
> will make the code to not execute the "return" (in the single step case)
> and to fall through the point where the signal is sent.


Using the latest version of the patch on do_syscall_trace(), it still
doesn't work unless I remove this test.  If indeed it's supposed to
fall through to receive the proper signal, (ie to single step properly
after an int op), then removing it is wrong, and I won't consider it
anymore.  I also have to use the patch shown below, with a typo-fixed,
to fix the other problem.  I broke it apart from the other because we
might want to consider it seperately right now.

I spent some time speaking to my brother about this.  He has done his
own kernel before for an embedded system.  He was rather frustrated
with me trying to find a reason for why a rather simple change broke
my program.  He told me I couldn't have it both ways.  However I
believe that I don't understand the linux code well enough to make
that conclusion.


> 
> 
> > The problem I think I see (and which the comment alludes to) is that if
> > the controlling process continues the debuggee with a signal, we'll be
> > doing the wrong thing: the code in do_syscall_trace() will take that
> > signal (in "current->exit_code") and send it as a real signal to the
> > debuggee, but since it is debugged, it will be caught (again) by the
> > controlling process, which apparently in the case of Wine gets very
> > confused.
> >
> > So I _think_ the problem happens for the following schenario:
> >  - wine for some reason does a PTRACE_SINGLESTEP over a system call
> >  - after the single-step, wine ends up trying to get the sub-process to
> >    enter a signal handler with ptrace( PTRACE_CONT, ... sig)
> >  - the normal ptrace path (where we trap a signal - see kernel/signal.c
> >    and the "ptrace_stop()" logic) handles this correctly, but
> >    do_syscall_trace() will do a "send_sig()"
> >  - we'll try to handle the signal when returning to the traced process
> >  - now "get_signal_to_deliver()" will invoce the ptrace logic AGAIN, and
> >    call ptrace_stop() for this fake signal, and we'll report a new event
> >    to the controlling process.
> >
> > Does this make sense?
> 
> This might explain what they were seeing, but OTOH it seems that the real
> cause of their problems is related to something else (according to other
> emails on this thread).
> 
> 

Actually, I don't think the vanilla kernel has the code that breaks
those other wine programs.  I just learned of this yesterday and it's
not related.  I believe it's only in fedora core 3 or -ac kernels and 
I use vanilla kernels.


Jesse

--- linux/arch/i386/kernel/ptrace.c	2004-12-29 14:10:34.000000000 -0700
+++ linux-mod/arch/i386/kernel/ptrace.c	2004-12-29 20:50:00.000000000 -0700
@@ -142,18 +142,31 @@
 {
 	long eflags;
 
+	/*
+	 * Always set TIF_SINGLESTEP - this guarantees that 
+	 * we single-step system calls etc.. 
+	 */
 	set_tsk_thread_flag(child, TIF_SINGLESTEP);
+
+	/*
+	 * If TF was already set, don't do anything else
+	 */
 	eflags = get_stack_long(child, EFL_OFFSET);
+	if (eflags & TRAP_FLAG)
+		return;
 	put_stack_long(child, EFL_OFFSET, eflags | TRAP_FLAG);
 	child->ptrace |= PT_DTRACE;
 }
 
 static void clear_singlestep(struct task_struct *child)
 {
+	/* Always clear TIF_SINGLESTEP... */
+	clear_tsk_thread_flag(child, TIF_SINGLESTEP);
+
+	/* But touch TF only if it was set by us.. */
 	if (child->ptrace & PT_DTRACE) {
 		long eflags;
 
-		clear_tsk_thread_flag(child, TIF_SINGLESTEP);
 		eflags = get_stack_long(child, EFL_OFFSET);
 		put_stack_long(child, EFL_OFFSET, eflags & ~TRAP_FLAG);
 		child->ptrace &= ~PT_DTRACE;