Block remote config signals during ftp functions (#2957)

* Reduce the amount of diagnostic messages sent

Done by deduplicating them.

Signed-off-by: Bob Weinand <[email protected]>
Handle case where the applictaion is stopped without other telemetry sent

This lead down to a path where the Stop action was actually enqueued without being ever causing a removal of the Application instances.

Signed-off-by: Bob Weinand <[email protected]>

* Fix appsec format

Signed-off-by: Bob Weinand <[email protected]>

* Block remote config signals during ftp functions

These are sensitive to EINTR, so block the signal during their execution.

Fixes #2952.

Signed-off-by: Bob Weinand <[email protected]>

* Avoid redundant signal checking on linux

Signed-off-by: Bob Weinand <[email protected]>

---------

Signed-off-by: Bob Weinand <[email protected]>

Author: bwoebi

Cargo.lock

@@ -216,9 +216,11 @@ static void _register_testing_objects(void);
 
 bool dd_config_minit(int module_number)
 {
-    // We have to disable remote config by default on lambda due to issues with the sidecar there. We'll eventually fix it though.
+    // We have to disable remote config by default on lambda due to issues with
+    // the sidecar there. We'll eventually fix it though.
     if (getenv("AWS_LAMBDA_FUNCTION_NAME")) {
-        config_entries[DDAPPSEC_CONFIG_DD_REMOTE_CONFIG_ENABLED].default_encoded_value = (zai_str) ZAI_STR_FROM_CSTR("false");
+        config_entries[DDAPPSEC_CONFIG_DD_REMOTE_CONFIG_ENABLED]
+            .default_encoded_value = (zai_str)ZAI_STR_FROM_CSTR("false");
     }
 
     if (!zai_config_minit(config_entries,

appsec/src/extension/configuration.c

@@ -171,7 +171,7 @@ struct ddog_RemoteConfigState *ddog_init_remote_config_state(const struct ddog_E
 
 const char *ddog_remote_config_get_path(const struct ddog_RemoteConfigState *remote_config);
 
-void ddog_process_remote_configs(struct ddog_RemoteConfigState *remote_config);
+bool ddog_process_remote_configs(struct ddog_RemoteConfigState *remote_config);
 
 bool ddog_type_can_be_instrumented(const struct ddog_RemoteConfigState *remote_config,
                                    ddog_CharSlice typename_);

components-rs/ddtrace.h

@@ -14,7 +14,7 @@ use datadog_remote_config::{
 use datadog_sidecar::service::blocking::SidecarTransport;
 use datadog_sidecar::service::{InstanceId, QueueId};
 use datadog_sidecar::shm_remote_config::{RemoteConfigManager, RemoteConfigUpdate};
-use datadog_sidecar_ffi::ddog_sidecar_send_debugger_data;
+use datadog_sidecar_ffi::{ddog_sidecar_send_debugger_data, ddog_sidecar_send_debugger_diagnostics};
 use ddcommon::Endpoint;
 use ddcommon_ffi::slice::AsBytes;
 use ddcommon_ffi::{CharSlice, MaybeError};
@@ -243,7 +243,8 @@ pub extern "C" fn ddog_remote_config_get_path(remote_config: &RemoteConfigState)
 }
 
 #[no_mangle]
-pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigState) {
+pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigState) -> bool {
+    let mut has_updates = false;
     loop {
         match remote_config.manager.fetch_update() {
             RemoteConfigUpdate::None => break,
@@ -292,7 +293,9 @@ pub extern "C" fn ddog_process_remote_configs(remote_config: &mut RemoteConfigSt
                 _ => (),
             },
         }
+        has_updates = true
     }
+    has_updates
 }
 
 fn apply_config(
@@ -531,5 +534,6 @@ pub unsafe extern "C" fn ddog_send_debugger_diagnostics<'a>(
         "Submitting debugger diagnostics data: {:?}",
         serde_json::to_string(&payload).unwrap()
     );
-    ddog_sidecar_send_debugger_data(transport, instance_id, queue_id, vec![payload])
+
+    ddog_sidecar_send_debugger_diagnostics(transport, instance_id, queue_id, payload)
 }

components-rs/remote_config.rs

@@ -223,6 +223,11 @@ ddog_MaybeError ddog_sidecar_send_debugger_datum(struct ddog_SidecarTransport **
                                                  ddog_QueueId queue_id,
                                                  struct ddog_DebuggerPayload *payload);
 
+ddog_MaybeError ddog_sidecar_send_debugger_diagnostics(struct ddog_SidecarTransport **transport,
+                                                       const struct ddog_InstanceId *instance_id,
+                                                       ddog_QueueId queue_id,
+                                                       struct ddog_DebuggerPayload diagnostics_payload);
+
 ddog_MaybeError ddog_sidecar_set_remote_config_data(struct ddog_SidecarTransport **transport,
                                                     const struct ddog_InstanceId *instance_id,
                                                     const ddog_QueueId *queue_id,

components-rs/sidecar.h

@@ -184,6 +184,7 @@ if test "$PHP_DDTRACE" != "no"; then
     ext/handlers_exception.c \
     ext/handlers_internal.c \
     ext/handlers_pcntl.c \
+    ext/handlers_signal.c \
     ext/integrations/exec_integration.c \
     ext/integrations/integrations.c \
     ext/ip_extraction.c \

config.m4

@@ -30,6 +30,9 @@ void ddtrace_free_unregistered_class(zend_class_entry *ce) {
 void ddtrace_curl_handlers_startup(void);
 void ddtrace_exception_handlers_startup(void);
 void ddtrace_pcntl_handlers_startup(void);
+#ifndef _WIN32
+void ddtrace_signal_block_handlers_startup(void);
+#endif
 
 #if PHP_VERSION_ID >= 80000 && PHP_VERSION_ID < 80200
 #include <hook/hook.h>
@@ -147,6 +150,10 @@ void ddtrace_internal_handlers_startup() {
     ddtrace_exception_handlers_startup();
 
     ddtrace_exec_handlers_startup();
+#ifndef _WIN32
+    // Block remote-config signals of some functions
+    ddtrace_signal_block_handlers_startup();
+#endif
 }
 
 void ddtrace_internal_handlers_shutdown(void) {

ext/handlers_internal.c

@@ -0,0 +1,78 @@
+#include "handlers_api.h"
+#include "remote_config.h"
+#include <signal.h>
+#include <php.h>
+
+/* We need to do signal blocking for the remote config signaling to not interfere with some PHP functions.
+ * See e.g. https://github.com/php/php-src/issues/16800
+ * I don't know the full problem space, so I expect there might be functions missing here, and we need to eventually expand this list.
+ */
+static void dd_handle_signal(zif_handler original_function, INTERNAL_FUNCTION_PARAMETERS) {
+    sigset_t x;
+    sigemptyset(&x);
+    sigaddset(&x, SIGVTALRM);
+    sigprocmask(SIG_BLOCK, &x, NULL);
+
+    original_function(INTERNAL_FUNCTION_PARAM_PASSTHRU);
+
+    sigprocmask(SIG_UNBLOCK, &x, NULL);
+#ifndef __linux__
+    // At least on linux unblocking causes immediate signal delivery.
+    ddtrace_check_for_new_config_now();
+#endif
+}
+
+#define BLOCKSIGFN(function) \
+    static zif_handler dd_handle_signal_zif_##function;\
+    static ZEND_FUNCTION(dd_handle_signal_##function) { \
+        dd_handle_signal(dd_handle_signal_zif_##function, INTERNAL_FUNCTION_PARAM_PASSTHRU); \
+    }
+
+#define BLOCK(x) \
+    x(ftp_alloc) \
+    x(ftp_append) \
+    x(ftp_cdup) \
+    x(ftp_chdir) \
+    x(ftp_chmod) \
+    x(ftp_close) \
+    x(ftp_connect) \
+    x(ftp_delete) \
+    x(ftp_exec) \
+    x(ftp_fget) \
+    x(ftp_fput) \
+    x(ftp_get) \
+    x(ftp_get_option) \
+    x(ftp_login) \
+    x(ftp_mdtm) \
+    x(ftp_mkdir) \
+    x(ftp_mlsd) \
+    x(ftp_nb_continue) \
+    x(ftp_nb_fget) \
+    x(ftp_nb_fput) \
+    x(ftp_nb_get) \
+    x(ftp_nb_put) \
+    x(ftp_nlist) \
+    x(ftp_pasv) \
+    x(ftp_put) \
+    x(ftp_pwd) \
+    x(ftp_quit) \
+    x(ftp_raw) \
+    x(ftp_rawlist) \
+    x(ftp_rename) \
+    x(ftp_rmdir) \
+    x(ftp_site) \
+    x(ftp_size) \
+    x(ftp_ssl_connect) \
+    x(ftp_systype) \
+
+BLOCK(BLOCKSIGFN)
+
+void ddtrace_signal_block_handlers_startup() {
+#define BLOCKFNENTRY(function) { ZEND_STRL(#function), &dd_handle_signal_zif_##function, ZEND_FN(dd_handle_signal_##function) },
+    datadog_php_zif_handler handlers[] = { BLOCK(BLOCKFNENTRY) };
+
+    size_t handlers_len = sizeof handlers / sizeof handlers[0];
+    for (size_t i = 0; i < handlers_len; ++i) {
+        datadog_php_install_handler(handlers[i]);
+    }
+}

ext/handlers_signal.c

@@ -57,6 +57,13 @@ DDTRACE_PUBLIC void ddtrace_set_all_thread_vm_interrupt(void) {
 #endif
 }
 
+void ddtrace_check_for_new_config_now(void) {
+    if (DDTRACE_G(remote_config_state) && !DDTRACE_G(reread_remote_configuration) && ddog_process_remote_configs(DDTRACE_G(remote_config_state))) {
+        // If we blocked the signal, notify the other threads too
+        ddtrace_set_all_thread_vm_interrupt();
+    }
+}
+
 DDTRACE_PUBLIC const char *ddtrace_remote_config_get_path() {
     if (DDTRACE_G(remote_config_state)) {
         return ddog_remote_config_get_path(DDTRACE_G(remote_config_state));

ext/remote_config.c

@@ -7,6 +7,8 @@ void ddtrace_minit_remote_config(void);
 void ddtrace_mshutdown_remote_config(void);
 void ddtrace_rinit_remote_config(void);
 void ddtrace_rshutdown_remote_config(void);
+void ddtrace_check_for_new_config_now(void);
+
 
 DDTRACE_PUBLIC void ddtrace_set_all_thread_vm_interrupt(void);
 DDTRACE_PUBLIC const char *ddtrace_remote_config_get_path(void);