ptrace on stopped processes (2.4)

This patch fixes a couple problems with ptrace's interaction with stopped processes on Linux 2.4.

The most significant bug is that gdb cannot attach to a stopped process. Specifically, the wait that follows the PTRACE_ATTACH will block indefinitely.

Another bug is that it is not possible to use PTRACE_DETACH to leave a process stopped, because ptrace ignores SIGSTOPs sent by the tracing process.

This patch is against 2.4.16 on x86. I have tested gdb and strace. After this patch is reviewed, I would be happy to submit an analogous patch for the other platforms, although I cannot test it.

Vic Zandy

@@ -620,9 +620,9 @@
 				continue;
 			current->exit_code = 0;
 
-			/* The debugger continued.  Ignore SIGSTOP.  */
-			if (signr == SIGSTOP)
-				continue;
+			/* The debugger continued. */
+			if (signr == SIGSTOP && current->ptrace & PT_PTRACED)
+				continue; /* ignore SIGSTOP */
 
 			/* Update the siginfo structure.  Is this good?  */
 			if (signr != info.si_signo) {

@@ -89,8 +89,10 @@
 		SET_LINKS(task);
 	}
 	write_unlock_irq(&tasklist_lock);
-
-	send_sig(SIGSTOP, task, 1);
+	if (task->state != TASK_STOPPED)
+		send_sig(SIGSTOP, task, 1);
+	else
+		task->exit_code = SIGSTOP;
 	return 0;
 
 bad:

Also make sure UML still runs.

Jeff

Are you sure this is a bug in ptrace? Maybe gdb and other such programs could notice that a wait is inappropriate in this case and not do it.

(It looks to me like the wait actually does complete when the process is continued.)

Also, is this something that used to work? Or would this be a change in the semantics of ptrace?

Unless I'm missing something (frequently the case), there are two cases here:
(1) the tracer wants to leave the tracee stopped, and (2) the tracer wants the process to continue running in as natural a way as possible, meaning without sending it a SIGCONT (which can cause the SIGCONT signal handler to execute). As things currently stand, we have behavior (2), and (1) is not possible. With your change, we'd have behavior (1), and (2) would not be possible.

To me, (2) seems very important, so I wouldn't want to give it up. Is there some way we can get both?

Although SUBTERFUGUE isn't widely used, I'd be curious to know how it reacts to this patch. (If you're running Debian, there's a package.)

Mike

This is to respond to feedback for the ptrace patch I sent toward the end of december. The original message is below.

From: Alan Cox <[email protected]>

I agree the second term should be wrapped in parens (it is now in the patch below); but isn't that logically equivalent to what I had?

From: Mike Coleman <[email protected]>:

This is a change of semantics at least going back to 2.2.

I agree that the ability to do (2) should be preserved, but I don't see how this patch breaks it; do you have an example?

From: OGAWA Hirofumi <[email protected]>:

I don't agree, it seems to work for me.

I'd still want to check uml and subterfuge, which I'll do after these points are cleared up.

Thanks,
Vic

From: vic <[email protected]>
Subject: [PATCH] ptrace on stopped processes (2.4)
To: [email protected]
Cc: [email protected], [email protected],
[email protected]
Date: Fri, 21 Dec 2001 13:53:32 -0600

This patch fixes a couple problems with ptrace's interaction with stopped processes on Linux 2.4.

The most significant bug is that gdb cannot attach to a stopped process. Specifically, the wait that follows the PTRACE_ATTACH will block indefinitely.

Another bug is that it is not possible to use PTRACE_DETACH to leave a process stopped, because ptrace ignores SIGSTOPs sent by the tracing process.

This patch is against 2.4.16 on x86. I have tested gdb and strace. After this patch is reviewed, I would be happy to submit an analogous patch for the other platforms, although I cannot test it.

Vic Zandy

@@ -620,9 +620,9 @@
                                continue;
                        current->exit_code = 0;
 
-                       /* The debugger continued.  Ignore SIGSTOP.  */
-                       if (signr == SIGSTOP)
-                               continue;
+                       /* The debugger continued. */
+                       if (signr == SIGSTOP && (current->ptrace & PT_PTRACED))
+                               continue; /* ignore SIGSTOP */
 
                        /* Update the siginfo structure.  Is this good?  */
                        if (signr != info.si_signo) {

@@ -89,8 +89,10 @@
                SET_LINKS(task);
        }
        write_unlock_irq(&tasklist_lock);
-
-       send_sig(SIGSTOP, task, 1);
+       if (task->state != TASK_STOPPED)
+               send_sig(SIGSTOP, task, 1);
+       else
+               task->exit_code = SIGSTOP;
        return 0;
 
 bad:

I tested the following on linux-2.4.16 + your_patch:

#include <stdio.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/ptrace.h>
#include <unistd.h>

int main(int argc, char *argv[])

{
	pid_t pid;
	int ret, status;

	pid = fork();
	if (pid == -1)
		exit(1);
	if (pid == 0) {
		raise(SIGSTOP);
		while (1)
			;
		_exit(1);
	}

	ret = waitpid(pid, &status, WUNTRACED);
	if (ret == -1) {
		perror("waitpid (1)");
		exit(1);
	}

	ret = ptrace(PTRACE_ATTACH, pid, NULL, NULL);
	if (ret == -1) {
		perror("PTRACE_ATTACH");
		exit(1);
	}
	ret = waitpid(pid, &status, 0);
	if (ret == -1) {
		perror("waitpid (2)");
		exit(1);
	}
	ret = ptrace(PTRACE_KILL, pid, NULL, NULL);
	if (ret == -1) {
		perror("PTRACE_KILL");
		exit(1);
	}

	return 0;
}

Test result:

hirofumi@devron (ptrace)[1111]$ ps ax|grep ptrace
688 tty1 S 0:00 grep ptrace
hirofumi@devron (ptrace)[1112]$ ls
ptrace ptrace.c
hirofumi@devron (ptrace)[1113]$ ./ptrace hirofumi@devron (ptrace)[1114]$ ps ax|grep ptrace
691 tty1 R 0:04 ./ptrace
693 tty1 S 0:00 grep ptrace
hirofumi@devron (ptrace)[1115]$

Do I misunderstand something?

OGAWA Hirofumi <[email protected]>

Okay. Is it at least backward compatible? Or are some tools expected to break?

No, I was just going by reading the kernel code. Can you describe how each of
(1) and (2) are accomplished by the ptracing program (with your patch)?

Mike

Now I see the problem with PTRACE_KILL. Thanks for the example.

I'm looking into it. I need to justify the quoted portion of the patch or find a better way to get its effect.

In the meantime, the problem could be fixed by changing the PTRACE_KILL implementation to call send_sig instead of setting exit_code. How does that strike people?

More generally, could someone explain to me why there are two mechanisms for interprocess signaling within the kernel, exit_code and send_sig?

Thanks,
Vic

PTRACE_SYSCALL, PTRACE_CONT, and PTRACE_SINGLESTEP can't send a signal by the same reason. Please read the do_signal().

OGAWA Hirofumi <[email protected]>

I've read that function, but I don't see why it would not get along with my suggestion to send SIGKILL rather than set exit_code to implement PTRACE_KILL.

No doubt I can be rather thick; in this case, induction doesn't help me.

kill(pid, SIGKILL) != ptrace(PTRACE_KILL, pid, NULL, NULL).

Whether the same effect as kill() is required for PTRACE_KILL is the problem which is unrelated to this problem. If so, please argue on another thread.

And If PTRACE_SYSCALL, PTRACE_CONT, and PTRACE_SINGLESTEP can send the signal, PTRACE_KILL also work.

BTW, did you read my first email? What do you think of my suggestion?

In an example,

ptrace_attach(),

	if (task->p_pptr != current) {
		REMOVE_LINKS(task);
		task->p_pptr = current;
		SET_LINKS(task);
	}
	write_unlock_irq(&tasklist_lock);

	stopped = (task->state == TASK_STOPPED);
	send_sig(SIGSTOP, task, 1);
	if (stopped)
		wake_up_process(task);

	return 0;

Note, this code isn't investigating at all.

OGAWA Hirofumi <[email protected]>

I'm sorry for this long message and my delayed replies to everyone's feedback.

Some tools may break: those that (1) attach to a stopped process or (2) detach and leave the process stopped. However, I expect the maintainers of such tools will appreciate this patch, as it would simplify the way they do these things now.

First, a tool that wants to attach to a stopped process needs some tricks. For example, if you SIGSTOP any process and then try to attach gdb to it, gdb will hang. The problem is that when the process is already stopped, the call to wait after PTRACE_ATTACH can block indefinitely because the process exit_code is left at 0 (so wait blocks) but the process is not awake. Currently tools need to detect this condition and manually wind the process past the pending SIGSTOP. gdb doesn't do that.

Second, our tool Paradyn (www.paradyn.org) sometimes wants to detach and leave a process stopped; I am not aware of any other tool that needs to do this, but it's not a strange request. The natural way you would do this with ptrace (PTRACE_DEATCH with SIGSTOP) does not work. We found that we had to read the kernel to figure out how to do it reliably on Linux (i.e., to understand the race conditions). The algorithm that we eventually settled on is non-intuitive and involves calling sleep to avoid a race we saw no other way around.

Again, tools that work around either of these problem, like ours, will be affected by this patch: they will need to retire their kludges and instead do what they probably wanted to do in the first place.

Offline I corresponded with OGAWA Hirofumi and he gave me an approach to the PTRACE_KILL issue that I have been slow to understand. Now when attaching, I always send the SIGSTOP, but wake the process if it was already stopped. I leave the exit_code the way it was.

Mike Coleman is concerned about SIGCONT and whether this patch prevents processes from being continued naturally, without handling a SIGCONT. Nothing has changed in this aspect. If the tracing process wants to continue a process, it just either does PTRACE_CONT or PTRACE_DETACH as usual.

As promised, I will check UML and Subterfuge; I haven't had the time. If anyone can alert me to potentially problematic operations in these programs, that would very helpful.

The updated patch is below.

Thanks,
Vic

@@ -54,6 +54,8 @@
 
 int ptrace_attach(struct task_struct *task)
 {
+	int stopped;
+
 	task_lock(task);
 	if (task->pid <= 1)
 		goto bad;

@@ -90,7 +92,13 @@
 	}
 	write_unlock_irq(&tasklist_lock);
 
+	stopped = (task->state == TASK_STOPPED);
 	send_sig(SIGSTOP, task, 1);
+	/* If it was stopped when we got here,
+	   clear the pending SIGSTOP. */
+	if (stopped)
+		wake_up_process(task);
+
 	return 0;
 
 bad:

@@ -620,9 +620,9 @@
 				continue;
 			current->exit_code = 0;
 
-			/* The debugger continued.  Ignore SIGSTOP.  */
-			if (signr == SIGSTOP)
-				continue;
+			/* The debugger continued. */
+			if (signr == SIGSTOP && (current->ptrace & PT_PTRACED))
+				continue; /* ignore SIGSTOP */
 
 			/* Update the siginfo structure.  Is this good?  */
 			if (signr != info.si_signo) {

This is a repost of the ptrace patch to 2.4 kernels we've discussed in recent months.

Since the last post, I have updated it to linux 2.4.18 (no changes) and tested it with subterfuge and uml.

Subterfuge seems to be unaffected.

UML needs minor modifications; I've discussed them with Jeff Dike and (I believe) he is happy.

I believe I have addressed everyone's concerns.

The patch fixes these two bugs:

1. gdb and other tools cannot attach to a stopped process. The wait that follows the PTRACE_ATTACH will block indefinitely.

2. It is not possible to use PTRACE_DETACH to leave a process stopped, because ptrace ignores SIGSTOPs sent by the tracing process.

Vic

@@ -54,6 +54,7 @@
 
 int ptrace_attach(struct task_struct *task)
 {
+	int stopped;
 	task_lock(task);
 	if (task->pid <= 1)
 		goto bad;

@@ -90,7 +91,13 @@
 	}
 	write_unlock_irq(&tasklist_lock);
 
+	stopped = (task->state == TASK_STOPPED);
 	send_sig(SIGSTOP, task, 1);
+	/* If it was stopped when we got here,
+	   clear the pending SIGSTOP. */
+	if (stopped)
+		wake_up_process(task);
+
 	return 0;
 
 bad:

@@ -620,9 +620,9 @@
 				continue;
 			current->exit_code = 0;
 
-			/* The debugger continued.  Ignore SIGSTOP.  */
-			if (signr == SIGSTOP)
-				continue;
+			/* The debugger continued. */
+			if (signr == SIGSTOP && current->ptrace & PT_PTRACED)
+				continue; /* ignore SIGSTOP */
 
 			/* Update the siginfo structure.  Is this good?  */
 			if (signr != info.si_signo) {