From 20781e8a28f5e9fe13f1c96e7feef18c8dd885fe Mon Sep 17 00:00:00 2001 From: csharp36 Date: Sun, 22 Feb 2026 18:35:01 -0500 Subject: [PATCH] Fix critical bugs: signal safety, packet parsing, fd leaks - Signal handler (intHandler): move non-async-signal-safe calls (LOG, fork, sleep, stopPacketLogger, run_aqualinkd_upgrade) out of the handler and into the post-main-loop cleanup path. The handler now only sets volatile flags and optionally closes the blocking fd. - Packet parsing (fix_packet): change loop bound from i <= packet_length to i < packet_length - 1 to prevent buffer over-read when checking packet_buffer[i+1]. - Pentair packet construction (send_pentair_command): replace pre-increment packet[++i] pattern with direct indexing to eliminate a gap byte and extra trailing NUL in sent packets. - Serial port init (_init_serial_port): add close(fd) on all error paths after open() succeeds to prevent file descriptor leaks. The tcsetattr failure path also calls unlock_port(fd) since the port was locked. Co-Authored-By: Claude Opus 4.6 --- source/aq_serial.c | 18 ++++++------ source/aqualinkd.c | 72 ++++++++++++++++++++++++---------------------- 2 files changed, 48 insertions(+), 42 deletions(-) diff --git a/source/aq_serial.c b/source/aq_serial.c index a3a8fd6..44f23bb 100644 --- a/source/aq_serial.c +++ b/source/aq_serial.c @@ -599,11 +599,13 @@ int _init_serial_port(const char* tty, bool blocking, bool readahead) if (tcgetattr(fd, &newtio) != 0) { LOG(RSSD_LOG,LOG_ERR, "Unable to get port attributes: %s, error %d\n", tty,errno); + close(fd); return -1; } if ( lock_port(fd, tty) < 0) { //LOG(RSSD_LOG,LOG_ERR, "Unable to lock port: %s, error %d\n", tty, errno); + close(fd); return -1; } @@ -656,6 +658,8 @@ int _init_serial_port(const char* tty, bool blocking, bool readahead) tcflush(fd, TCIFLUSH); if (tcsetattr(fd, TCSANOW, &newtio) != 0) { LOG(RSSD_LOG,LOG_ERR, "Unable to set port attributes: %s, error %d\n", tty,errno); + unlock_port(fd); + close(fd); return -1; } @@ -755,14 +759,12 @@ void send_pentair_command(int fd, unsigned char *packet_buffer, int size) //packet[i] = packet_buffer[i-4]; } - packet[++i] = NUL; // Checksum - packet[++i] = NUL; // Checksum - generate_pentair_checksum(&packet[1], i); - //packet[++i] = NUL; + // Place checksum placeholders immediately after data (no gap byte) + packet[i] = NUL; // Checksum high + packet[i+1] = NUL; // Checksum low + generate_pentair_checksum(&packet[1], i+2); - - //logPacket(packet, i); - send_packet(fd,packet,++i); + send_packet(fd, packet, i+2); } #ifndef SEND_CMD_WITH_TRAILING_NUL @@ -1077,7 +1079,7 @@ int fix_packet(unsigned char *packet_buffer, int packet_length, bool getCached) // Ignore the first two bytes, assuming they are 0x10, 0x02 - for (int i=2; i <= packet_length; i++ ) { + for (int i=2; i < packet_length - 1; i++ ) { if (packet_buffer[i] == DLE && packet_buffer[i+1] == STX) { LOG(RSSD_LOG,LOG_DEBUG, "Packet Fix: found start in middle of frame\n"); int p1_length = i; diff --git a/source/aqualinkd.c b/source/aqualinkd.c index 5a852c4..1d8cb03 100644 --- a/source/aqualinkd.c +++ b/source/aqualinkd.c @@ -72,6 +72,8 @@ static volatile bool _keepRunning = true; static volatile bool _restart = false; +static volatile sig_atomic_t _received_signal = 0; +static volatile sig_atomic_t _upgrade_requested = 0; //char** _argv; //static struct aqconfig _aqconfig_; static struct aqualinkdata _aqualink_data; @@ -103,49 +105,25 @@ bool isAqualinkDStopping() { void intHandler(int sig_num) { - if (sig_num == SIGRUPGRADE) { - if (! run_aqualinkd_upgrade(false)) { - LOG(AQUA_LOG,LOG_ERR, "AqualinkD upgrade failed!\n"); - } - return; // Let the upgrade process terminate us. - } + // Only perform async-signal-safe operations here: + // set flags and let the main loop handle cleanup. + _received_signal = sig_num; - LOG(AQUA_LOG,LOG_WARNING, "Stopping!\n"); + if (sig_num == SIGRUPGRADE) { + _upgrade_requested = 1; + return; + } _keepRunning = false; if (sig_num == SIGRESTART) { - LOG(AQUA_LOG,LOG_WARNING, "Restarting AqualinkD!\n"); - // If we are deamonized, we need to use the system - if (_aqconfig_.deamonize) { - if(fork() == 0) { - sleep(2); - char *newargv[] = {"/bin/systemctl", "restart", "aqualinkd", NULL}; - char *newenviron[] = { NULL }; - execve(newargv[0], newargv, newenviron); - exit (EXIT_SUCCESS); - } - } else { - _restart = true; - } + _restart = true; } - //LOG(AQUA_LOG,LOG_NOTICE, "Stopping!\n"); - //if (dummy){}// stop compile warnings - // In blocking mode, die as cleanly as possible. -#ifdef AQ_NO_THREAD_NETSERVICE - if (_aqconfig_.rs_poll_speed < 0) { - stopPacketLogger(); - // This should force port to close and do somewhat gracefull exit. - close_blocking_serial_port(); - //exit(-1); - } -#else - stopPacketLogger(); - // This should force port to close and do somewhat gracefull exit. + // In blocking mode, force the serial port closed so the blocking + // read returns and the main loop can exit. close() is async-signal-safe. if (serial_blockingmode()) close_blocking_serial_port(); -#endif } bool isVirtualButtonEnabled() { @@ -1531,6 +1509,32 @@ void main_loop() //delay(10); } + // Handle deferred signal actions (moved out of signal handler for async-signal-safety). + if (_received_signal != 0) { + LOG(AQUA_LOG,LOG_WARNING, "Received signal %d, stopping!\n", _received_signal); + } + + if (_upgrade_requested) { + _upgrade_requested = 0; + if (! run_aqualinkd_upgrade(false)) { + LOG(AQUA_LOG,LOG_ERR, "AqualinkD upgrade failed!\n"); + } + // Let the upgrade process terminate us. + } + + if (_restart && _aqconfig_.deamonize) { + // When daemonized, fork a child to restart via systemctl + LOG(AQUA_LOG,LOG_WARNING, "Restarting AqualinkD (daemonized)!\n"); + if(fork() == 0) { + sleep(2); + char *newargv[] = {"/bin/systemctl", "restart", "aqualinkd", NULL}; + char *newenviron[] = { NULL }; + execve(newargv[0], newargv, newenviron); + exit(EXIT_SUCCESS); + } + _restart = false; // Let the parent exit normally + } + //if (_aqconfig_.debug_RSProtocol_packets) stopPacketLogger(); stopPacketLogger();