v4.2.0 Async-signal-safe, thread-safe, and memory-safe reloading#2207
Open
atomicturtle wants to merge 1 commit into
Open
v4.2.0 Async-signal-safe, thread-safe, and memory-safe reloading#2207atomicturtle wants to merge 1 commit into
atomicturtle wants to merge 1 commit into
Conversation
Summary of changes: - Standardized loop-wide signal masking using `sigprocmask` across all daemons (analysisd, remoted, agentd, logcollector, etc.) to prevent signal re-entrancy during critical processing and memory operations. - Implemented "copy-swap-free" atomic reload pattern for all configuration structures. - Hardened `shared/sig_op.c` signal handler (HandleSIG) to be strictly async-signal-safe, eliminating unsafe `snprintf` and `log2file` in favor of direct `write()` syscalls and a pre-cached PID path. - Added `DeletePID_AsyncSafe()` using `unlink()` for safe PID file removal in signal handler context. - Implemented "lock-first" pattern in `remoted/secure.c` to prevent deadlocks between main and worker threads during reloads. - Guaranteed memory safety by zero-initializing configuration reload structures and standardizing `Eventinfo` cleanup via `Free_Eventinfo()`. - Updated OpenSSL version guards for compatibility with modern distributions (EL8+). - Removed `SA_RESTART` for `SIGHUP` to ensure immediate reload responsiveness. These changes eliminate race conditions, memory leaks, and potential deadlocks triggered by SIGHUP while maintaining project-wide build integrity. Signed-off-by: Scott R. Shinn <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make daemon reloads safer and more responsive by standardizing SIGHUP handling (masking/unmasking around blocking calls), introducing copy-swap-free reload patterns for configs, and hardening signal/PID-file handling for async-signal-safety across the codebase.
Changes:
- Standardize SIGHUP masking/unmasking in daemon loops and add SIGHUP-triggered reload logic using “copy-swap-free” config replacement.
- Make signal handling async-signal-safe (no stdio/log calls in handlers) and add async-safe PID-file deletion via a cached PID path.
- Add/extend Free* helpers for multiple configuration/rule/list structures and refactor os_dbd DB calls toward per-config function pointers.
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/win32/win_agent.c | WIN32 entrypoint updates to pass cfg/accept flags into logcollector and updated config APIs. |
| src/syscheckd/syscheck.h | Syscheck config API updated for reload (Read/Free + cfgfile extern). |
| src/syscheckd/syscheck.c | Syscheck startup updated to pass config struct and set cfgfile for reload. |
| src/syscheckd/run_check.c | Adds SIGHUP-triggered syscheck config reload + signal masking around waits. |
| src/syscheckd/config.c | Refactors syscheck config parsing into provided struct + adds FreeSyscheckConfig. |
| src/shared/sig_op.c | Reworks SIGHUP handling (flag) and makes fatal signal handler async-signal-safe; switches to sigaction. |
| src/shared/list_op.c | Adds list cleanup/free helpers to support reload cleanup paths. |
| src/shared/file-queue.c | Handles EINTR from select() in file-queue sleep path. |
| src/shared/file_op.c | Caches PID path on CreatePID and adds DeletePID_AsyncSafe(). |
| src/remoted/syslogtcp.c | Adds loop-wide SIGHUP masking and reload logic around select(). |
| src/remoted/syslog.c | Adds loop-wide SIGHUP masking and reload logic around select(). |
| src/remoted/sendmsg.c | Exposes sendmsg mutex lock/unlock helpers for “lock-first” reload pattern. |
| src/remoted/secure.c | Adds SIGHUP masking/unmasking and thread-safe reload via sendmsg lock. |
| src/remoted/remoted.h | Adds FreeRemotedConfig, send_msg_lock/unlock, and cfgfile extern. |
| src/remoted/remoted.c | Defines cfgfile global for remoted reload. |
| src/remoted/main.c | Sets cfgfile for remoted reload logic. |
| src/remoted/config.c | Adds FreeRemotedConfig and initializes more fields for reload safety. |
| src/os_maild/maild.h | Adds FreeMailConfig prototype for reload cleanup. |
| src/os_maild/maild.c | Adds SIGHUP reload loop (copy-swap) and unblocks SIGHUP around queue wait. |
| src/os_maild/config.c | Adds FreeMailConfig and returns “disabled” as rc=1 for non-test runs. |
| src/os_execd/execd.c | Adds SIGHUP masking/unmasking around select() and placeholder reload hook. |
| src/os_dbd/server.c | Refactors DB queries to use function pointers on DBConfig. |
| src/os_dbd/rules.c | Refactors DB queries to use function pointers on DBConfig. |
| src/os_dbd/main.c | Sets cfgfile for reload and switches connect call to db_config function pointer. |
| src/os_dbd/dbd.h | Adds FreeDBConfig + cfgfile extern; updates signatures to non-const DBConfig. |
| src/os_dbd/dbd.c | Adds SIGHUP reload with reconnect + hash rebuild + copy-swap DBConfig. |
| src/os_dbd/db_op.h | Changes query function signatures to accept DBConfig; adds error/config helpers. |
| src/os_dbd/db_op.c | Removes global function-pointer dispatch; routes queries via DBConfig and updates error handling signatures. |
| src/os_dbd/config.c | Zero-initializes DBConfig for reload and adds FreeDBConfig + cfgfile global. |
| src/os_dbd/alert.c | Refactors DB queries to use function pointers on DBConfig; updates OS_SelectMaxID signature. |
| src/os_csyslogd/main.c | Sets cfgfile and adjusts test-config exit behavior when config is empty. |
| src/os_csyslogd/csyslogd.h | Adds FreeSyslogConfig + cfgfile extern. |
| src/os_csyslogd/csyslogd.c | Adds SIGHUP reload for syslog forwarding + unblocks SIGHUP around filemon wait. |
| src/os_csyslogd/config.c | Stops ErrorExit on config read failure; adds FreeSyslogConfig + cfgfile global. |
| src/os_auth/main-server.c | Adds SIGHUP masking/unmasking around waits/select; truncates agentname in error responses; uses DeletePID_AsyncSafe in cleanup. |
| src/os_auth/check_cert.h | Adds OpenSSL-compat macro for SSL_get1_peer_certificate on < 3.0. |
| src/os_auth/check_cert.c | Switches to SSL_get1_peer_certificate. |
| src/monitord/monitord.c | Adds SIGHUP masking/unmasking around sleep with placeholder reload hook. |
| src/logcollector/main.c | Updates LogCollectorConfig/Start call sites for new signatures and return types. |
| src/logcollector/logcollector.h | Updates signatures; adds Localfile_Init and Free_Localfile prototypes. |
| src/logcollector/logcollector.c | Adds Free_Localfile + Localfile_Init split and SIGHUP reload with copy-swap. |
| src/logcollector/config.c | Returns allocated logreader array (or NULL) instead of int status; cleans up on shared-config read failure. |
| src/init/ossec-server.sh | Changes “reload” action to send SIGHUP instead of stop/start. |
| src/init/ossec-local.sh | Adds “reload” action; updates usage text; fixes unlocking in stop flow. |
| src/init/ossec-client.sh | Changes “reload” action to send SIGHUP instead of stop/start. |
| src/headers/sig_op.h | Exposes sighup_received + HandleSIGHUP prototype. |
| src/headers/list_op.h | Exposes OSList_CleanNodes and OSList_Free. |
| src/headers/file_op.h | Exposes DeletePID_AsyncSafe and relaxes DeletePID nonnull attribute. |
| src/config/dbd-config.h | Adds DBConfig function pointers for connect/query/close/escape dispatch. |
| src/client-agent/receiver-win.c | Fixes WIN32 thread proc signature/return type. |
| src/client-agent/main.c | Sets cfgfile and updates ClientConf signature to accept agent pointer. |
| src/client-agent/config.c | Refactors ClientConf to accept explicit agent pointer; adds FreeAgentConfig and cfgfile global (non-WIN32). |
| src/client-agent/agentd.h | Updates ClientConf prototype; adds FreeAgentConfig + cfgfile extern; adjusts WIN32 receiver prototype. |
| src/client-agent/agentd.c | Adds SIGHUP reload with copy-swap agent config + signal masking around select(). |
| src/analysisd/rules.h | Adds rule free helpers prototypes. |
| src/analysisd/rules_list.c | Implements deep-free for RuleInfo and RuleNode trees; uses OSList_Free. |
| src/analysisd/lists.h | Adds OS_FreeLists prototype. |
| src/analysisd/lists_list.c | Implements OS_FreeLists for list rules/nodes. |
| src/analysisd/fts.h | Adds FTS_Free prototype. |
| src/analysisd/fts.c | Implements FTS_Free for list/hash/file resources. |
| src/analysisd/decoders/decoders_list.c | Adds decoder free helpers and includes analysisd config for decoder order size. |
| src/analysisd/decoders/decoder.h | Exposes decoder free helpers prototypes. |
| src/analysisd/config.h | Adds FreeConfig prototype. |
| src/analysisd/config.c | Implements FreeConfig for analysisd global config allocations. |
| src/analysisd/analysisd.c | Adds SIGHUP masking around queue wait; adjusts Eventinfo allocation lifetime and cleanup. |
| src/analysisd/active-response.h | Adds FreeARConfig prototype. |
| src/analysisd/active-response.c | Implements FreeARConfig using new OSList cleanup helpers. |
| src/analysisd/accumulator.h | Adds Accumulate_Free prototype. |
| src/analysisd/accumulator.c | Implements Accumulate_Free to release accumulator hash content. |
| src/agentlessd/main.c | Sets cfgfile for reload. |
| src/agentlessd/agentlessd.h | Adds FreeAgentlessConfig prototype + cfgfile extern. |
| src/agentlessd/agentlessd.c | Adds SIGHUP reload with copy-swap agentless config + masking around sleep. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+166
to
173
| if (!config->dir) { | ||
| config->dir = SYSCHECK_EMPTY; | ||
| } | ||
| if (!syscheck.registry) { | ||
| syscheck.registry = SYSCHECK_EMPTY; | ||
| if (!config->registry) { | ||
| config->registry = SYSCHECK_EMPTY; | ||
| } | ||
| if ((syscheck.dir[0] == NULL) && (syscheck.registry[0] == NULL)) { | ||
| if ((config->dir[0] == NULL) && (config->registry[0] == NULL)) { | ||
| return (1); |
Comment on lines
+24
to
+42
| /* Free the syscheck configuration */ | ||
| void FreeSyscheckConfig(syscheck_config *config) | ||
| { | ||
| int i = 0; | ||
|
|
||
| if (config->dir) { | ||
| while (config->dir[i]) { | ||
| free(config->dir[i]); | ||
| i++; | ||
| } | ||
| free(config->dir); | ||
| config->dir = NULL; | ||
| } | ||
|
|
||
| if (config->opts) { | ||
| free(config->opts); | ||
| config->opts = NULL; | ||
| } | ||
|
|
Comment on lines
+197
to
+206
| if (sighup_received) { | ||
| syscheck_config new_syscheck; | ||
| memset(&new_syscheck, 0, sizeof(syscheck_config)); | ||
|
|
||
| sighup_received = 0; | ||
| merror("%s: INFO: SIGHUP received. Reloading configuration.", ARGV0); | ||
|
|
||
| if (Read_Syscheck_Config(cfgfile, &new_syscheck) < 0) { | ||
| merror("%s: ERROR: Error reloading configuration (using old config)", ARGV0); | ||
| } else { |
Comment on lines
+26
to
+30
| #ifndef WIN32 | ||
| volatile sig_atomic_t sighup_received = 0; | ||
| #else | ||
| volatile int sighup_received = 0; | ||
| #endif |
Comment on lines
283
to
295
| @@ -259,13 +295,43 @@ void LogCollectorStart() | |||
| } | |||
Comment on lines
+179
to
+181
| if (RemotedConfig(cfgfile, &new_logr) < 0) { | ||
| merror("%s: ERROR: Error reloading configuration (using old config)", ARGV0); | ||
| } else { |
Comment on lines
+100
to
+102
| if (RemotedConfig(cfgfile, &new_logr) < 0) { | ||
| merror("%s: ERROR: Error reloading configuration (using old config)", ARGV0); | ||
| } else { |
Comment on lines
+38
to
+40
| result = db_config->db_query_select(db_config, sql_query); | ||
| if (result == 0) { | ||
| } |
Comment on lines
+36
to
+38
| result = db_config->db_query_select(db_config, sql_query); | ||
| if (result == 0) { | ||
| } |
Comment on lines
+316
to
+323
| /* filename and matcher are pointers that might be shared */ | ||
| /* If they were allocated in OS_AddListRule, they should be freed */ | ||
| /* In OS_AddListRule, filename is assigned from the argument. */ | ||
| /* Let's assume the callers of OS_AddListRule provide stable pointers | ||
| * or we need to free them. | ||
| * Looking at OS_AddListRule in lists_list.c: | ||
| * new_rulelist_pt->filename = listname; | ||
| * new_rulelist_pt->matcher = matcher; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes:
sigprocmaskacross all daemons (analysisd, remoted, agentd, logcollector, etc.) to prevent signal re-entrancy during critical processing and memory operations.shared/sig_op.csignal handler (HandleSIG) to be strictly async-signal-safe, eliminating unsafesnprintfandlog2filein favor of directwrite()syscalls and a pre-cached PID path.DeletePID_AsyncSafe()usingunlink()for safe PID file removal in signal handler context.remoted/secure.cto prevent deadlocks between main and worker threads during reloads.Eventinfocleanup viaFree_Eventinfo().SA_RESTARTforSIGHUPto ensure immediate reload responsiveness.These changes eliminate race conditions, memory leaks, and potential deadlocks triggered by SIGHUP while maintaining project-wide build integrity.