From b08dccee31903dc9cc5b0357561991d49bce005b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Thu, 13 Jun 2024 21:04:05 +0200 Subject: [PATCH] Extend sendsignal*() API some more to optionally fall back to checkprocname() vs current getpid() [#2463] Signed-off-by: Jim Klimov --- NEWS.adoc | 3 +- UPGRADING.adoc | 3 +- clients/upsmon.c | 6 +- common/common.c | 137 +++++++++++++++++++++++++++++++++----- drivers/main.c | 12 ++-- drivers/upsdrvctl.c | 36 +++++----- include/common.h | 14 ++-- scripts/Windows/wininit.c | 4 +- server/upsd.c | 6 +- 9 files changed, 167 insertions(+), 54 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index 7de6de1172..a7279a9e66 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -145,7 +145,8 @@ https://github.com/networkupstools/nut/milestone/11 significantly from a `progname` string defined in the respective NUT source file, so a boolean `NUT_IGNORE_CHECKPROCNAME` environment variable support was added to optionally disable this verification. - [issue #2463] + Also the NUT daemons should request to double-check against their + run-time process name (if it can be detected). [issue #2463] - various recipe, documentation and source files were revised to address respective warnings issued by the new generations of analysis tools. diff --git a/UPGRADING.adoc b/UPGRADING.adoc index 7f7877c09e..0c5a7ac125 100644 --- a/UPGRADING.adoc +++ b/UPGRADING.adoc @@ -46,7 +46,8 @@ Changes from 2.8.2 to 2.8.3 names differ significantly from a `progname` defined in the respective NUT source file, so a boolean `NUT_IGNORE_CHECKPROCNAME` environment variable support was added to optionally disable this verification. - [issue #2463] + Also the NUT daemons should request to double-check against their + run-time process name (if it can be detected). [issue #2463] Changes from 2.8.1 to 2.8.2 diff --git a/clients/upsmon.c b/clients/upsmon.c index 6dd7c2b9ca..72314267dc 100644 --- a/clients/upsmon.c +++ b/clients/upsmon.c @@ -3022,15 +3022,15 @@ int main(int argc, char *argv[]) * is running by sending signal '0' (i.e. 'kill 0' equivalent) */ if (oldpid < 0) { - cmdret = sendsignal(prog, cmd); + cmdret = sendsignal(prog, cmd, 1); } else { - cmdret = sendsignalpid(oldpid, cmd, prog); + cmdret = sendsignalpid(oldpid, cmd, prog, 1); } #else /* WIN32 */ if (cmd) { /* Command the running daemon, it should be there */ - cmdret = sendsignal(UPSMON_PIPE_NAME, cmd); + cmdret = sendsignal(UPSMON_PIPE_NAME, cmd, 1); } else { /* Starting new daemon, check for competition */ mutex = CreateMutex(NULL, TRUE, UPSMON_PIPE_NAME); diff --git a/common/common.c b/common/common.c index 86c67ac472..b27f464eab 100644 --- a/common/common.c +++ b/common/common.c @@ -1150,10 +1150,11 @@ void writepid(const char *name) /* send sig to pid, returns -1 for error, or * zero for a successfully sent signal */ -int sendsignalpid(pid_t pid, int sig, const char *progname) +int sendsignalpid(pid_t pid, int sig, const char *progname, int check_current_progname) { #ifndef WIN32 - int ret; + int ret, cpn1 = -10, cpn2 = -10; + char *current_progname = NULL; /* TOTHINK: What about containers where a NUT daemon *is* the only process * and is the PID=1 of the container (recycle if dead)? */ @@ -1165,13 +1166,114 @@ int sendsignalpid(pid_t pid, int sig, const char *progname) return -1; } - if (progname && !checkprocname(pid, progname)) { - if (nut_debug_level > 0 || nut_sendsignal_debug_level > 1) - upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX - " which exists but is not of program '%s'", - (uintmax_t)pid, progname); + ret = 0; + if (progname) { + /* Check against some expected (often built-in) name */ + if (!(cpn1 = checkprocname(pid, progname))) { + /* Did not match expected (often built-in) name */ + ret = -1; + } else { + if (cpn1 > 0) { + /* Matched expected name, ok to proceed */ + ret = 1; + } + /* ...else could not determine name of PID; think later */ + } + } + /* if (cpn1 == -3) => NUT_IGNORE_CHECKPROCNAME=true */ + /* if (cpn1 == -1) => could not determine name of PID... retry just in case? */ + if (ret <= 0 && check_current_progname && cpn1 != -3) { + /* NOTE: This could be optimized a bit by pre-finding the procname + * of "pid" and re-using it, but this is not a hot enough code path + * to bother much. + */ + current_progname = getprocname(getpid()); + if (current_progname && (cpn2 = checkprocname(pid, current_progname))) { + if (cpn2 > 0) { + /* Matched current process as asked, ok to proceed */ + ret = 2; + } + /* ...else could not determine name of PID; think later */ + } else { + if (current_progname) { + /* Did not match current process name */ + ret = -2; + } /* else just did not determine current process + * name, so did not actually check either + * // ret = -3; + */ + } + } + + /* if ret == 0, ok to proceed - not asked for any sanity checks; + * if ret > 0 we had some definitive match above + */ + if (ret < 0) { + upsdebugx(1, + "%s: ran at least one check, and all such checks " + "found a process name for PID %" PRIuMAX " and " + "failed to match: expected progname='%s' (res=%d), " + "current progname='%s' (res=%d)", + __func__, (uintmax_t)pid, + NUT_STRARG(progname), cpn1, + NUT_STRARG(current_progname), cpn2); + + if (nut_debug_level > 0 || nut_sendsignal_debug_level > 1) { + switch (ret) { + case -1: + upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX + " which exists but is not of" + " expected program '%s'; not asked" + " to cross-check current PID's name", + (uintmax_t)pid, progname); + break; + + /* Maybe we tried both data sources, maybe just current_progname */ + case -2: + /*case -3:*/ + if (progname && current_progname) { + /* Tried both, downgraded verdict further */ + upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX + " which exists but is not of expected" + " program '%s' nor current '%s'", + (uintmax_t)pid, progname, current_progname); + } else if (current_progname) { + /* Not asked for progname==NULL */ + upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX + " which exists but is not of" + " current program '%s'", + (uintmax_t)pid, current_progname); + } else if (progname) { + upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX + " which exists but is not of" + " expected program '%s'; could not" + " cross-check current PID's name", + (uintmax_t)pid, progname); + } else { + /* Both NULL; one not asked, another not detected; + * should not actually get here (wannabe `ret==-3`) + */ + upslogx(LOG_ERR, "Tried to signal PID %" PRIuMAX + " but could not cross-check current PID's" + " name: did not expect to get here", + (uintmax_t)pid); + } + break; + } + } + + if (current_progname) { + free(current_progname); + current_progname = NULL; + } + + /* Logged or not, sanity-check was requested and failed */ return -1; } + if (current_progname) { + free(current_progname); + current_progname = NULL; + } /* see if this is going to work first - does the process exist, * and do we have permissions to signal it? */ @@ -1199,6 +1301,8 @@ int sendsignalpid(pid_t pid, int sig, const char *progname) NUT_UNUSED_VARIABLE(pid); NUT_UNUSED_VARIABLE(sig); NUT_UNUSED_VARIABLE(progname); + NUT_UNUSED_VARIABLE(check_current_progname); + /* Windows builds use named pipes, not signals per se */ upslogx(LOG_ERR, "%s: not implemented for Win32 and " "should not have been called directly!", @@ -1239,7 +1343,7 @@ pid_t parsepid(const char *buf) * zero for a successfully sent signal */ #ifndef WIN32 -int sendsignalfn(const char *pidfn, int sig, const char *progname) +int sendsignalfn(const char *pidfn, int sig, const char *progname, int check_current_progname) { char buf[SMALLBUF]; FILE *pidf; @@ -1275,7 +1379,7 @@ int sendsignalfn(const char *pidfn, int sig, const char *progname) if (pid >= 0) { /* this method actively reports errors, if any */ - ret = sendsignalpid(pid, sig, progname); + ret = sendsignalpid(pid, sig, progname, check_current_progname); } fclose(pidf); @@ -1284,10 +1388,11 @@ int sendsignalfn(const char *pidfn, int sig, const char *progname) #else /* => WIN32 */ -int sendsignalfn(const char *pidfn, const char * sig, const char *progname_ignored) +int sendsignalfn(const char *pidfn, const char * sig, const char *progname_ignored, int check_current_progname_ignored) { BOOL ret; NUT_UNUSED_VARIABLE(progname_ignored); + NUT_UNUSED_VARIABLE(check_current_progname_ignored); ret = send_to_named_pipe(pidfn, sig); @@ -1350,19 +1455,21 @@ int snprintfcat(char *dst, size_t size, const char *fmt, ...) /* lazy way to send a signal if the program uses the PIDPATH */ #ifndef WIN32 -int sendsignal(const char *progname, int sig) +int sendsignal(const char *progname, int sig, int check_current_progname) { char fn[SMALLBUF]; snprintf(fn, sizeof(fn), "%s/%s.pid", rootpidpath(), progname); - return sendsignalfn(fn, sig, progname); + return sendsignalfn(fn, sig, progname, check_current_progname); } #else -int sendsignal(const char *progname, const char * sig) +int sendsignal(const char *progname, const char * sig, int check_current_progname) { - /* progname is used as the pipe name for WIN32 */ - return sendsignalfn(progname, sig, NULL); + /* progname is used as the pipe name for WIN32 + * check_current_progname is de-facto ignored + */ + return sendsignalfn(progname, sig, NULL, check_current_progname); } #endif diff --git a/drivers/main.c b/drivers/main.c index 67d853fd81..788a1aa163 100644 --- a/drivers/main.c +++ b/drivers/main.c @@ -2223,9 +2223,9 @@ int main(int argc, char **argv) int cmdret = -1; /* Send a signal to older copy of the driver, if any */ if (oldpid < 0) { - cmdret = sendsignalfn(pidfnbuf, cmd, progname); + cmdret = sendsignalfn(pidfnbuf, cmd, progname, 1); } else { - cmdret = sendsignalpid(oldpid, cmd, progname); + cmdret = sendsignalpid(oldpid, cmd, progname, 1); } switch (cmdret) { @@ -2321,7 +2321,7 @@ int main(int argc, char **argv) upslogx(LOG_WARNING, "Duplicate driver instance detected (PID file %s exists)! Terminating other driver!", pidfnbuf); - if ((sigret = sendsignalfn(pidfnbuf, SIGTERM, progname) != 0)) { + if ((sigret = sendsignalfn(pidfnbuf, SIGTERM, progname, 1) != 0)) { upsdebugx(1, "Can't send signal to PID, assume invalid PID file %s; " "sendsignalfn() returned %d (errno=%d): %s", pidfnbuf, sigret, errno, strerror(errno)); @@ -2339,9 +2339,9 @@ int main(int argc, char **argv) struct stat st; if (stat(pidfnbuf, &st) == 0) { upslogx(LOG_WARNING, "Duplicate driver instance is still alive (PID file %s exists) after several termination attempts! Killing other driver!", pidfnbuf); - if (sendsignalfn(pidfnbuf, SIGKILL, progname) == 0) { + if (sendsignalfn(pidfnbuf, SIGKILL, progname, 1) == 0) { sleep(5); - if (sendsignalfn(pidfnbuf, 0, progname) == 0) { + if (sendsignalfn(pidfnbuf, 0, progname, 1) == 0) { upslogx(LOG_WARNING, "Duplicate driver instance is still alive (could signal the process)"); /* TODO: Should we writepid() below in this case? * Or if driver init fails, restore the old content @@ -2385,7 +2385,7 @@ int main(int argc, char **argv) upslogx(LOG_WARNING, "Duplicate driver instance detected! Terminating other driver!"); for(i=0;i<10;i++) { DWORD res; - sendsignal(name, COMMAND_STOP); + sendsignal(name, COMMAND_STOP, 1); if(mutex != NULL ) { res = WaitForSingleObject(mutex,1000); if(res==WAIT_OBJECT_0) { diff --git a/drivers/upsdrvctl.c b/drivers/upsdrvctl.c index 3d9295f13e..5eb62f839c 100644 --- a/drivers/upsdrvctl.c +++ b/drivers/upsdrvctl.c @@ -304,9 +304,9 @@ static void signal_driver_cmd(const ups_t *ups, nut_sendsignal_debug_level = NUT_SENDSIGNAL_DEBUG_LEVEL_KILL_SIG0PING - 1; #ifndef WIN32 if (ups->pid == -1) { - ret = sendsignalfn(pidfn, cmd, ups->driver); + ret = sendsignalfn(pidfn, cmd, ups->driver, 0); } else { - ret = sendsignalpid(ups->pid, cmd, ups->driver); + ret = sendsignalpid(ups->pid, cmd, ups->driver, 0); /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { upslog_with_errno(LOG_WARNING, @@ -315,7 +315,7 @@ static void signal_driver_cmd(const ups_t *ups, } } #else - ret = sendsignal(pidfn, cmd); + ret = sendsignal(pidfn, cmd, 0); #endif /* Restore the signal errors verbosity */ nut_sendsignal_debug_level = NUT_SENDSIGNAL_DEBUG_LEVEL_DEFAULT; @@ -381,25 +381,25 @@ static void stop_driver(const ups_t *ups) #ifndef WIN32 if (ups->pid == -1) { - ret = sendsignalfn(pidfn, SIGTERM, ups->driver); + ret = sendsignalfn(pidfn, SIGTERM, ups->driver, 0); } else { - ret = sendsignalpid(ups->pid, SIGTERM, ups->driver); + ret = sendsignalpid(ups->pid, SIGTERM, ups->driver, 0); /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { goto clean_return; } } #else - ret = sendsignal(pidfn, COMMAND_STOP); + ret = sendsignal(pidfn, COMMAND_STOP, 0); #endif if (ret < 0) { #ifndef WIN32 upsdebugx(2, "SIGTERM to %s failed, retrying with SIGKILL", pidfn); if (ups->pid == -1) { - ret = sendsignalfn(pidfn, SIGKILL, ups->driver); + ret = sendsignalfn(pidfn, SIGKILL, ups->driver, 0); } else { - ret = sendsignalpid(ups->pid, SIGKILL, ups->driver); + ret = sendsignalpid(ups->pid, SIGKILL, ups->driver, 0); /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { goto clean_return; @@ -407,7 +407,7 @@ static void stop_driver(const ups_t *ups) } #else upsdebugx(2, "Stopping %s failed, retrying again", pidfn); - ret = sendsignal(pidfn, COMMAND_STOP); + ret = sendsignal(pidfn, COMMAND_STOP, 0); #endif if (ret < 0) { upslog_with_errno(LOG_ERR, "Stopping %s failed", pidfn); @@ -419,16 +419,16 @@ static void stop_driver(const ups_t *ups) for (i = 0; i < 5 ; i++) { #ifndef WIN32 if (ups->pid == -1) { - ret = sendsignalfn(pidfn, 0, ups->driver); + ret = sendsignalfn(pidfn, 0, ups->driver, 0); } else { /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { goto clean_return; } - ret = sendsignalpid(ups->pid, 0, ups->driver); + ret = sendsignalpid(ups->pid, 0, ups->driver, 0); } #else - ret = sendsignalfn(pidfn, 0, ups->driver); + ret = sendsignalfn(pidfn, 0, ups->driver, 0); #endif if (ret != 0) { upsdebugx(2, "Sending signal to %s failed, driver is finally down or wrongly owned", pidfn); @@ -440,32 +440,32 @@ static void stop_driver(const ups_t *ups) #ifndef WIN32 upslog_with_errno(LOG_ERR, "Stopping %s failed, retrying harder", pidfn); if (ups->pid == -1) { - ret = sendsignalfn(pidfn, SIGKILL, ups->driver); + ret = sendsignalfn(pidfn, SIGKILL, ups->driver, 0); } else { /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { goto clean_return; } - ret = sendsignalpid(ups->pid, SIGKILL, ups->driver); + ret = sendsignalpid(ups->pid, SIGKILL, ups->driver, 0); } #else upslog_with_errno(LOG_ERR, "Stopping %s failed, retrying again", pidfn); - ret = sendsignal(pidfn, COMMAND_STOP); + ret = sendsignal(pidfn, COMMAND_STOP, 0); #endif if (ret == 0) { for (i = 0; i < 5 ; i++) { #ifndef WIN32 if (ups->pid == -1) { - ret = sendsignalfn(pidfn, 0, ups->driver); + ret = sendsignalfn(pidfn, 0, ups->driver, 0); } else { /* reap zombie if this child died */ if (waitpid(ups->pid, NULL, WNOHANG) == ups->pid) { goto clean_return; } - ret = sendsignalpid(ups->pid, 0, ups->driver); + ret = sendsignalpid(ups->pid, 0, ups->driver, 0); } #else - ret = sendsignalfn(pidfn, 0, ups->driver); + ret = sendsignalfn(pidfn, 0, ups->driver, 0); #endif if (ret != 0) { upsdebugx(2, "Sending signal to %s failed, driver is finally down or wrongly owned", pidfn); diff --git a/include/common.h b/include/common.h index 1a1e693149..9313245d63 100644 --- a/include/common.h +++ b/include/common.h @@ -268,9 +268,9 @@ pid_t parsepid(const char *buf); /* send a signal to another running NUT process */ #ifndef WIN32 -int sendsignal(const char *progname, int sig); +int sendsignal(const char *progname, int sig, int check_current_progname); #else -int sendsignal(const char *progname, const char * sig); +int sendsignal(const char *progname, const char * sig, int check_current_progname); #endif int snprintfcat(char *dst, size_t size, const char *fmt, ...) @@ -281,7 +281,7 @@ pid_t get_max_pid_t(void); /* send sig to pid after some sanity checks, returns * -1 for error, or zero for a successfully sent signal */ -int sendsignalpid(pid_t pid, int sig, const char *progname); +int sendsignalpid(pid_t pid, int sig, const char *progname, int check_current_progname); /* open , get the pid, then send it * returns zero for successfully sent signal, @@ -293,11 +293,15 @@ int sendsignalpid(pid_t pid, int sig, const char *progname); #ifndef WIN32 /* open , get the pid, then send it * if executable process with that pid has suitable progname + * (specified or that of the current process, depending on args: + * most daemons request to check_current_progname for their other + * process instancees, but upsdrvctl which manages differently + * named driver programs does not request it) */ -int sendsignalfn(const char *pidfn, int sig, const char *progname); +int sendsignalfn(const char *pidfn, int sig, const char *progname, int check_current_progname); #else /* No progname here - communications via named pipe */ -int sendsignalfn(const char *pidfn, const char * sig, const char *progname_ignored); +int sendsignalfn(const char *pidfn, const char * sig, const char *progname_ignored, int check_current_progname_ignored); #endif const char *xbasename(const char *file); diff --git a/scripts/Windows/wininit.c b/scripts/Windows/wininit.c index 6c81929f8f..abb6643090 100644 --- a/scripts/Windows/wininit.c +++ b/scripts/Windows/wininit.c @@ -193,7 +193,7 @@ static void run_upsd(void) static void stop_upsd(void) { - if (sendsignal(UPSD_PIPE_NAME, COMMAND_STOP)) { + if (sendsignal(UPSD_PIPE_NAME, COMMAND_STOP, 0)) { print_event(LOG_ERR, "Error stopping upsd (%d)", GetLastError()); } } @@ -215,7 +215,7 @@ static void run_upsmon(void) static void stop_upsmon(void) { - if (sendsignal(UPSMON_PIPE_NAME, COMMAND_STOP)) { + if (sendsignal(UPSMON_PIPE_NAME, COMMAND_STOP, 0)) { print_event(LOG_ERR, "Error stopping upsmon (%d)", GetLastError()); } } diff --git a/server/upsd.c b/server/upsd.c index 63483f8d76..833b269395 100644 --- a/server/upsd.c +++ b/server/upsd.c @@ -2049,14 +2049,14 @@ int main(int argc, char **argv) */ if (oldpid < 0) { - cmdret = sendsignalfn(pidfn, cmd, progname); + cmdret = sendsignalfn(pidfn, cmd, progname, 1); } else { - cmdret = sendsignalpid(oldpid, cmd, progname); + cmdret = sendsignalpid(oldpid, cmd, progname, 1); } #else /* if WIN32 */ if (cmd) { /* Command the running daemon, it should be there */ - cmdret = sendsignal(UPSD_PIPE_NAME, cmd); + cmdret = sendsignal(UPSD_PIPE_NAME, cmd, 1); } else { /* Starting new daemon, check for competition */ mutex = CreateMutex(NULL, TRUE, UPSD_PIPE_NAME);