From 7bca8c9c117e026b1c9381570edf71686cfdabf0 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 12 Aug 2024 11:35:09 +0200 Subject: [PATCH 1/4] Handle non-SIGSTOP signals --- src/linux/ptrace_dumper.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index 40bb1800..c69a79f6 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -143,8 +143,31 @@ impl PtraceDumper { ptrace::attach(pid).map_err(|e| AttachErr(child, e))?; loop { match wait::waitpid(pid, Some(wait::WaitPidFlag::__WALL)) { - Ok(_) => break, - Err(_e @ Errno::EINTR) => continue, + Ok(status) => { + let wait::WaitStatus::Stopped(_, status) = status else { + return Err(DumperError::WaitPidError( + child, + nix::errno::Errno::UnknownErrno, + )); + }; + + // Any signal will stop the thread, make sure it is SIGSTOP. Otherwise, this + // signal will be delivered after PTRACE_DETACH, and the thread will enter + // the "T (stopped)" state. + if status == nix::sys::signal::SIGSTOP { + break; + } + + // Signals other than SIGSTOP that are received need to be reinjected, + // or they will otherwise get lost. + // Note that the breakpad code doesn't do a detach, but that + // feels like a bug... + if let Err(err) = ptrace::cont(pid, status) { + ptrace_detach(child)?; + return Err(DumperError::WaitPidError(child, err)); + } + } + Err(Errno::EINTR) => continue, Err(e) => { ptrace_detach(child)?; return Err(DumperError::WaitPidError(child, e)); From 0bb9e1f3cfcff22126912ac45494c23cf3c36758 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 12 Aug 2024 11:35:18 +0200 Subject: [PATCH 2/4] Add failed attempt to test --- src/bin/test.rs | 43 +++++++++++++++++++++--------------- tests/ptrace_dumper.rs | 50 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 18 deletions(-) diff --git a/src/bin/test.rs b/src/bin/test.rs index bb064554..6713852c 100644 --- a/src/bin/test.rs +++ b/src/bin/test.rs @@ -18,10 +18,8 @@ mod linux { macro_rules! test { ($x:expr, $errmsg:expr) => { - if $x { - Ok(()) - } else { - Err($errmsg) + if !$x { + return Err($errmsg.into()); } }; } @@ -35,7 +33,7 @@ mod linux { fn test_thread_list() -> Result<()> { let ppid = getppid(); let dumper = PtraceDumper::new(ppid.as_raw(), STOP_TIMEOUT, Default::default())?; - test!(!dumper.threads.is_empty(), "No threads")?; + test!(!dumper.threads.is_empty(), "No threads"); test!( dumper .threads @@ -44,7 +42,16 @@ mod linux { .count() == 1, "Thread found multiple times" - )?; + ); + + test!( + dumper + .threads + .iter() + .any(|thread| thread.name.as_deref() == Some("sighup-thread")), + "Failed to locate and/or stop sighup-thread" + ); + Ok(()) } @@ -65,11 +72,11 @@ mod linux { let mut val = [0u8; std::mem::size_of::()]; let read = reader.read(stack_var, &mut val)?; assert_eq!(read, val.len()); - test!(val == expected_stack, "stack var not correct")?; + test!(val == expected_stack, "stack var not correct"); let read = reader.read(heap_var, &mut val)?; assert_eq!(read, val.len()); - test!(val == expected_heap, "heap var not correct")?; + test!(val == expected_heap, "heap var not correct"); Ok(()) }; @@ -99,12 +106,12 @@ mod linux { let stack_res = PtraceDumper::copy_from_process(ppid, stack_var, std::mem::size_of::())?; - test!(stack_res == expected_stack, "stack var not correct")?; + test!(stack_res == expected_stack, "stack var not correct"); let heap_res = PtraceDumper::copy_from_process(ppid, heap_var, std::mem::size_of::())?; - test!(heap_res == expected_heap, "heap var not correct")?; + test!(heap_res == expected_heap, "heap var not correct"); dumper.resume_threads()?; Ok(()) @@ -121,13 +128,13 @@ mod linux { .find_mapping(addr2) .ok_or("No mapping for addr2 found")?; - test!(dumper.find_mapping(0).is_none(), "NULL found")?; + test!(dumper.find_mapping(0).is_none(), "NULL found"); Ok(()) } fn test_file_id() -> Result<()> { let ppid = getppid().as_raw(); - let exe_link = format!("/proc/{}/exe", ppid); + let exe_link = format!("/proc/{ppid}/exe"); let exe_name = std::fs::read_link(exe_link)?.into_os_string(); let mut dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; dumper.suspend_threads()?; @@ -178,13 +185,13 @@ mod linux { dumper.suspend_threads()?; let module_reader::BuildId(id) = PtraceDumper::from_process_memory_for_mapping(&mapping, ppid)?; - test!(!id.is_empty(), "id-vec is empty")?; - test!(id.iter().any(|&x| x > 0), "all id elements are 0")?; + test!(!id.is_empty(), "id-vec is empty"); + test!(id.iter().any(|&x| x > 0), "all id elements are 0"); dumper.resume_threads()?; break; } } - test!(found_linux_gate, "found no linux_gate")?; + test!(found_linux_gate, "found no linux_gate"); Ok(()) } @@ -192,7 +199,7 @@ mod linux { let ppid = getppid().as_raw(); let dumper = PtraceDumper::new(ppid, STOP_TIMEOUT, Default::default())?; let linux_gate_loc = dumper.auxv.get_linux_gate_address().unwrap(); - test!(linux_gate_loc != 0, "linux_gate_loc == 0")?; + test!(linux_gate_loc != 0, "linux_gate_loc == 0"); let mut found_linux_gate = false; for mapping in &dumper.mappings { if mapping.name == Some(LINUX_GATE_LIBRARY_NAME.into()) { @@ -200,7 +207,7 @@ mod linux { test!( linux_gate_loc == mapping.start_address.try_into()?, "linux_gate_loc != start_address" - )?; + ); // This doesn't work here, as we do not test via "fork()", so the addresses are different // let ll = mapping.start_address as *const u8; @@ -214,7 +221,7 @@ mod linux { break; } } - test!(found_linux_gate, "found no linux_gate")?; + test!(found_linux_gate, "found no linux_gate"); Ok(()) } diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index dfc2976f..dd021c56 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -31,6 +31,56 @@ fn test_setup() { #[test] fn test_thread_list_from_child() { // Child spawns and looks in the parent (== this process) for its own thread-ID + + let (tx, rx) = std::sync::mpsc::sync_channel(1); + + // // We also spawn another thread that we send a SIGHUP to to ensure that the + // // ptracedumper correctly handles it + let _thread = std::thread::Builder::new() + .name("sighup-thread".into()) + .spawn(move || { + tx.send(unsafe { libc::pthread_self() }).unwrap(); + loop {} + }) + .unwrap(); + + let thread_id = rx.recv().unwrap(); + + // Unfortunately we need to set a signal handler to ignore the SIGHUP we send + // to the thread, as otherwise the default test harness fails + unsafe { + let mut act: libc::sigaction = std::mem::zeroed(); + if libc::sigemptyset(&mut act.sa_mask) != 0 { + eprintln!( + "unable to clear action mask: {:?}", + std::io::Error::last_os_error() + ); + return; + } + + unsafe extern "C" fn on_sig( + _sig: libc::c_int, + _info: *mut libc::siginfo_t, + _uc: *mut libc::c_void, + ) { + } + + act.sa_flags = libc::SA_SIGINFO; + act.sa_sigaction = on_sig as usize; + + // Register the action with the signal handler + if libc::sigaction(libc::SIGHUP, &act, std::ptr::null_mut()) != 0 { + eprintln!( + "unable to register signal handler: {:?}", + std::io::Error::last_os_error() + ); + } + } + + unsafe { + libc::pthread_kill(thread_id, libc::SIGHUP); + } + spawn_child("thread_list", &[]); } From 45568e6ad430ac858299db495854f4fa33061263 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 12 Aug 2024 11:55:50 +0200 Subject: [PATCH 3/4] Fixup --- tests/ptrace_dumper.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/ptrace_dumper.rs b/tests/ptrace_dumper.rs index dd021c56..48c4fa27 100644 --- a/tests/ptrace_dumper.rs +++ b/tests/ptrace_dumper.rs @@ -39,8 +39,10 @@ fn test_thread_list_from_child() { let _thread = std::thread::Builder::new() .name("sighup-thread".into()) .spawn(move || { - tx.send(unsafe { libc::pthread_self() }).unwrap(); - loop {} + tx.send(unsafe { libc::pthread_self() as usize }).unwrap(); + loop { + std::thread::sleep(std::time::Duration::from_secs(1)); + } }) .unwrap(); @@ -78,7 +80,7 @@ fn test_thread_list_from_child() { } unsafe { - libc::pthread_kill(thread_id, libc::SIGHUP); + libc::pthread_kill(thread_id as _, libc::SIGHUP); } spawn_child("thread_list", &[]); From 11618a4ab398093559bf5a6407bc9b3c61fdda6e Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Mon, 19 Aug 2024 15:49:51 +0200 Subject: [PATCH 4/4] Remove unneeded ptrace_detach --- src/linux/ptrace_dumper.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/linux/ptrace_dumper.rs b/src/linux/ptrace_dumper.rs index c69a79f6..66f36564 100644 --- a/src/linux/ptrace_dumper.rs +++ b/src/linux/ptrace_dumper.rs @@ -160,10 +160,7 @@ impl PtraceDumper { // Signals other than SIGSTOP that are received need to be reinjected, // or they will otherwise get lost. - // Note that the breakpad code doesn't do a detach, but that - // feels like a bug... if let Err(err) = ptrace::cont(pid, status) { - ptrace_detach(child)?; return Err(DumperError::WaitPidError(child, err)); } }