From 8e187e6a532ea7417c008207cf216c19be45a68a Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Thu, 22 Aug 2024 23:49:17 +0200 Subject: [PATCH 1/4] Allow using abstract namespace for crashtracker unix socket Thanks to PrivateTmp in systemd it's pretty likely that different processes will see the same sidecar (which lives in a network namespace), but may not see the crashtracker socket in /tmp. Defaulting to the abstract unix namespace on Linux will work around any possible problems here. Signed-off-by: Bob Weinand --- crashtracker/src/collector/crash_handler.rs | 9 +++++++ crashtracker/src/receiver.rs | 29 ++++++++++++++++----- sidecar/src/crashtracker.rs | 11 +++++--- sidecar/src/setup/unix.rs | 4 +++ 4 files changed, 43 insertions(+), 10 deletions(-) diff --git a/crashtracker/src/collector/crash_handler.rs b/crashtracker/src/collector/crash_handler.rs index 68d64bb87..ce4f71256 100644 --- a/crashtracker/src/collector/crash_handler.rs +++ b/crashtracker/src/collector/crash_handler.rs @@ -346,6 +346,15 @@ fn handle_posix_signal_impl(signum: i32) -> anyhow::Result<()> { res } ReceiverType::UnixSocket(path) => { + #[cfg(target_os = "linux")] + let mut unix_stream = if path.starts_with(['.', '/']) { + UnixStream::connect(path) + } else { + use std::os::linux::net::SocketAddrExt; + let addr = std::os::unix::net::SocketAddr::from_abstract_name(path)?; + UnixStream::connect_addr(&addr) + }?; + #[cfg(not(target_os = "linux"))] let mut unix_stream = UnixStream::connect(path)?; let res = emit_crashreport( &mut unix_stream, diff --git a/crashtracker/src/receiver.rs b/crashtracker/src/receiver.rs index 0387e3139..d2f9ea3a8 100644 --- a/crashtracker/src/receiver.rs +++ b/crashtracker/src/receiver.rs @@ -24,15 +24,30 @@ pub fn resolve_frames( } pub fn get_unix_socket(socket_path: impl AsRef) -> anyhow::Result { - let socket_path = socket_path.as_ref(); - if std::fs::metadata(socket_path).is_ok() { - std::fs::remove_file(socket_path) - .with_context(|| format!("could not delete previous socket at {:?}", socket_path))?; + fn path_bind(socket_path: impl AsRef) -> anyhow::Result { + let socket_path = socket_path.as_ref(); + if std::fs::metadata(socket_path).is_ok() { + std::fs::remove_file(socket_path).with_context(|| { + format!("could not delete previous socket at {:?}", socket_path) + })?; + } + Ok(UnixListener::bind(socket_path)?) } - let unix_listener = - UnixListener::bind(socket_path).context("Could not create the unix socket")?; - Ok(unix_listener) + #[cfg(target_os = "linux")] + let unix_listener = if socket_path.as_ref().starts_with(['.', '/']) { + path_bind(socket_path) + } else { + use std::os::linux::net::SocketAddrExt; + std::os::unix::net::SocketAddr::from_abstract_name(socket_path.as_ref()) + .and_then(|addr| { + std::os::unix::net::UnixListener::bind_addr(&addr).and_then(UnixListener::from_std) + }) + .map_err(anyhow::Error::msg) + }; + #[cfg(not(target_os = "linux"))] + let unix_listener = path_bind(socket_path); + unix_listener.context("Could not create the unix socket") } pub fn receiver_entry_point_unix_socket(socket_path: impl AsRef) -> anyhow::Result<()> { diff --git a/sidecar/src/crashtracker.rs b/sidecar/src/crashtracker.rs index 75f3cebcc..176ec45a2 100644 --- a/sidecar/src/crashtracker.rs +++ b/sidecar/src/crashtracker.rs @@ -1,13 +1,18 @@ // Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ // SPDX-License-Identifier: Apache-2.0 -use std::{env, path::PathBuf}; +use std::path::PathBuf; use crate::primary_sidecar_identifier; pub fn crashtracker_unix_socket_path() -> PathBuf { - env::temp_dir().join(format!( + let base_path = format!( concat!("libdatadog.ct.", crate::sidecar_version!(), "@{}.sock"), primary_sidecar_identifier() - )) + ); + #[cfg(target_os = "linux")] + let ret = base_path.into(); + #[cfg(not(target_os = "linux"))] + let ret = std::env::temp_dir().join(base_path); + ret } diff --git a/sidecar/src/setup/unix.rs b/sidecar/src/setup/unix.rs index 5f328ce85..b8337bed7 100644 --- a/sidecar/src/setup/unix.rs +++ b/sidecar/src/setup/unix.rs @@ -119,6 +119,10 @@ impl Default for SharedDirLiaison { } #[cfg(target_os = "linux")] +// Important note: +// Never put any runtime data which both the sidecar and the client processes must see onto disk. +// In particular, when using different mount namespaces, but a shared network namespace, the +// processes don't necessarily see the same things. mod linux { use std::{io, os::unix::net::UnixListener, path::PathBuf}; From c7a60a65d713919782257de99c055cf13a9e373f Mon Sep 17 00:00:00 2001 From: iamluc Date: Fri, 23 Aug 2024 15:31:43 +0200 Subject: [PATCH 2/4] TEST with rust 1.73.0 --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6f813e7d4..0b6b71ae7 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -2,7 +2,7 @@ variables: # These are gitlab variables so that it's easier to do a manual deploy # If these are set witih value and description, then it gives you UI elements DOWNSTREAM_BRANCH: - value: "main" + value: "luc/bump-rust-to-1.73" description: "downstream jobs are triggered on this branch" include: From 355367afbefdbc01e2fb185092c560927056a99d Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Fri, 23 Aug 2024 16:32:58 +0200 Subject: [PATCH 3/4] Append Crashtracker to file to avoid overwrites with telemetry Signed-off-by: Bob Weinand --- crashtracker/src/crash_info/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crashtracker/src/crash_info/mod.rs b/crashtracker/src/crash_info/mod.rs index f5d38fee8..57ceb0993 100644 --- a/crashtracker/src/crash_info/mod.rs +++ b/crashtracker/src/crash_info/mod.rs @@ -239,8 +239,11 @@ impl CrashInfo { impl CrashInfo { /// Emit the CrashInfo as structured json in file `path`. pub fn to_file(&self, path: &Path) -> anyhow::Result<()> { - let file = - File::create(path).with_context(|| format!("Failed to create {}", path.display()))?; + let file = File::options() + .create(true) + .append(true) + .open(path) + .with_context(|| format!("Failed to create {}", path.display()))?; serde_json::to_writer_pretty(file, self) .with_context(|| format!("Failed to write json to {}", path.display()))?; Ok(()) From c4b5e72cb9cad44bfb2ea63b08490a571ebe25bc Mon Sep 17 00:00:00 2001 From: iamluc Date: Mon, 26 Aug 2024 10:20:53 +0200 Subject: [PATCH 4/4] Revert "TEST with rust 1.73.0" This reverts commit c7a60a65d713919782257de99c055cf13a9e373f. --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0b6b71ae7..6f813e7d4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -2,7 +2,7 @@ variables: # These are gitlab variables so that it's easier to do a manual deploy # If these are set witih value and description, then it gives you UI elements DOWNSTREAM_BRANCH: - value: "luc/bump-rust-to-1.73" + value: "main" description: "downstream jobs are triggered on this branch" include: