Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@
devShells.default = pkgs.mkShell {
packages = rustDeps ++ jsDeps ++ pyDeps ++ cdnDeps;

# jemalloc's configure uses -O0 test builds, which conflict with
# Nix's _FORTIFY_SOURCE hardening (requires -O).
hardeningDisable = [ "fortify" ];

shellHook = ''
export LIBCLANG_PATH="${pkgs.libclang.lib}/lib"
'';
Expand Down
9 changes: 9 additions & 0 deletions nix/modules/moq-relay.nix
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ in
default = "/var/lib/moq-relay";
description = "State directory for keys and runtime data";
};

heapDumpPrefix = lib.mkOption {
type = lib.types.str;
default = "/tmp/moq-relay.heap";
description = "Path prefix for jemalloc heap profile dumps (triggered via kill -USR1)";
Comment on lines +146 to +149

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In systemd's systemd.exec(5), what does PrivateTmp=truedo to a service's/tmp, and are files written there visible at the host path /tmp/...?

💡 Result:

When PrivateTmp=true is set for a systemd service, systemd creates a new file system namespace for the service's processes and mounts private /tmp and /var/tmp directories inside it. These are bind-mounted over the service's view of /tmp and /var/tmp, using subdirectories of the host's /tmp and /var/tmp (typically named like /tmp/systemd-private--service-). From the service's perspective, /tmp behaves normally but is isolated. Files written by the service to its /tmp are not visible at the host's /tmp/... paths, nor shared with other services or the host unless they join the same namespace via JoinsNamespaceOf=.

Citations:


/tmp is the wrong default dump location with PrivateTmp enabled.

Line 148 sets the default heap dump prefix to /tmp/moq-relay.heap, but the unit enables PrivateTmp = true on line 206. With this setting, systemd creates an isolated file system namespace where the service's /tmp is private—files written there are not visible at the host's /tmp/... paths. The profile will not appear where the option description implies. Default this to cfg.stateDir instead, which is already allowlisted in ReadWritePaths on line 209, or disable PrivateTmp if host-visible dumps are required.

Suggested fix
-      default = "/tmp/moq-relay.heap";
+      default = "${cfg.stateDir}/moq-relay.heap";

Also applies to: 216-217

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nix/modules/moq-relay.nix` around lines 146 - 149, The default heap dump
prefix uses /tmp which is private because PrivateTmp is enabled; update the
heapDumpPrefix default to use cfg.stateDir (e.g., join cfg.stateDir with
"moq-relay.heap") so dumps land in the service's allowed ReadWritePaths, and
make the same change for the other heap/profile default(s) referenced later (the
entries on lines ~216-217) instead of /tmp; alternatively, if you truly need
host-visible /tmp paths, disable PrivateTmp in the systemd service unit
(PrivateTmp = false).

};
};

config = lib.mkIf cfg.enable {
Expand Down Expand Up @@ -207,6 +213,9 @@ in
};

environment = {
# Enable jemalloc heap profiling; dump with `kill -USR1 <pid>`
MALLOC_CONF = "prof:true,prof_active:true,prof_prefix:${cfg.heapDumpPrefix}";

MOQ_LOG_LEVEL = lib.mkDefault cfg.logLevel;

# Server configuration
Expand Down
5 changes: 4 additions & 1 deletion nix/overlay.nix
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ in
crateInfo ../rs/moq-relay/Cargo.toml
// {
src = craneLib.cleanCargoSource ../.;
cargoExtraArgs = "-p moq-relay";
cargoExtraArgs = "-p moq-relay --features jemalloc";
# Enable frame pointers for profiling support (negligible overhead on x86_64).
# This also ensures the CDN build matches what Cachix caches.
RUSTFLAGS = "-C force-frame-pointers=yes";
# jemalloc's configure uses -O0 test builds, which conflict with
# Nix's _FORTIFY_SOURCE hardening (requires -O).
hardeningDisable = [ "fortify" ];
}
);

Expand Down
3 changes: 3 additions & 0 deletions rs/moq-relay/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ doc = false
[features]
default = ["iroh", "quinn", "websocket"]
iroh = ["moq-native/iroh"]
jemalloc = ["dep:tikv-jemallocator", "dep:tikv-jemalloc-ctl"]
noq = ["moq-native/noq"]
quinn = ["moq-native/quinn"]
quiche = ["moq-native/quiche"]
Expand Down Expand Up @@ -50,6 +51,8 @@ rustls = { version = "0.23", features = [
serde = { version = "1", features = ["derive"] }
serde_with = { version = "3", features = ["json", "base64"] }
thiserror = "2"
tikv-jemalloc-ctl = { version = "0.6", optional = true }
tikv-jemallocator = { version = "0.6", features = ["profiling", "unprefixed_malloc_on_supported_platforms"], optional = true }
tokio = { workspace = true, features = ["full"] }
toml = "0.9"
tower-http = { version = "0.6", features = ["cors"] }
Expand Down
34 changes: 34 additions & 0 deletions rs/moq-relay/src/jemalloc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
use tikv_jemalloc_ctl::raw;

/// Activate jemalloc heap profiling and listen for SIGUSR1 to dump profiles.
///
/// The dump path is controlled by `MALLOC_CONF=prof_prefix:<path>`.
/// Returns `Ok(())` if profiling is not available (i.e. MALLOC_CONF=prof:true was not set).
pub async fn run() -> anyhow::Result<()> {
let prof_active = b"prof.active\0";

match unsafe { raw::read::<bool>(prof_active) } {
Ok(true) => tracing::info!("jemalloc heap profiling is active"),
Ok(false) => {
tracing::info!("jemalloc profiling compiled in; activating");
unsafe { raw::write(prof_active, true) }
.map_err(|err| anyhow::anyhow!("failed to activate jemalloc profiling: {err}"))?;
}
Comment thread
kixelated marked this conversation as resolved.
Err(err) => {
tracing::debug!(%err, "jemalloc profiling not available — set MALLOC_CONF=prof:true to enable");
return Ok(());
}
}

let mut sig = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined1())?;

loop {
sig.recv().await;

// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
Comment on lines +28 to +31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In jemalloc's mallctlAPI forprof.dump, does passing NULLuseprof_prefix, and does passing an empty string behave differently?

💡 Result:

Yes, passing NULL to the newp parameter of mallctl("prof.dump", NULL, NULL, NULL, 0) uses the prof_prefix to generate the filename according to the pattern <prefix><pid><seq>.m<mseq>.heap, where <prefix> comes from opt.prof_prefix or prof.prefix. No sources explicitly describe passing an empty string (i.e., mallctl("prof.dump", NULL, NULL, "", strlen(""))), but based on the man page description treating only NULL specially to use prof_prefix while non-NULL specifies a filename, an empty string would likely be treated as a filename "", which may fail to open or behave differently (e.g., error or empty file in current directory), not falling back to prof_prefix.

Citations:


prof.dump must be written with NULL to use prof_prefix.

The jemalloc manual specifies that prof.dump uses the <prefix> naming pattern only when passed NULL. This code passes a non-null pointer to an empty string instead, which jemalloc treats as a filename and does not trigger the prof_prefix fallback described in the comment. Use std::ptr::null::<std::ffi::c_char>() to pass the required NULL value.

Suggested fix
-		// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
-		match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
+		// NULL tells jemalloc to use prof_prefix from MALLOC_CONF.
+		match unsafe { raw::write(b"prof.dump\0", std::ptr::null::<std::ffi::c_char>()) } {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Empty path tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", b"\0" as *const u8) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
// NULL tells jemalloc to use prof_prefix from MALLOC_CONF.
match unsafe { raw::write(b"prof.dump\0", std::ptr::null::<std::ffi::c_char>()) } {
Ok(()) => tracing::info!("heap profile dumped"),
Err(err) => tracing::error!(%err, "failed to dump heap profile"),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@rs/moq-relay/src/jemalloc.rs` around lines 30 - 33, The jemalloc prof.dump
call is incorrectly passing a non-null empty C-string (b"\0") which jemalloc
treats as a filename; change the second argument to a true NULL pointer so
jemalloc will use prof_prefix. In the raw::write call in jemalloc.rs (the match
on unsafe { raw::write(b"prof.dump\0", ...) }), replace the b"\0" pointer with
std::ptr::null::<std::ffi::c_char>() (or std::ptr::null::<libc::c_char>() if
libc types are used) so the call becomes raw::write(b"prof.dump\0",
std::ptr::null::<std::ffi::c_char>()) inside the existing unsafe block.

}
}
}
2 changes: 2 additions & 0 deletions rs/moq-relay/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ mod auth;
mod cluster;
mod config;
mod connection;
#[cfg(feature = "jemalloc")]
pub mod jemalloc;
mod web;
#[cfg(feature = "websocket")]
mod websocket;
Expand Down
10 changes: 10 additions & 0 deletions rs/moq-relay/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ use moq_relay::*;

use anyhow::Context;

#[cfg(feature = "jemalloc")]
#[global_allocator]
static ALLOC: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc;

#[tokio::main]
async fn main() -> anyhow::Result<()> {
// TODO: It would be nice to remove this and rely on feature flags only.
Expand Down Expand Up @@ -48,10 +52,16 @@ async fn main() -> anyhow::Result<()> {
// Notify systemd that we're ready after all initialization is complete
let _ = sd_notify::notify(&[sd_notify::NotifyState::Ready]);

#[cfg(feature = "jemalloc")]
let jemalloc = jemalloc::run();
#[cfg(not(feature = "jemalloc"))]
let jemalloc = std::future::pending::<anyhow::Result<()>>();

tokio::select! {
Err(err) = cluster.clone().run() => return Err(err).context("cluster failed"),
Err(err) = web.run() => return Err(err).context("web server failed"),
Err(err) = serve(server, cluster, auth) => return Err(err).context("server failed"),
Err(err) = jemalloc => return Err(err).context("jemalloc profiler failed"),
else => Ok(()),
}
}
Expand Down
Loading