public inbox for frysk@sourceware.org
 help / color / mirror / Atom feed
* Reset breakpoints on fork
@ 2008-04-25 17:52 Mark Wielaard
  2008-04-29 18:25 ` Reset breakpoints on fork (vfork followup) Mark Wielaard
  2008-05-01 22:49 ` Reset breakpoints on fork Roland McGrath
  0 siblings, 2 replies; 3+ messages in thread
From: Mark Wielaard @ 2008-04-25 17:52 UTC (permalink / raw)
  To: frysk; +Cc: Roland McGrath

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

Hi,

This fixes a long standing issue with breakpoints being left in the
inferior on a fork. We used to get away with it most times since often a
fork is followed by an exec() which would just clear everything. But
there are actually single threaded applications that use fork as a way
to have a poor mans clone().

Roland, I CCed you in the hope that you could maybe point out some nice
way now or in the future to do this. The patch itself might not be very
interesting to you. The way we model Tasks and events means we do this
in three stages. First we get a fork event and capture all the
(software) breakpoint addresses, then we already create the Proc and
then the main Task based on the pid given in the event, then we wait for
an (stopped) event on this now Task, then we go over all the breakpoints
left and patch in the original instructions again.

This of course isn't very efficient since I assume the kernel could help
us out here since it know the pages we touched through ptrace poke to
set the breakpoints in the first place. If there was some way to have an
event from the kernel where we got the forking and forked tasks in a
quiescent state (at the same time) and then could inspect both and tell
the kernel whether or not to copy the modified or original code pages
that would be much better. Bonus points for having advanced knowledge of
a pending exec event, which would wipe away any changes we do anyway
(although I don't see how this would be possible).

frysk-core/frysk/proc/live/ChangeLog
2008-04-22  Mark Wielaard  <mwielaard@redhat.com>
    
     * Breakpoint.java (reset): Make package private.
     (cloneForProc): New method.
     * BreakpointAddresses.java (cloneForProc): New method.
     (clearAllBreakpoints): New method.
     * LinuxPtraceProc.java (LinuxPtraceProc(Task,ProcessIdentifier)):
     Set breakpoints through forkingProc.breakpoints.cloneForProc().
     (breakpoints): Make package private.
     * LinuxPtraceTask.java (LinuxPtraceTask(LinuxPtraceTask,
     LinuxPtraceProc,TaskAttachedObserverXXX)): Set currentISA.
     * LinuxPtraceTaskState.java
     (StartMainTask.wantToDetach.handleAttach): Clear breakpoints.
     (StartMainTask.wantToDetach.StoppedEvent): Likewise.
     * TestTaskObserverCode.java (testCodeOverFork): Resolve bug #5331.

No regressions on fedora 8 x86 and x86_64. The testCodeOverFork now
passes.

Committed and pushed,

Mark

[-- Attachment #2: breakpoints-on-fork.patch --]
[-- Type: text/x-patch, Size: 7932 bytes --]

diff --git a/frysk-core/frysk/proc/live/Breakpoint.java b/frysk-core/frysk/proc/live/Breakpoint.java
index 0372b6d..54ea21f 100644
--- a/frysk-core/frysk/proc/live/Breakpoint.java
+++ b/frysk-core/frysk/proc/live/Breakpoint.java
@@ -175,8 +175,11 @@ public class Breakpoint implements Comparable
 
   /**
    * Actually removes the breakpoint.
+   * Package private for BreakpointAddresses.clearBreakpoints(),
+   * all other callers should use <code>remove()</code>.
+   *
    */
-  private void reset(Task task)
+  void reset(Task task)
   {
       ByteBuffer buffer = ((LinuxPtraceTask)task).getRawMemory();
     buffer.position(address);
@@ -379,4 +382,22 @@ public class Breakpoint implements Comparable
     return this.getClass().getName() + "[proc=" + proc
       + ", address=0x" + Long.toHexString(address) + "]";
   }
+
+  /**
+   * Makes a clone of this Breakpoint, but for a particular Proc. This
+   * is used by the BreakPointAddresses class to make a clone of all
+   * breakpoints set on a froked proc. First makes sure to have the
+   * canonical (installed) breakpoint. Then makes a copy and adds the
+   * original instruction. See BreakPointAddresses cloneForProc() and
+   * clearAllAddresses for more explanation.
+   */
+  Breakpoint cloneForProc(Proc proc)
+  {
+    // Make sure to have the canonical (installed) breakpoint.
+    // Then make a copy and add the original instruction.
+    Breakpoint breakpoint = create(this.getAddress(), this.getProc());
+    Breakpoint result = new Breakpoint(breakpoint.address, proc);
+    result.origInstruction = breakpoint.origInstruction;
+    return result;
+  }
 }
diff --git a/frysk-core/frysk/proc/live/BreakpointAddresses.java b/frysk-core/frysk/proc/live/BreakpointAddresses.java
index 1fb127a..4564d40 100644
--- a/frysk-core/frysk/proc/live/BreakpointAddresses.java
+++ b/frysk-core/frysk/proc/live/BreakpointAddresses.java
@@ -236,4 +236,41 @@ class BreakpointAddresses
     map.clear();
     breakpoints.clear();
   }
+
+  /**
+   * Sets up a BreakpointAddresses map for a forked proc.  This sets
+   * the list of breakpoint addresses (plus original instructions) to
+   * be reset later with clearAllBreakpoints(). See comments in
+   * LinuxPtraceProc, LinuxPtraceTask and
+   * LinuxPtraceTaskState.StartMainTask.wantToDetach for the sequence
+   * of events.
+   */
+  BreakpointAddresses cloneForProc(Proc proc)
+  {
+    BreakpointAddresses addresses = new BreakpointAddresses(proc);
+    Iterator it = breakpoints.iterator();
+    while (it.hasNext())
+      {
+	Breakpoint breakpoint = (Breakpoint) it.next();
+	breakpoint = breakpoint.cloneForProc(proc);
+	addresses.breakpoints.add(breakpoint);
+      }
+    return addresses;
+  }
+
+  /**
+   * Actually resets the breakpoints in the breakpoint addresses left
+   * by cloneForProc(). Called when the main task of the forked proc
+   * is ready in LinuxPtraceTaskState.StartMainTask.wantToDetach.
+   */
+  void clearAllBreakpoints()
+  {
+    Iterator it = breakpoints.iterator();
+    while (it.hasNext())
+      {
+        Breakpoint breakpoint = (Breakpoint) it.next();
+        breakpoint.reset(proc.getMainTask());
+      }
+    removeAllCodeObservers();
+  }
 }
diff --git a/frysk-core/frysk/proc/live/LinuxPtraceProc.java b/frysk-core/frysk/proc/live/LinuxPtraceProc.java
index 14e1e00..c9a22f0 100644
--- a/frysk-core/frysk/proc/live/LinuxPtraceProc.java
+++ b/frysk-core/frysk/proc/live/LinuxPtraceProc.java
@@ -97,7 +97,8 @@ public class LinuxPtraceProc extends LiveProc {
     public LinuxPtraceProc(Task task, ProcessIdentifier fork) {
 	super(task, fork);
 	this.newState = LinuxPtraceProcState.initial(true);
-	this.breakpoints = new BreakpointAddresses(this);
+	LinuxPtraceProc forkingProc = (LinuxPtraceProc) task.getProc();
+	this.breakpoints = forkingProc.breakpoints.cloneForProc(this);
 	this.watchpoints = new WatchpointAddresses(this);
 	((LinuxPtraceHost)getHost()).addProc(this);
     }
@@ -833,9 +834,13 @@ public class LinuxPtraceProc extends LiveProc {
     }
 
     /**
-     * XXX: Should not be public.
+     * The addresses on which we have installed breakpoints (with
+     * associated Tasks and TaskObserver.Code listeners). Package
+     * private for access in LinuxPtraceTask constructor that creates
+     * a forked task and the LinuxPtraceTaskState.MainTask.wantToDetach
+     * class to clearthem. See the comments there.
      */
-    public final BreakpointAddresses breakpoints;
+    final BreakpointAddresses breakpoints;
     
     // List of available addresses for out of line stepping.
     // Used a lock in getOutOfLineAddress() and doneOutOfLine().
diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTask.java b/frysk-core/frysk/proc/live/LinuxPtraceTask.java
index 397f69b..ede0c2b 100644
--- a/frysk-core/frysk/proc/live/LinuxPtraceTask.java
+++ b/frysk-core/frysk/proc/live/LinuxPtraceTask.java
@@ -110,6 +110,19 @@ public class LinuxPtraceTask extends LiveTask {
 	((LinuxPtraceHost)proc.getHost()).putTask(tid, this);
 	((LinuxPtraceProc)proc).addTask(this);
 	newState = LinuxPtraceTaskState.mainState();
+	// See the various FIXMEs below around the isa, ISA,
+	// currentISA, getISA(), getIsaFIXME() and clearIsa(). The
+	// current design is such that the isa isn't a constant of a
+	// proc (actually task), which means we cannot remove
+	// breakpoint instructions from the fork (the breakpoint
+	// instruction is an intrinsic of the isa).  Luckily we know
+	// that the fork will have the same isa as the task it forked
+	// from, so we explicitly set it now. We cannot do the
+	// breakpoint resetting here (the LinuxPtraceTask is created,
+	// but not fully setup yet), we do that in the
+	// LinuxPtraceTaskState.StartMainTask.wantToDetach class
+	// handlers.
+	currentISA = forkingTask.currentISA;
 	if (attached != null) {
 	    TaskObservation ob = new TaskObservation(this, attachedObservers,
 						     attached, true) {
diff --git a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
index 50ebabd..3f9b058 100644
--- a/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
+++ b/frysk-core/frysk/proc/live/LinuxPtraceTaskState.java
@@ -459,13 +459,21 @@ abstract class LinuxPtraceTaskState extends State {
 	    new StartMainTask("wantToDetach") {
 		LinuxPtraceTaskState handleAttach(LinuxPtraceTask task) {
 		    fine.log("handleAttach", task); 
-		    ((LinuxPtraceProc)task.getProc())
-			.performTaskAttachCompleted(task);
+		    LinuxPtraceProc proc = (LinuxPtraceProc) task.getProc();
+		    // Clear left over breakpoints here. They were
+		    // captured when the Proc was created.
+		    proc.breakpoints.clearAllBreakpoints();
+		    proc.performTaskAttachCompleted(task);
 		    return StartMainTask.wantToAttach;
 		}
 		LinuxPtraceTaskState handleStoppedEvent(LinuxPtraceTask task,
 							Signal signal) {
 		    if (signal == Signal.STOP || signal == Signal.TRAP) {
+		        LinuxPtraceProc proc;
+		        proc = (LinuxPtraceProc) task.getProc();
+		        // Clear left over breakpoints here. They were
+		        // captured when the Proc was created.
+		        proc.breakpoints.clearAllBreakpoints();
 			task.initializeAttachedState();
 			if (task.notifyForkedOffspring() > 0) {
 			    return StartMainTask.detachBlocked;
diff --git a/frysk-core/frysk/proc/live/TestTaskObserverCode.java b/frysk-core/frysk/proc/live/TestTaskObserverCode.java
index db209b8..f7e6f6a 100644
--- a/frysk-core/frysk/proc/live/TestTaskObserverCode.java
+++ b/frysk-core/frysk/proc/live/TestTaskObserverCode.java
@@ -886,9 +886,6 @@ public class TestTaskObserverCode extends TestLib
   // after the fork.
   public void testCodeOverFork() throws Exception
   {
-    if (unresolved(5331))
-      return;
-
     String[] argv = {getExecPath ("funit-fib-fork"), "2"};
     child = null;
     DaemonBlockedAtEntry child = new DaemonBlockedAtEntry(argv);

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Reset breakpoints on fork (vfork followup)
  2008-04-25 17:52 Reset breakpoints on fork Mark Wielaard
@ 2008-04-29 18:25 ` Mark Wielaard
  2008-05-01 22:49 ` Reset breakpoints on fork Roland McGrath
  1 sibling, 0 replies; 3+ messages in thread
From: Mark Wielaard @ 2008-04-29 18:25 UTC (permalink / raw)
  To: frysk; +Cc: Roland McGrath

Hi,

On Tue, 2008-04-22 at 18:52 +0200, Mark Wielaard wrote:
> Roland, I CCed you in the hope that you could maybe point out some nice
> way now or in the future to do this. The patch itself might not be very
> interesting to you. The way we model Tasks and events means we do this
> in three stages. First we get a fork event and capture all the
> (software) breakpoint addresses, then we already create the Proc and
> then the main Task based on the pid given in the event, then we wait for
> an (stopped) event on this now Task, then we go over all the breakpoints
> left and patch in the original instructions again.
> 
> This of course isn't very efficient since I assume the kernel could help
> us out here since it know the pages we touched through ptrace poke to
> set the breakpoints in the first place. If there was some way to have an
> event from the kernel where we got the forking and forked tasks in a
> quiescent state (at the same time) and then could inspect both and tell
> the kernel whether or not to copy the modified or original code pages
> that would be much better. Bonus points for having advanced knowledge of
> a pending exec event, which would wipe away any changes we do anyway
> (although I don't see how this would be possible).

A small followup without code, but hopefully enough information for
someone to design the right setup to make it happen. There is also still
the outstanding issue of frysk not handling vfork.
http://sourceware.org/bugzilla/show_bug.cgi?id=1583

Getting events for vfork is not hard. In Ptrace.cxx we should also
request PTRACE_O_TRACEVFORK and in Wait.cxx we should handle the
PTRACE_EVENT_VFORK. This however does not bring us full vfork support.
vfork() is a special kind of fork() that blocks the parent till the
child process it just vforked calls exec(), the child process is not
allowed to do much (see the vfork manual page) but between the vfork
call and the exec call the child does share its memory pages with its
parent. This makes our current approach to handling fork() not work for
vfork(). If we alter any breakpoints in this new child before it calls
exec() it will alter the parent's memory pages also. So if we alter any
(software) breakpoints we need to track the state changes in the parent
(which will be blocked itself). Luckily the kernel can give us some
events to help with this all. In Ptrace.cxx we must request
PTRACE_O_TRACEVFORKDONE and in Wait.cxx we must also handle
PTRACE_EVENT_VFORK_DONE. Then we must put both child and parent into new
states and track any breakpoint manipulation.

I don't immediately see a clean way to handle fork and vfork the same
way even though they are so similar to the user. It would be nice to
have a way to express the TaskObserver.Forked in a way that the user of
such an observer doesn't have to care about what kind of fork it is
about. And to have it in such a way that the user of the observer can
easily inspect/manipulate both parent and child. Currently it must
handle two different events for parent and child, but it cannot assume
that either the parent or the child is in a state which allows any
inspection/manipulation. 

I did make a little change to the current vfork test to have the right
counts (but didn't resolve it since the code isn't there yet, I tested
it by doing the above changes to Wait.cxx and Ptrace.cxx and pretending
the vfork is just another fork, but obviously it breaks when also doing
breakpoint Code observers at the same time).

2008-04-28  Mark Wielaard  <mwielaard@redhat.com>

        * TestTaskForkedObserver.java (testTaskVforkObserver): 
        Correct counts.

Cheers,

Mark

Test patch:

diff --git a/frysk-core/frysk/proc/TestTaskForkedObserver.java b/frysk-core/frys
index 5990546..ada14bd 100644
--- a/frysk-core/frysk/proc/TestTaskForkedObserver.java
+++ b/frysk-core/frysk/proc/TestTaskForkedObserver.java
@@ -89,10 +89,10 @@ public class TestTaskForkedObserver extends TestLib {
 
        assertEquals("number of child processes created",
                     1, forkObserver.forkCount);
-       assertEquals("number of child processes destroyed",
-                    1, forkObserver.terminatedCount);
-       assertEquals("number of times fork observer added",
-                    2, forkObserver.addedCount());
+       assertEquals("number of exits (includes initial process)",
+                    2, forkObserver.terminatedCount);
+       assertEquals("number of times fork+terminated observer added",
+                    2 + 2, forkObserver.addedCount());
     }
 
     private void runForkTest(ForkObserver forkObserver, String[] argv) {


Not-committed-and-wrong! fvork handling patch:

diff --git a/frysk-sys/frysk/sys/cni/Wait.cxx b/frysk-sys/frysk/sys/cni/Wait.cxx
index f902bb0..ce267d5 100644
--- a/frysk-sys/frysk/sys/cni/Wait.cxx
+++ b/frysk-sys/frysk/sys/cni/Wait.cxx
@@ -102,6 +102,9 @@ logWait(frysk::rsl::Log* logger, pid_t pid, int status, int 
       case PTRACE_EVENT_FORK:
        wif_name = "WIFSTOPPED/FORK";
        break;
+      case PTRACE_EVENT_VFORK:
+       wif_name = "WIFSTOPPED/VFORK";
+       break;
       case PTRACE_EVENT_EXIT:
        wif_name = "WIFSTOPPED/EXIT";
        break;
@@ -168,6 +171,7 @@ processStatus(frysk::sys::ProcessIdentifier* pid, int status
       }
       break;
     case PTRACE_EVENT_FORK:
+    case PTRACE_EVENT_VFORK:
       try {
        // The event message contains the process-ID of the new
        // process.
diff --git a/frysk-sys/frysk/sys/ptrace/cni/Ptrace.cxx b/frysk-sys/frysk/sys/ptr
index 4aa12eb..5fa1654 100644
--- a/frysk-sys/frysk/sys/ptrace/cni/Ptrace.cxx
+++ b/frysk-sys/frysk/sys/ptrace/cni/Ptrace.cxx
@@ -137,7 +137,7 @@ frysk::sys::ptrace::Ptrace::optionTraceClone() {
 }
 jlong
 frysk::sys::ptrace::Ptrace::optionTraceFork() {
-  return PTRACE_O_TRACEFORK;
+  return PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK;
 }
 jlong
 frysk::sys::ptrace::Ptrace::optionTraceExit() {


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Reset breakpoints on fork
  2008-04-25 17:52 Reset breakpoints on fork Mark Wielaard
  2008-04-29 18:25 ` Reset breakpoints on fork (vfork followup) Mark Wielaard
@ 2008-05-01 22:49 ` Roland McGrath
  1 sibling, 0 replies; 3+ messages in thread
From: Roland McGrath @ 2008-05-01 22:49 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: frysk

> Roland, I CCed you in the hope that you could maybe point out some nice
> way now or in the future to do this. 

There is no great magic to offer here, but I will be sure to keep this
wrinkle of the problem in mind for the future.

> This of course isn't very efficient since I assume the kernel could help
> us out here since it know the pages we touched through ptrace poke to
> set the breakpoints in the first place. 

In fact, it doesn't really.  There is no trail of this per se any more than
there is a trail of what memory writes the process itself made.  The
breakpoints normally go in read-only text pages, which did get COW'd when
you wrote them via ptrace.  It's probably possible to grovel around in the
VM data structures and figure out which read-only pages are COW copies.
But nothing distinguishes your pokes from the process itself doing
mprotect, then write to cause COW of a file-backed page, then mprotect back
to read-only.  For example, RELRO areas will always look that same way.

> If there was some way to have an event from the kernel where we got the
> forking and forked tasks in a quiescent state (at the same time) and then
> could inspect both and tell the kernel whether or not to copy the
> modified or original code pages that would be much better. 

Sorry, quiescent does not include "half-created".  When the child
exists, its address space has been populated.  fork is an atomic
operation.  Consider thread A in the parent running along while thread B
forks a child; then A calls munmap, etc.  The fork happens either before
or after anything A did.

> Bonus points for having advanced knowledge of a pending exec event, which
> would wipe away any changes we do anyway (although I don't see how this
> would be possible).

It's a mere matter of interpreting the machine code from the PC on,
solving the halting problem, etc.  ;-)

For advanced knowledge that an exec is likely, there is knowing that
it was a vfork.  Then an exec or an exit is inevitably entailed before
the parent runs again.  That's it.

Back at the dawn of time, one big idea was that fancy new VM tricks
would be the bee's knees for breakpoints.  VM tricks have great sex
appeal, but will be much deep voodoo to implement, and probably at least
slightly mind-bending to use (maybe true of sex appeal in general ;->).
The original thinking was to rely on this for breakpoints vs threads,
but that was before we clued in to the instruction-copying technique.
It might still be in the cards one day and a big win for e.g. per-thread
breakpoints, but it remains to be seen and is certainly a long way off
(after most of the rest of anything else anticipated for new kernel
features).  Much of that sex appeal is how clean and nice it can be for
issues like the one you are concerned with here.  But we'll be admiring
it from afar for a long time to come.


Thanks,
Roland

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2008-04-30  3:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-04-25 17:52 Reset breakpoints on fork Mark Wielaard
2008-04-29 18:25 ` Reset breakpoints on fork (vfork followup) Mark Wielaard
2008-05-01 22:49 ` Reset breakpoints on fork Roland McGrath

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).