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]>
DataDog · Nov 15, 2024 · 19dcc2f · 19dcc2f
1 parent d1a8696
commit 19dcc2f
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 2 deletions.
diff --git a/components-rs/ddtrace.h b/components-rs/ddtrace.h
@@ -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_);

diff --git a/components-rs/remote_config.rs b/components-rs/remote_config.rs
@@ -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(

diff --git a/config.m4 b/config.m4
@@ -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 \

diff --git a/ext/handlers_internal.c b/ext/handlers_internal.c
@@ -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) {

diff --git a/ext/handlers_signal.c b/ext/handlers_signal.c
@@ -0,0 +1,75 @@
+#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);
+    ddtrace_check_for_new_config_now();
+}
+
+#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]);
+    }
+}
diff --git a/ext/remote_config.c b/ext/remote_config.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));

diff --git a/ext/remote_config.h b/ext/remote_config.h
@@ -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);