clients/upsmon.c, clients/upssched.c, docs: handle NOTIFYCMD and CMDSCRIPT values strictly as paths [#3499]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

Author: jimklimov

NEWS.adoc

@@ -185,6 +185,14 @@ https://github.com/networkupstools/nut/milestone/13
       success of SSL initialization with both backends is more diligently
       tracked) should now not cause exit of the client if secure connection
       is not required by its configuration in the first place. [#3420]
+    * Potentially a breaking change for existing deployments with `upsmon` and
+      `upssched` integration: the `CMDSCRIPT` and `NOTIFYCMD` definitions were
+      revised to mean an exact program name (possibly with spaces in the value),
+      not a shell scriptlet. Documentation in earlier NUT releases implied that
+      arguments could be passed here -- this is no longer true. If you need to
+      pass special arguments to a notification program, please write a small
+      script to do so and specify its path here. The `SHUTDOWNCMD` is still
+      a scriptlet, as far as specifying command arguments is concerned. [#3499]
 
  - `NUT-Monitor` Python GUI client:
     * Fixed Qt tray tooltips to render in plain text, not rich text which

UPGRADING.adoc

@@ -46,6 +46,14 @@ Changes from 2.8.5 to 2.8.6
   if the requested value is larger than what is allowed (minus some reserve
   for configuration files and other use-cases). [issue #3365]
 
+- Potentially a breaking change for existing deployments with `upsmon` and
+  `upssched` integration: the `CMDSCRIPT` and `NOTIFYCMD` definitions were
+  revised to mean an exact program name (possibly with spaces in the value),
+  not a shell scriptlet. Documentation in earlier NUT releases implied that
+  arguments could be passed here -- this is no longer true. If you need to
+  pass special arguments to a notification program, please write a small
+  script to do so and specify its path here. The `SHUTDOWNCMD` is still
+  a scriptlet, as far as specifying command arguments is concerned. [#3499]
 
 Changes from 2.8.4 to 2.8.5
 ---------------------------

clients/upsmon.c

@@ -238,25 +238,18 @@ static void wall(const char *text)
 	pclose(wf);
 #else	/* WIN32 */
 #	define MESSAGE_CMD "message.exe"
-	char	*command;
-
-	/* first +1 is for the space between message and text
-	 * second +1 is for trailing 0
-	 * +2 is for ""
-	 */
-	size_t	commandsz = strlen(MESSAGE_CMD) + 1 + 2 + strlen(text) + 1;
+	char	*argv[3];
+	int	ret;
 
-	command = malloc (commandsz);
-	if (command == NULL) {
-		upslog_with_errno(LOG_NOTICE, "Not enough memory for wall");
-		return;
-	}
+	argv[0] = MESSAGE_CMD;
+	argv[1] = (char *)text;
+	argv[2] = NULL;
 
-	snprintf(command, commandsz, "%s \"%s\"", MESSAGE_CMD, text);
-	if (system(command) != 0) {
-		upslog_with_errno(LOG_NOTICE, "Can't invoke wall");
+	upsdebugx(6, "%s: executing %s with message: %s", __func__, MESSAGE_CMD, NUT_STRARG(text));
+	ret = _spawnvp(_P_WAIT, MESSAGE_CMD, (const char * const *)argv);
+	if (ret != 0) {
+		upslog_with_errno(LOG_NOTICE, "Can't invoke wall (status: %d)", ret);
 	}
-	free(command);
 #endif	/* WIN32 */
 }
 
@@ -271,32 +264,37 @@ typedef struct async_notify_s {
 
 static unsigned __stdcall async_notify(LPVOID param)
 {
-	char	exec[LARGEBUF];
-	char	notice[LARGEBUF];
+	char	*argv[3];
+	int	ret;
 
 	/* the following code is a copy of the content of the NOT WIN32 part of
-	"notify" function below */
+	 * "notify" function below */
 
 	async_notify_t *data = (async_notify_t *)param;
 
 	if (flag_isset(data->flags, NOTIFY_WALL)) {
-		snprintf(notice,LARGEBUF,"%s: %s", data->date, data->notice);
+		char	notice[LARGEBUF];
+
+		snprintf(notice, sizeof(notice), "%s: %s", data->date, data->notice);
 		wall(notice);
 	}
 
 	if (flag_isset(data->flags, NOTIFY_EXEC)) {
 		if (notifycmd != NULL) {
-			snprintf(exec, sizeof(exec), "%s \"%s\"", notifycmd, data->notice);
-
-			upsdebugx(6, "%s: Calling NOTIFYCMD: %s", __func__, exec);
+			upsdebugx(6, "%s: Calling NOTIFYCMD: %s with notice: %s",
+				__func__, notifycmd, NUT_STRARG(data->notice));
 			if (data->upsname)
 				setenv("UPSNAME", data->upsname, 1);
 			else
 				setenv("UPSNAME", "", 1);
 
 			setenv("NOTIFYTYPE", data->ntype, 1);
-			if (system(exec) == -1) {
-				upslog_with_errno(LOG_ERR, "%s", __func__);
+			argv[0] = notifycmd;
+			argv[1] = data->notice;
+			argv[2] = NULL;
+			ret = _spawnvp(_P_WAIT, notifycmd, (const char * const *)argv);
+			if (ret == -1) {
+				upslog_with_errno(LOG_ERR, "%s: _spawnvp failed", __func__);
 			}
 		}
 	}
@@ -314,12 +312,11 @@ static void notify(const char *notice, unsigned int flags, const char *ntype,
 			const char *upsname)
 {
 #ifndef WIN32
-	char	exec[LARGEBUF];
 	int	ret;
 #endif	/* !WIN32 */
 
 	upsdebugx(6, "%s: sending notification for [%s]: type %s with flags 0x%04x: %s",
-		__func__, upsname ? upsname : "upsmon itself", ntype, flags, notice);
+		__func__, upsname ? upsname : "upsmon itself", ntype, flags, NUT_STRARG(notice));
 
 	if (flag_isset(flags, NOTIFY_IGNORE)) {
 		upsdebugx(6, "%s: NOTIFY_IGNORE", __func__);
@@ -328,7 +325,7 @@ static void notify(const char *notice, unsigned int flags, const char *ntype,
 
 	if (flag_isset(flags, NOTIFY_SYSLOG)) {
 		upsdebugx(6, "%s: NOTIFY_SYSLOG (as LOG_NOTICE)", __func__);
-		upslogx(LOG_NOTICE, "%s", notice);
+		upslogx(LOG_NOTICE, "%s", NUT_STRARG(notice));
 	}
 
 #ifndef WIN32
@@ -357,20 +354,24 @@ static void notify(const char *notice, unsigned int flags, const char *ntype,
 
 	if (flag_isset(flags, NOTIFY_EXEC)) {
 		if (notifycmd != NULL) {
-			upsdebugx(6, "%s (%schild): NOTIFY_EXEC: calling NOTIFYCMD as '%s \"%s\"'",
-				__func__, use_pipe ? "grand" : "", notifycmd, notice);
+			char	*argv[3];
 
-			snprintf(exec, sizeof(exec), "%s \"%s\"", notifycmd, notice);
+			upsdebugx(6, "%s (%schild): NOTIFY_EXEC: calling NOTIFYCMD as '%s' with notice: '%s'",
+				__func__, use_pipe ? "grand" : "", notifycmd, NUT_STRARG(notice));
 
 			if (upsname)
 				setenv("UPSNAME", upsname, 1);
 			else
 				setenv("UPSNAME", "", 1);
 
 			setenv("NOTIFYTYPE", ntype, 1);
-			if (system(exec) == -1) {
-				upslog_with_errno(LOG_ERR, "%s", __func__);
-			}
+			argv[0] = (char *)notifycmd;
+			argv[1] = (char *)notice;
+			argv[2] = NULL;
+			execvp(notifycmd, argv);
+			/* execvp() only returns on error */
+			upslog_with_errno(LOG_ERR, "%s: execvp(%s) failed", __func__, notifycmd);
+			exit(EXIT_FAILURE);
 		} else {
 			upsdebugx(6, "%s (%schild): NOTIFY_EXEC: no NOTIFYCMD was configured", __func__, use_pipe ? "grand" : "");
 		}

clients/upssched.c

@@ -105,32 +105,71 @@ static OVERLAPPED connect_overlapped;
 
 static void exec_cmd(const char *cmd)
 {
-	int	err;
-	char	buf[LARGEBUF];
+#ifndef WIN32
+	int	waitstatus = 0;
+	pid_t	pid, waitret;
+#else
+	int	err = 0;
+#endif
+	char	*argv[3];
+
+	if (cmdscript == NULL) {
+		upslogx(LOG_ERR, "No CMDSCRIPT defined, cannot execute command: %s", NUT_STRARG(cmd));
+		return;
+	}
 
-	snprintf(buf, sizeof(buf), "%s %s", cmdscript, cmd);
+	argv[0] = cmdscript;
+	argv[1] = (char *)cmd;
+	argv[2] = NULL;
+
+	upsdebugx(4, "%s: calling: %s %s", __func__, cmdscript, NUT_STRARG(cmd));
 
-	upsdebugx(4, "%s: calling: %s", __func__, buf);
-	err = system(buf);
-	upsdebugx(3, "%s(%s): returned %d", __func__, buf, err);
 #ifndef WIN32
-	if (WIFEXITED(err)) {
-		if (WEXITSTATUS(err)) {
-			upslogx(LOG_INFO, "exec_cmd(%s) returned %d", buf, WEXITSTATUS(err));
+	pid = fork();
+	if (pid < 0) {
+		upslog_with_errno(LOG_ERR, "fork() failed in exec_cmd");
+		return;
+	}
+
+	if (pid == 0) {
+		/* child process */
+		execvp(cmdscript, argv);
+		/* execvp() only returns on error */
+		upslog_with_errno(LOG_ERR, "execvp(%s) failed", cmdscript);
+		exit(EXIT_FAILURE);
+	}
+
+	/* parent process - wait for child */
+	waitret = waitpid(pid, &waitstatus, 0);
+	if (waitret < 0) {
+		upslog_with_errno(LOG_ERR, "waitpid(%d) failed", (int)pid);
+		return;
+	}
+
+	if (WIFEXITED(waitstatus)) {
+		if (WEXITSTATUS(waitstatus)) {
+			upslogx(LOG_INFO, "exec_cmd(%s %s) returned %d",
+				cmdscript, NUT_STRARG(cmd), WEXITSTATUS(waitstatus));
 		}
 	} else {
-		if (WIFSIGNALED(err)) {
-			upslogx(LOG_WARNING, "exec_cmd(%s) terminated with signal %d", buf, WTERMSIG(err));
+		if (WIFSIGNALED(waitstatus)) {
+			upslogx(LOG_WARNING, "exec_cmd(%s %s) terminated with signal %d",
+				cmdscript, NUT_STRARG(cmd), WTERMSIG(waitstatus));
 		} else {
-			upslogx(LOG_ERR, "Execute command failure: %s", buf);
+			upslogx(LOG_ERR, "Execute command failure: %s %s",
+				cmdscript, NUT_STRARG(cmd));
 		}
 	}
+
+	upsdebugx(3, "%s: returned status %d", __func__, waitstatus);
 #else	/* WIN32 */
-	if(err != -1) {
-		upslogx(LOG_INFO, "Execute command \"%s\" OK", buf);
-	}
-	else {
-		upslogx(LOG_ERR, "Execute command failure : %s", buf);
+	/* Use _spawnvp for Windows */
+	err = _spawnvp(_P_WAIT, cmdscript, (const char * const *)argv);
+	if (err != -1) {
+		upslogx(LOG_INFO, "Execute command \"%s %s\" OK", cmdscript, NUT_STRARG(cmd));
+		upsdebugx(3, "%s: returned status %d", __func__, err);
+	} else {
+		upslog_with_errno(LOG_ERR, "Execute command \"%s %s\" failure", cmdscript, NUT_STRARG(cmd));
 	}
 #endif	/* WIN32 */
 

conf/upsmon.conf.sample.in

@@ -210,6 +210,9 @@ SHUTDOWNCMD "/sbin/shutdown -h +0"
 #
 # upsmon calls this to send messages when things happen
 #
+# The value is interpreted as a complete path to an externally stored
+# executable script (e.g. not an inline scriptlet) or program file without
+# further arguments (this is a change from NUT v2.8.5 and older releases).
 # This command is called with the full text of the message (from NOTIFYMSG)
 # as one argument.
 #

conf/upssched.conf.sample.in

@@ -21,6 +21,9 @@
 # CMDSCRIPT <scriptname>
 #
 # This script gets called to invoke commands for timers that trigger.
+# The value is interpreted as a complete path to an externally stored
+# executable script (e.g. not an inline scriptlet) or program file without
+# further arguments (this is a change from NUT v2.8.5 and older releases).
 # It is given a single argument - the <timername> in your
 # AT ... START-TIMER defines.
 #

docs/man/upsmon.conf.txt

@@ -230,10 +230,13 @@ NOTIFYFLAG.  See NOTIFYFLAG below for more details.
 Making this some sort of shell script might not be a bad idea.  For
 more information and ideas, see docs/scheduling.txt
 +
-Remember, this command also needs to be one element in the configuration file,
-so if your command has spaces, then wrap it in quotes.
+The value is interpreted as a complete path to an externally stored
+executable script (e.g. not an inline scriptlet) or program file without
+further arguments (this is a change from NUT v2.8.5 and older releases).
+Remember, this command also needs to be one element in the configuration
+file, so if your command path has spaces, then wrap it in quotes.
 +
-+NOTIFYCMD "/path/to/script --foo --bar"+
++NOTIFYCMD "/path to/script"+
 +
 This script is run in the background--that is, upsmon forks before it
 calls out to start it.  This means that your NOTIFYCMD may have multiple

docs/man/upsmon.txt

@@ -258,7 +258,12 @@ Example:
  - `NOTIFYCMD "/usr/local/bin/notifyme"`
 
 Remember to wrap the path in "double quotes" if it contains any spaces.
-It should probably not rely on receiving any command-line arguments.
+It should not rely on receiving any command-line arguments: the value of
+this directive is interpreted as a complete path to an externally stored
+executable script (e.g. not an inline scriptlet) or program file without
+further arguments (this is a change from NUT v2.8.5 and older releases).
+If you need to pass special arguments to a notification program, please
+write a small script to do so and specify its path here.
 
 The program you run as your NOTIFYCMD can use the environment variables
 NOTIFYTYPE and UPSNAME to know what has happened and on which UPS.  It

docs/man/upssched.conf.txt

@@ -46,6 +46,10 @@ Example:
 Required.  This must be above any AT lines.  This script is used to
 invoke commands when your timers are triggered.  It receives a single
 argument which is the name of the timer that caused it to trigger.
++
+The value is interpreted as a complete path to an externally stored
+executable script (e.g. not an inline scriptlet) or program file without
+further arguments (this is a change from NUT v2.8.5 and older releases).
 
 *PIPEFN* 'filename'::
 Required.  This sets the file name of the socket which will be used for

docs/man/upssched.txt

@@ -98,9 +98,16 @@ EARLY SHUTDOWNS
 ---------------
 
 To shut down the system early, define a timer that starts due to an ONBATT
-condition.  When it triggers, make your CMDSCRIPT call your shutdown
-routine.  It should finish by calling `upsmon -c fsd` so that upsmon gets
-to shut down the slaves in a controlled manner.
+condition.  When it triggers, make your `CMDSCRIPT` call your shutdown
+routine, especially if you have to prioritize something to stop first,
+or have subsystems that can take longer to gracefully stop than the normal
+OS shutdown patience allows (some systems might `kill` remaining processes
+after a hard-coded timeout).
+
+Your `CMDSCRIPT` should finish by calling `upsmon -c fsd`, so that your
+primary instance of linkman:upsmon[8] gets to shut down the secondary
+systems in a controlled manner (note the primary instance would also run
+its local definition of `SHUTDOWNCMD` in the end).
 
 Be sure you cancel the timer if power returns (ONLINE).