Misc fixes

Author: cataphract

appsec/helper-rust/coverage_init.c

@@ -2,6 +2,9 @@
 //
 // The embedded sidecar calls this module explicitly from its entrypoint. It
 // reinitializes the LLVM profiling runtime to use the correct output path.
+// Every other process that loads the library is left pointing at the path the
+// harness passes in LLVM_PROFILE_FILE, which is /dev/null: none of them runs
+// helper-rust, so their profiles cover nothing that is reported on.
 //
 // Coverage data is flushed when the process receives SIGUSR1, as well as
 // at normal process exit via atexit handler.
@@ -15,13 +18,16 @@
 #include <signal.h>
 #include <fcntl.h>
 #include <stdarg.h>
+#include <limits.h>
+#include <sys/stat.h>
 #include <time.h>
 #include <errno.h>
 
 // LLVM profiling runtime API (from compiler-rt/lib/profile/)
 extern void __llvm_profile_initialize_file(void);
 extern void __llvm_profile_set_filename(const char *);
 extern int __llvm_profile_write_file(void);
+extern void __llvm_profile_reset_counters(void);
 extern const char *__llvm_profile_get_filename(void);
 
 static int log_fd = -1;
@@ -55,10 +61,17 @@ static void coverage_log(const char *fmt, ...) {
     write(log_fd, buf, offset);
 }
 
+// Writing in merge mode adds the file's counters back into the in-process
+// ones, so a process that writes more than once counts everything it has
+// already written a second time and the totals double per flush. This process
+// flushes on SIGUSR1 as well as at exit, and the runtime registers a writer of
+// its own on top of that, so the counters have to be dropped once they are on
+// disk.
 static void coverage_flush(void) {
     coverage_log("coverage_flush() called, flushing coverage data");
 
     int ret = __llvm_profile_write_file();
+    __llvm_profile_reset_counters();
     coverage_log("__llvm_profile_write_file() returned %d (errno=%d: %s)",
                  ret, errno, strerror(errno));
 
@@ -79,16 +92,67 @@ static void coverage_signal_handler(int signo) {
     (void)signo;
     coverage_log("SIGUSR1 received, flushing coverage data");
     int ret = __llvm_profile_write_file();
+    __llvm_profile_reset_counters();
     coverage_log("__llvm_profile_write_file() returned %d", ret);
 }
 
+// %m selects merge mode, where the runtime serializes on an fcntl write lock
+// and accumulates into one shared file per instrumented binary instead of
+// writing one file per process. The runtime creates that file itself, at the
+// first write, with open(..., 0666); the container's 022 umask turns it into
+// 0644. Every container in a run shares the file, and a sidecar inherits the
+// user of the SAPI that spawned it, so whoever writes first would otherwise
+// end up owning a file the other users cannot open. Get there first and create
+// it with the bits the umask would have dropped, after which the runtime only
+// ever opens a file that everyone can already write. A zero-length file is
+// what merge mode expects to find when there is nothing to merge yet.
+//
+// Only the runtime can expand the name, since %m stands for a hash of the
+// instrumented binary, hence __llvm_profile_get_filename() rather than a path
+// built by the caller.
+//
+// Created under a private name and hard-linked into place, so the file is
+// never reachable under its final name while it is still 0644; link() fails
+// with EEXIST if another process got there first, which is the wanted
+// create-only-if-absent. The staging name deliberately does not end in
+// .profraw, so a leftover cannot reach the coverage report.
+static void coverage_precreate_profile_file(void) {
+    const char *filename = __llvm_profile_get_filename();
+    if (!filename || !filename[0]) {
+        return;
+    }
+
+    char staging[PATH_MAX];
+    int len = snprintf(staging, sizeof(staging), "%s.new.%d", filename,
+                       (int)getpid());
+    if (len < 0 || len >= (int)sizeof(staging)) {
+        return;
+    }
+
+    int fd = open(staging, O_RDWR | O_CREAT | O_EXCL, 0666);
+    if (fd < 0) {
+        return;
+    }
+    if (fchmod(fd, 0666) != 0) {
+        coverage_log("Failed to make %s group/other-writable: %s", staging,
+                     strerror(errno));
+    } else if (link(staging, filename) != 0 && errno != EEXIST) {
+        coverage_log("Failed to create %s: %s", filename, strerror(errno));
+    }
+    close(fd);
+    unlink(staging);
+}
+
 void ddappsec_helper_coverage_init(void) {
     coverage_log("ddappsec_helper_coverage_init() called");
 
-    const char *profile_path = getenv("_DD_HELPER_RUST_COVERAGE_FILE");
-    if (!profile_path) {
-        profile_path = "/helper-rust/coverage/helper-%h-%p-%m.profraw";
-    }
+    // Hardcoded rather than read from the environment on purpose. Only about
+    // a third of the sidecars in a run inherit the ambient environment at all,
+    // so a variable naming this path would reach some of them and not others,
+    // and pointing it anywhere else would split the merge pool in two instead
+    // of moving it. %m rather than %p so that repeatedly respawned sidecars
+    // keep merging into the one file.
+    const char *profile_path = "/helper-rust/helper-%m.profraw";
     coverage_log("Helper coverage profile = %s", profile_path);
 
     const char *filename_before = __llvm_profile_get_filename();
@@ -103,6 +167,8 @@ void ddappsec_helper_coverage_init(void) {
     coverage_log("Profile filename AFTER reinit: %s",
                  filename_after ? filename_after : "(null)");
 
+    coverage_precreate_profile_file();
+
     signal(SIGUSR1, coverage_signal_handler);
     coverage_log("Signal handler installed for SIGUSR1");
 

appsec/helper-rust/src/service/sampler.rs

@@ -204,7 +204,7 @@ impl SchemaSampler {
         }
 
         // most recent at the top
-        entries.sort_by(|a, b| b.data.last_accessed.cmp(&a.data.last_accessed));
+        entries.sort_by_key(|entry| std::cmp::Reverse(entry.data.last_accessed));
 
         let count = std::cmp::min(entries.len(), MAX_ITEMS * 2 / 3);
         for ce in entries.into_iter().take(count) {

appsec/helper-rust/src/service/waf_diag.rs

@@ -32,7 +32,7 @@ pub fn report_diagnostics_errors(
     use libddwaf::object::WafObjectType;
 
     let path_label = rc_path.map_or("(bundled rules)", |p| p.as_str());
-    let parsed_key = match rc_path.map(|p| rc::ParsedConfigKey::from_rc_path(p)) {
+    let parsed_key = match rc_path.map(rc::ParsedConfigKey::from_rc_path) {
         Some(None) => {
             warning!("Failed to parse config key for {:?}", path_label);
             return;

appsec/src/extension/commands_helpers.c

@@ -244,23 +244,22 @@ static ATTR_WARN_UNUSED dd_result _imsg_recv(dd_imsg *nonnull imsg,
 {
     mlog(dd_log_debug, "Will exchange message with helper");
 
-    dd_helper_response response;
     dd_result res = dd_conn_roundtrip(
-        conn, buffer->data, buffer->final_msg_size, &response);
+        conn, buffer->data, buffer->final_msg_size, &imsg->_response);
     if (res) {
         return res;
     }
-    imsg->_response = response;
 
-    mpack_tree_init(&imsg->_tree, response.data, response.data_len);
+    mpack_tree_init(
+        &imsg->_tree, imsg->_response.data, imsg->_response.data_len);
     mpack_tree_parse(&imsg->_tree);
     imsg->root = mpack_tree_root(&imsg->_tree);
     mpack_error_t err = mpack_tree_error(&imsg->_tree);
     if (err != mpack_ok) {
         mlog(dd_log_warning, "Error parsing msgpack message: %s",
             mpack_error_to_string(err));
-        _dump_in_msg(dd_log_debug, response.data, response.data_len);
-        dd_helper_response_destroy(&response);
+        _dump_in_msg(
+            dd_log_debug, imsg->_response.data, imsg->_response.data_len);
         UNUSED(_imsg_destroy(imsg));
         return dd_helper_say_goobye;
     }

appsec/src/extension/ddappsec.c

@@ -194,6 +194,7 @@ static PHP_GINIT_FUNCTION(ddappsec)
     if (!is_main_thread) {
 #    if defined(__linux__)
         extern void *__dso_handle;
+        // adds a dependency on glibc 2.18
         extern int __cxa_thread_atexit_impl(
             void (*func)(void *), void *arg, void *dso_handle);
         __cxa_thread_atexit_impl(_tshutdown_handler, NULL, __dso_handle);

appsec/src/extension/network.c

@@ -100,7 +100,8 @@ dd_result dd_conn_roundtrip(dd_conn *nonnull conn, char *nonnull request,
     dd_header h;
     memcpy(&h, response.ptr, sizeof(h));
     if (memcmp(h.code, "dds", 4) != 0) {
-        mlog(dd_log_warning, "Helper response has invalid magic: %s", h.code);
+        mlog(dd_log_warning, "Helper response has invalid magic: %.*s",
+            (int)sizeof(h), h.code);
         ret = dd_helper_say_goobye;
         goto error;
     }

appsec/src/extension/request_lifecycle.c

@@ -262,10 +262,6 @@ retry:;
         dd_duration_waf_ext_account(&start);
     }
 
-    if (rbe) {
-        zend_string_release(rbe);
-    }
-
     // we might have been disabled by request_init
 
     zend_array *spec = NULL;
@@ -298,6 +294,10 @@ retry:;
         goto retry;
     }
 
+    if (rbe) {
+        zend_string_release(rbe);
+    }
+
     dd_request_abort_destroy_block_params(&req_info.req_info.block_params);
 
     return spec;

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy

@@ -559,8 +559,18 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
                 withFileSystemBind(
                         coverageDirectory, '/helper-rust',
                         BindMode.READ_WRITE)
-                withEnv '_DD_HELPER_RUST_COVERAGE_FILE',
-                        '/helper-rust/helper-%h-%p-%m.profraw'
+                // Only the sidecar runs helper-rust, but the SSI loader maps
+                // the coverage-instrumented libdatadog_php.so into *every* PHP
+                // process, and the profiling runtime in each of them writes
+                // the whole 34M counter table out on exit. Those profiles
+                // cover none of the reported sources, so send them nowhere:
+                // the report is built from the helper's profile alone, and
+                // ddappsec_helper_coverage_init() names that one itself, in
+                // /helper-rust, rather than taking it from the environment.
+                // Leaving this unset is not an option -- the runtime would
+                // fall back to its default name and drop a default_*.profraw
+                // per process into the cwd instead.
+                withEnv 'LLVM_PROFILE_FILE', '/dev/null'
             }
             withEnv 'USE_SSI', '1'
             withEnv 'DD_LOADER_PACKAGE_PATH', '/tmp/dd-package'

libdatadog

@@ -19,19 +19,37 @@ fn add_glibc_direct_preloads(config: &mut Config) {
         return;
     };
 
-    let is_glibc = unsafe { libc::dlsym(libc::RTLD_DEFAULT, c"gnu_get_libc_version".as_ptr()) };
-    if is_glibc.is_null() {
+    if !glibc_version().is_some_and(|version| version < (2, 34)) {
         return;
     }
 
     // Direct spawn asks ld-linux to load the entrypoint DSO itself, before the
-    // sidecar can run any initialization code. Supply the old-glibc providers
-    // as loader --preload arguments.
+    // sidecar can run any initialization code. Before 2.34, glibc provided
+    // these symbols in separate libraries, so load them explicitly.
     direct_spawn
         .preloads
         .extend(["libdl.so.2", "libm.so.6", "libpthread.so.0", "librt.so.1"].map(String::from));
 }
 
+#[cfg(all(feature = "glibc-compat", target_os = "linux"))]
+fn glibc_version() -> Option<(u32, u32)> {
+    let symbol = unsafe { libc::dlsym(libc::RTLD_DEFAULT, c"gnu_get_libc_version".as_ptr()) };
+    if symbol.is_null() {
+        return None;
+    }
+
+    let get_version: unsafe extern "C" fn() -> *const libc::c_char =
+        unsafe { std::mem::transmute(symbol) };
+    let version = unsafe { get_version() };
+    if version.is_null() {
+        return None;
+    }
+
+    let version = unsafe { std::ffi::CStr::from_ptr(version) }.to_str().ok()?;
+    let (major, minor) = version.split_once('.')?;
+    Some((major.parse().ok()?, minor.parse().ok()?))
+}
+
 pub fn start_or_connect_to_sidecar(config: Config) -> anyhow::Result<SidecarTransport> {
     #[cfg(all(feature = "glibc-compat", target_os = "linux"))]
     let mut config = config;