Merge branch 'ptrace-sane-block' into 'master'

Ptrace sane block

See merge request redox-os/kernel!129

src/ptrace.rs

@@ -13,7 +13,7 @@ use crate::{
         }
     },
     common::unique::Unique,
-    context::{self, signal, Context, ContextId, Status},
+    context::{self, signal, Context, ContextId},
     event,
     scheme::proc,
     sync::WaitCondition,
@@ -73,6 +73,12 @@ impl SessionData {
         });
     }
 
+    /// Returns true if the breakpoint is reached, or if there isn't a
+    /// breakpoint
+    pub fn is_reached(&self) -> bool {
+        self.breakpoint.as_ref().map(|b| b.reached).unwrap_or(false)
+    }
+
     /// Used for getting the flags in fevent
     pub fn session_fevent_flags(&self) -> EventFlags {
         let mut flags = EventFlags::empty();
@@ -106,7 +112,11 @@ impl Session {
         F: FnOnce(&Session) -> Result<T>,
     {
         let sessions = sessions();
-        let session = sessions.get(&pid).ok_or(Error::new(ENODEV))?;
+        let session = sessions.get(&pid).ok_or_else(|| {
+            println!("session doesn't exist - returning ENODEV.");
+            println!("can this ever happen?");
+            Error::new(ENODEV)
+        })?;
 
         callback(session)
     }
@@ -154,6 +164,18 @@ pub fn close_session(pid: ContextId) {
     if let Some(session) = sessions_mut().remove(&pid) {
         session.tracer.notify();
         session.tracee.notify();
+    }
+}
+
+/// Wake up the tracer to make sure it catches on that the tracee is dead. This
+/// is different from `close_session` in that it doesn't actually close the
+/// session, and instead waits for the file handle to be closed, where the
+/// session will *actually* be closed. This is partly to ensure ENOSRCH is
+/// returned rather than ENODEV (which occurs when there's no session - should
+/// never really happen).
+pub fn close_tracee(pid: ContextId) {
+    if let Some(session) = sessions().get(&pid) {
+        session.tracer.notify();
 
         let data = session.data.lock();
         proc_trigger_event(data.file_id, EVENT_READ);
@@ -244,13 +266,6 @@ pub fn wait(pid: ContextId) -> Result<()> {
         }
     }
 
-    let contexts = context::contexts();
-    let context = contexts.get(pid).ok_or(Error::new(ESRCH))?;
-    let context = context.read();
-    if let Status::Exited(_) = context.status {
-        return Err(Error::new(ESRCH));
-    }
-
     Ok(())
 }
 

src/scheme/proc.rs

@@ -345,6 +345,7 @@ impl Scheme for ProcScheme {
                     })?,
                     RegsKind::Int => try_stop_context(info.pid, |context| match unsafe { ptrace::regs_for(&context) } {
                         None => {
+                            assert!(!context.running, "try_stop_context is broken, clearly");
                             println!("{}:{}: Couldn't read registers from stopped process", file!(), line!());
                             Err(Error::new(ENOTRECOVERABLE))
                         },
@@ -365,27 +366,45 @@ impl Scheme for ProcScheme {
                 Ok(len)
             },
             Operation::Trace => {
+                let mut handles = self.handles.write();
+                let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?;
+                let data = handle.data.trace_data().expect("operations can't change");
+
+                // Wait for event
+                if handle.info.flags & O_NONBLOCK != O_NONBLOCK {
+                    ptrace::wait(handle.info.pid)?;
+                }
+
+                // Check if context exists
+                with_context(handle.info.pid, |_| Ok(()))?;
+
+                // Read events
                 let slice = unsafe {
                     slice::from_raw_parts_mut(
                         buf.as_mut_ptr() as *mut PtraceEvent,
                         buf.len() / mem::size_of::<PtraceEvent>()
                     )
                 };
-                let read = ptrace::Session::with_session(info.pid, |session| {
-                    Ok(session.data.lock().recv_events(slice))
+                let (read, reached) = ptrace::Session::with_session(info.pid, |session| {
+                    let mut data = session.data.lock();
+                    Ok((data.recv_events(slice), data.is_reached()))
                 })?;
 
-                // Won't context switch, don't worry about the locks
-                let mut handles = self.handles.write();
-                let handle = handles.get_mut(&id).ok_or(Error::new(EBADF))?;
-                let data = handle.data.trace_data().expect("operations can't change");
-
+                // Save child processes in a list of processes to restart
                 for event in &slice[..read] {
                     if event.cause == PTRACE_EVENT_CLONE {
                         data.clones.push(ContextId::from(event.a));
                     }
                 }
 
+                // If there are no events, and breakpoint isn't reached, we
+                // must not have waited.
+                if read == 0 && !reached {
+                    assert!(handle.info.flags & O_NONBLOCK == O_NONBLOCK, "wait woke up spuriously??");
+                    return Err(Error::new(EAGAIN));
+                }
+
+                // Return read events
                 Ok(read * mem::size_of::<PtraceEvent>())
             }
         }
@@ -470,15 +489,12 @@ impl Scheme for ProcScheme {
                 let op = u64::from_ne_bytes(bytes);
                 let op = PtraceFlags::from_bits(op).ok_or(Error::new(EINVAL))?;
 
-                let should_continue = !op.contains(PTRACE_FLAG_WAIT) || op.intersects(PTRACE_STOP_MASK);
-
                 // Set next breakpoint
                 ptrace::Session::with_session(info.pid, |session| {
-                    if op.intersects(PTRACE_STOP_MASK) {
-                        session.data.lock().set_breakpoint(Some(op));
-                    } else if should_continue {
-                        session.data.lock().set_breakpoint(None);
-                    }
+                    session.data.lock().set_breakpoint(
+                        Some(op)
+                            .filter(|op| op.intersects(PTRACE_STOP_MASK | PTRACE_EVENT_MASK))
+                    );
                     Ok(())
                 })?;
 
@@ -497,25 +513,17 @@ impl Scheme for ProcScheme {
                     })?;
                 }
 
-                // Continue execution, if requested
-                if should_continue {
-                    // disable the ptrace_stop flag, which is used in some cases
-                    with_context_mut(info.pid, |context| {
-                        context.ptrace_stop = false;
-                        Ok(())
-                    })?;
+                // disable the ptrace_stop flag, which is used in some cases
+                with_context_mut(info.pid, |context| {
+                    context.ptrace_stop = false;
+                    Ok(())
+                })?;
 
-                    // and notify the tracee's WaitCondition, which is used in other cases
-                    ptrace::Session::with_session(info.pid, |session| {
-                        session.tracee.notify();
-                        Ok(())
-                    })?;
-                }
-
-                // And await the tracee, if requested
-                if op.contains(PTRACE_FLAG_WAIT) || info.flags & O_NONBLOCK != O_NONBLOCK {
-                    ptrace::wait(info.pid)?;
-                }
+                // and notify the tracee's WaitCondition, which is used in other cases
+                ptrace::Session::with_session(info.pid, |session| {
+                    session.tracee.notify();
+                    Ok(())
+                })?;
 
                 Ok(mem::size_of::<u64>())
             }

src/syscall/process.rs

@@ -1065,6 +1065,8 @@ pub fn fexec(fd: FileHandle, arg_ptrs: &[[usize; 2]], var_ptrs: &[[usize; 2]]) -
 }
 
 pub fn exit(status: usize) -> ! {
+    ptrace::breakpoint_callback(PTRACE_STOP_EXIT, Some(ptrace_event!(PTRACE_STOP_EXIT, status)));
+
     {
         let context_lock = {
             let contexts = context::contexts();
@@ -1072,8 +1074,6 @@ pub fn exit(status: usize) -> ! {
             Arc::clone(&context_lock)
         };
 
-        ptrace::breakpoint_callback(PTRACE_STOP_EXIT, Some(ptrace_event!(PTRACE_STOP_EXIT, status)));
-
         let mut close_files = Vec::new();
         let pid = {
             let mut context = context_lock.write();
@@ -1152,7 +1152,7 @@ pub fn exit(status: usize) -> ! {
         }
 
         // Alert any tracers waiting of this process
-        ptrace::close_session(pid);
+        ptrace::close_tracee(pid);
 
         if pid == ContextId::from(1) {
             println!("Main kernel thread exited with status {:X}", status);