Rewrite of "--remote", to use a temporary file for information exchange instead of a SysV IPC message queue, to correct the fault where "make check -j32" would fail due to every PV exit removing the message queue.

docs/NEWS.md

@@ -1,5 +1,6 @@
 ### 1.8.x-UNRELEASED
 
+ * fix: replaced "`--remote`" mechanism, using a temporary file instead of SysV IPC, so it can work reliably even when there are multiple PV instances
  * fix: corrected compilation failure when without IPC support
  * cleanup: compilation warnings fixed on non-IPC and MacOS systems
 

docs/pv.1

@@ -689,13 +689,11 @@ before using
 .IP \(bu 3
 The
 .B -R
-option is not available on Cygwin without
-.B cygserver
-running (SYSV IPC is needed). To fix this, start
-.B cygserver
-before running the instance of
-.B pv
-you want, at runtime, to change the parameters of.
+option requires that either
+.B /run/user/<uid>/
+or
+.B /tmp/
+can be written to, for inter-process communication.
 .P
 If you find any other problems, please report them.
 

docs/pv.1.md

@@ -517,10 +517,8 @@ The following problems are known to exist in **pv**:
     is needed to handle the terminal scrolling). To fix this, start
     **cygserver** before using **pv -c**.
 
--   The **-R** option is not available on Cygwin without **cygserver**
-    running (SYSV IPC is needed). To fix this, start **cygserver**
-    before running the instance of **pv** you want, at runtime, to
-    change the parameters of.
+-   The **-R** option requires that either **/run/user/\<uid\>/** or
+    **/tmp/** can be written to, for inter-process communication.
 
 If you find any other problems, please report them.
 

src/include/pv-internal.h

@@ -142,6 +142,10 @@ struct pvstate_s {
 	volatile sig_atomic_t pv_sig_newsize;	 /* whether we need to get term size again */
 	volatile sig_atomic_t pv_sig_abort;	 /* whether we need to abort right now */
 	volatile sig_atomic_t reparse_display;	 /* whether to re-check format string */
+#ifdef SA_SIGINFO
+	volatile sig_atomic_t pv_sig_rxusr2;	 /* whether SIGUSR2 was received */
+	volatile pid_t pv_sig_sender;		 /* PID of sending process for SIGUSR2 */
+#endif
 	struct sigaction pv_sig_old_sigpipe;
 	struct sigaction pv_sig_old_sigttou;
 	struct sigaction pv_sig_old_sigtstp;
@@ -150,6 +154,9 @@ struct pvstate_s {
 	struct sigaction pv_sig_old_sigint;
 	struct sigaction pv_sig_old_sighup;
 	struct sigaction pv_sig_old_sigterm;
+#ifdef SA_SIGINFO
+	struct sigaction pv_sig_old_sigusr2;
+#endif
 
 	/*****************
 	 * Display state *

src/include/pv.h

@@ -15,6 +15,7 @@
 
 #include <stdlib.h>
 #include <stdbool.h>
+#include <signal.h>
 #include <time.h>
 #include <sys/types.h>
 
@@ -200,6 +201,13 @@ extern off_t pv_calc_total_size(pvstate_t);
  */
 extern void pv_sig_init(pvstate_t);
 
+#ifdef SA_SIGINFO
+/*
+ * Return true if SIGUSR2 has been received, and indicate the sender.
+ */
+extern bool pv_sigusr2_received(pvstate_t, pid_t *);
+#endif
+
 /*
  * Enter the main transfer loop, transferring all input files to the output.
  */

src/main/help.c

@@ -9,6 +9,7 @@
 #include "config.h"
 #include "pv.h"
 #include <stdio.h>
+#include <signal.h>
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
@@ -383,11 +384,11 @@ void display_help(void)
 		{ "-X", "--discard", NULL,
 		 N_("discard input instead of writing to output"),
 		 { 0, 0, 0, 0} },
-#ifdef HAVE_IPC
+#ifdef SA_SIGINFO
 		{ "-R", "--remote", N_("PID"),
 		 N_("update settings of process PID"),
 		 { 0, 0, 0, 0} },
-#endif				/* HAVE_IPC */
+#endif				/* SA_SIGINFO */
 		{ "", NULL, NULL, NULL, { 0, 0, 0, 0} },
 		{ "-P", "--pidfile", N_("FILE"),
 		 N_("save process ID in FILE"),

src/main/main.c

@@ -24,7 +24,7 @@
 #include <sys/stat.h>
 
 
-int pv_remote_set(opts_t);
+int pv_remote_set(opts_t, pvstate_t);
 void pv_remote_init(void);
 void pv_remote_fini(void);
 
@@ -60,15 +60,6 @@ int main(int argc, char **argv)
 		return 0;
 	}
 
-	/*
-	 * -R specified - send the message, then exit.
-	 */
-	if (opts->remote > 0) {
-		retcode = pv_remote_set(opts);
-		opts_free(opts);
-		return retcode;
-	}
-
 	/*
 	 * Allocate our internal state buffer.
 	 */
@@ -87,6 +78,23 @@ int main(int argc, char **argv)
 		/*@+mustfreefresh@ */
 	}
 
+	/*
+	 * -R specified - send the message, then exit.
+	 */
+	if (opts->remote > 0) {
+		/* Initialise signal handling. */
+		pv_sig_init(state);
+		/* Send the message. */
+		retcode = pv_remote_set(opts, state);
+		/* Close down the signal handling. */
+		pv_sig_fini(state);
+		/* Free resources. */
+		pv_state_free(state);
+		opts_free(opts);
+		/* Early exit. */
+		return retcode;
+	}
+
 	/*
 	 * Write a PID file if -P was specified.
 	 */

src/main/remote.c

@@ -15,18 +15,14 @@
 #include <errno.h>
 #include <signal.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include <sys/types.h>
 #include <sys/time.h>
 
-#ifdef HAVE_IPC
-#include <sys/ipc.h>
-#include <sys/msg.h>
-#endif				/* HAVE_IPC */
+#ifdef SA_SIGINFO
+void pv_error(pvstate_t, char *, ...);
 
-
-#ifdef HAVE_IPC
 struct remote_msg {
-	long mtype;
 	bool progress;			 /* progress bar flag */
 	bool timer;			 /* timer flag */
 	bool eta;			 /* ETA flag */
@@ -53,67 +49,77 @@ struct remote_msg {
  * bounded to one less than their size so they are always \0 terminated.
  */
 
-static int remote__msgid = -1;
-
 
 /*
- * Return a key for use with msgget() which will be unique to the current
- * user.
- *
- * We can't just use ftok() because the queue needs to be user-specific
- * so that a user cannot send messages to another user's process, and we
- * can't easily find out the terminal a given process is connected to in a
- * cross-platform way.
+ * Return a stream pointer, and populate the filename buffer, for a control
+ * file associated with a particular process ID; it will be opened for
+ * writing if "sender" is true.  Returns NULL on error.
  */
-static key_t remote__genkey(void)
+static FILE *pv__control_file(char *filename, size_t bufsize, pid_t control_pid, bool sender)
 {
-	uid_t uid;
-	key_t key;
+	int open_flags, open_mode, control_fd;
+	FILE *control_fptr;
 
-	/*@-type@ *//* splint doesn't like uid_t */
-	uid = geteuid();
-	/*@+type@ */
+	open_flags = O_RDONLY;
+#ifdef O_NOFOLLOW
+	open_flags += O_NOFOLLOW;
+#endif
+	if (sender)
+		open_flags = O_WRONLY | O_CREAT | O_EXCL;
 
-	key = ftok("/tmp", (int) 'P') | uid;
+	open_mode = 0644;
 
-	return key;
+	(void) pv_snprintf(filename, bufsize, "/run/user/%lu/pv.remote.%lu", (unsigned long) geteuid(),
+			   (unsigned long) control_pid);
+	control_fd = open(filename, open_flags, open_mode);	/* flawfinder: ignore */
+
+	if (control_fd < 0) {
+		(void) pv_snprintf(filename, bufsize, "/tmp/pv.remote.%lu", (unsigned long) control_pid);
+		control_fd = open(filename, open_flags, open_mode);	/* flawfinder: ignore */
+	}
+
+	/*
+	 * flawfinder rationale: the files are in a directory whose parents
+	 * cannot be manipulated, and we are not allowing the final
+	 * component to be a symbolic link.
+	 */
+
+	if (control_fd < 0)
+		return NULL;
+
+	control_fptr = fdopen(control_fd, sender ? "wb" : "rb");
+
+	return control_fptr;
 }
 
 
 /*
- * Return a message queue ID that is unique to the current user and the
- * given process ID, or -1 on error.
- */
-static int remote__msgget(void)
-{
-	/* Catch SIGSYS in case msgget() raises it, so we get ENOSYS */
-	/*@-unrecog@ *//* splint doesn't see SIGSYS */
-	(void) signal(SIGSYS, SIG_IGN);
-	/*@+unrecog@ */
-	return msgget(remote__genkey(), IPC_CREAT | 0600);
-}
-
-
-/*
- * Set the options of a remote process by setting up an IPC message queue,
- * sending a message containing the new options, and then waiting for the
- * message to be consumed by the remote process.
+ * Set the options of a remote process by writing them to a file, sending a
+ * signal to the receiving process, and waiting for the message to be
+ * consumed by the remote process.
  *
  * Returns nonzero on error.
  */
-int pv_remote_set(opts_t opts)
+int pv_remote_set(opts_t opts, pvstate_t state)
 {
+	char control_filename[4096];	/* flawfinder: ignore */
+	FILE *control_fptr;
 	struct remote_msg msgbuf;
-	struct msqid_ds qbuf;
+	pid_t signal_sender;
 	long timeout;
-	int msgid;
-	unsigned long initial_qnum;
+	bool received;
+
+	/*
+	 * flawfinder rationale: buffer is large enough, explicitly zeroed,
+	 * and always bounded properly as we are only writing to it with
+	 * pv_snprintf().
+	 */
 
 	/*
 	 * Check that the remote process exists.
 	 */
 	if (kill((pid_t) (opts->remote), 0) != 0) {
-		fprintf(stderr, "%s: %u: %s\n", opts->program_name, opts->remote, strerror(errno));
+		pv_error(state, "%u: %s", opts->remote, strerror(errno));
 		return 1;
 	}
 
@@ -137,7 +143,6 @@ int pv_remote_set(opts_t opts)
 	 * Copy parameters into message buffer.
 	 */
 	memset(&msgbuf, 0, sizeof(msgbuf));
-	msgbuf.mtype = (long) (opts->remote);
 	msgbuf.progress = opts->progress;
 	msgbuf.timer = opts->timer;
 	msgbuf.eta = opts->eta;
@@ -169,30 +174,56 @@ int pv_remote_set(opts_t opts)
 	 * terminated by memset() earlier.
 	 */
 
-	msgid = remote__msgget();
-	if (msgid < 0) {
-		fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno));
+	/*
+	 * Get the filename and file stream to use for remote control.
+	 */
+	memset(control_filename, 0, sizeof(control_filename));
+	control_fptr = pv__control_file(control_filename, sizeof(control_filename), getpid(), true);
+	if (NULL == control_fptr) {
+		pv_error(state, "%s", strerror(errno));
 		return 1;
 	}
 
-	memset(&qbuf, 0, sizeof(qbuf));
-	if (msgctl(msgid, IPC_STAT, &qbuf) < 0) {
-		fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno));
+	/*
+	 * Write the message buffer to the remote control file, and close
+	 * it.
+	 */
+	if (1 != fwrite(&msgbuf, sizeof(msgbuf), 1, control_fptr)) {
+		pv_error(state, "%s", strerror(errno));
+		(void) fclose(control_fptr);
+		(void) remove(control_filename);
 		return 1;
 	}
 
-	initial_qnum = qbuf.msg_qnum;
-
-	if (msgsnd(msgid, &msgbuf, sizeof(msgbuf) - sizeof(long), 0) != 0) {
-		fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno));
+	if (0 != fclose(control_fptr)) {
+		pv_error(state, "%s", strerror(errno));
+		(void) remove(control_filename);
 		return 1;
 	}
 
+	/*
+	 * Send a SIGUSR2 signal to the remote process, to tell it a message
+	 * is ready to read, after clearing our own "SIGUSR2 received" flag.
+	 */
+	signal_sender = 0;
+	(void) pv_sigusr2_received(state, &signal_sender);
+	if (kill((pid_t) (opts->remote), SIGUSR2) != 0) {
+		pv_error(state, "%u: %s", opts->remote, strerror(errno));
+		(void) remove(control_filename);
+		return 1;
+	}
+	
 	debug("%s", "message sent");
 
-	timeout = 1100000;
+	/*
+	 * Wait for a signal from the remote process to say it has received
+	 * the message.
+	 */
 
-	while (timeout > 10000) {
+	timeout = 1100000;
+	received = false;
+
+	while (timeout > 10000 && !received) {
 		struct timeval tv;
 
 		memset(&tv, 0, sizeof(tv));
@@ -203,84 +234,92 @@ int pv_remote_set(opts_t opts)
 		/*@+nullpass@ */
 		timeout -= 10000;
 
-		/*
-		 * If we can't stat the queue, it must have been deleted.
-		 */
-		memset(&qbuf, 0, sizeof(qbuf));
-		if (msgctl(msgid, IPC_STAT, &qbuf) < 0) {
-			debug("%s(%d) pid %ld: %s", "msgctl", (int) msgid, (long) opts->remote, strerror(errno));
-			break;
-		}
-
-		/*
-		 * If the message count is at or below the message count
-		 * before we sent our message, assume it was received.
-		 */
-		if (qbuf.msg_qnum <= initial_qnum) {
-			debug("%s: %lu <= %lu", "message received", (unsigned long)(qbuf.msg_qnum), initial_qnum);
-			return 0;
+		if (pv_sigusr2_received(state, &signal_sender)) {
+			if (signal_sender == opts->remote) {
+				debug("%s", "message received");
+				received = true;
+			}
 		}
 	}
 
 	/*
-	 * Message not received - delete it.
+	 * Remove the remote control file.
 	 */
-	memset(&qbuf, 0, sizeof(qbuf));
-	if (msgctl(msgid, IPC_STAT, &qbuf) >= 0) {
-		(void) msgrcv(msgid, &msgbuf, sizeof(msgbuf) - sizeof(long), (long) (opts->remote), IPC_NOWAIT);
-		/*
-		 * If this leaves nothing on the queue, remove the
-		 * queue, in case we created one for no reason.
-		 */
-		if (msgctl(msgid, IPC_STAT, &qbuf) >= 0) {
-			if (qbuf.msg_qnum < 1)
-				(void) msgctl(msgid, IPC_RMID, &qbuf);
-		}
+	if (0 != remove(control_filename)) {
+		pv_error(state, "%s", strerror(errno));
 	}
 
+	/*
+	 * Return 0 if the message was received.
+	 */
+	if (received)
+		return 0;
+
 	/*@-mustfreefresh@ */
 	/*
 	 * splint note: the gettext calls made by _() cause memory leak
 	 * warnings, but in this case it's unavoidable, and mitigated by the
 	 * fact we only translate each string once.
 	 */
-	fprintf(stderr, "%s: %u: %s\n", opts->program_name, opts->remote, _("message not received"));
+	pv_error(state, "%u: %s", opts->remote, _("message not received"));
 	return 1;
 	/*@+mustfreefresh @ */
 }
 
 
 /*
- * Check for an IPC remote handling message and, if there is one, replace
- * the current process's options with those being passed in.
+ * Check for a remote control message and, if there is one, replace the
+ * current process's options with those being passed in.
  *
  * NB relies on pv_state_set_format() causing the output format to be
  * reparsed.
  */
 void pv_remote_check(pvstate_t state)
 {
+	pid_t signal_sender;
+	char control_filename[4096];	/* flawfinder: ignore */
+	FILE *control_fptr;
 	struct remote_msg msgbuf;
-	ssize_t got;
 
-	if (remote__msgid < 0)
+	/* flawfinder rationale: as above. */
+
+	/*
+	 * Return early if a SIGUSR2 signal has not been received.
+	 */
+	signal_sender = 0;
+	if (!pv_sigusr2_received(state, &signal_sender))
 		return;
 
-	memset(&msgbuf, 0, sizeof(msgbuf));
-
-	got = msgrcv(remote__msgid, &msgbuf, sizeof(msgbuf) - sizeof(long), getpid(), IPC_NOWAIT);
-	if (got < 0) {
-		/*
-		 * If our queue had been deleted, re-create it.
-		 */
-		/*@-unrecog@ *//* splint doesn't see ENOMSG */
-		if (errno != EAGAIN && errno != ENOMSG) {
-			remote__msgid = remote__msgget();
-		}
-		/*@+unrecog@ */
+	memset(control_filename, 0, sizeof(control_filename));
+	control_fptr = pv__control_file(control_filename, sizeof(control_filename), signal_sender, false);
+	if (NULL == control_fptr) {
+		pv_error(state, "%s", strerror(errno));
+		return;
 	}
-	if (got < 1)
-		return;
 
+	/*
+	 * Read the message buffer from the remote control file, and close
+	 * it.
+	 */
+	if (1 != fread(&msgbuf, sizeof(msgbuf), 1, control_fptr)) {
+		pv_error(state, "%s", strerror(errno));
+		(void) fclose(control_fptr);
+		return;
+	}
+
+	if (0 != fclose(control_fptr)) {
+		pv_error(state, "%s", strerror(errno));
+		return;
+	}
+
+	/*
+	 * Send a SIGUSR2 signal to the sending process, to tell it the
+	 * message has been received.
+	 */
+	if (kill(signal_sender, SIGUSR2) != 0) {
+		debug("%u: %s", signal_sender, strerror(errno));
+	}
+	
 	debug("%s", "received remote message");
 
 	pv_state_format_string_set(state, NULL);
@@ -318,7 +357,6 @@ void pv_remote_check(pvstate_t state)
  */
 void pv_remote_init(void)
 {
-	remote__msgid = remote__msgget();
 }
 
 
@@ -327,23 +365,18 @@ void pv_remote_init(void)
  */
 void pv_remote_fini(void)
 {
-	if (remote__msgid >= 0) {
-		struct msqid_ds qbuf;
-		memset(&qbuf, 0, sizeof(qbuf));
-		(void) msgctl(remote__msgid, IPC_RMID, &qbuf);
-	}
 }
 
-#else				/* !HAVE_IPC */
+#else				/* !SA_SIGINFO */
 
 /*
- * Dummy stubs for remote control when we don't have IPC.
+ * Dummy stubs for remote control when we don't have SA_SIGINFO.
  */
 void pv_remote_init(void)
 {
 }
 
-void pv_remote_check(pvstate_t state)
+void pv_remote_check(/*@unused@*/ __attribute__((unused)) pvstate_t state)
 {
 }
 
@@ -351,12 +384,14 @@ void pv_remote_fini(void)
 {
 }
 
-int pv_remote_set(pvstate_t state)
+int pv_remote_set(/*@unused@*/ __attribute__((unused)) opts_t opts, /*@unused@*/ __attribute__((unused)) pvstate_t state)
 {
-	fprintf(stderr, "%s\n", _("IPC not supported on this system"));
+	/*@-mustfreefresh@ */ /* splint - see above */
+	fprintf(stderr, "%s\n", _("SA_SIGINFO not supported on this system"));
+	/*@+mustfreefresh@ */
 	return 1;
 }
 
-#endif				/* HAVE_IPC */
+#endif				/* SA_SIGINFO */
 
 /* EOF */

src/pv/signal.c

@@ -206,6 +206,43 @@ static void pv_sig_term( /*@unused@ */  __attribute__((unused))
 }
 
 
+#ifdef SA_SIGINFO
+/*
+ * Handle a SIGUSR2 by setting a flag to say we received it, after recording
+ * the sending PID.
+ */
+static void pv_sig_usr2( /*@unused@ */  __attribute__((unused))
+			int sig, siginfo_t * info, /*@unused@ */  __attribute__((unused))
+			void *ucontext)
+{
+	if (NULL == pv_sig_state)
+		return;
+	if (NULL == info)
+		return;
+	pv_sig_state->pv_sig_rxusr2 = 1;
+	pv_sig_state->pv_sig_sender = info->si_pid;
+}
+
+
+/*
+ * Return true if a SIGUSR2 signal has been received since the last time
+ * this function was called, populating *pid with the sending PID if so.
+ */
+bool pv_sigusr2_received(pvstate_t state, pid_t *pid)
+{
+	if (NULL == state)
+		return false;
+	if (0 == state->pv_sig_rxusr2)
+		return false;
+	if (NULL != pid)
+		*pid = state->pv_sig_sender;
+	state->pv_sig_rxusr2 = 0;
+	return true;
+}
+
+#endif
+
+
 /*
  * Initialise signal handling.
  */
@@ -301,6 +338,21 @@ void pv_sig_init(pvstate_t state)
 	sa.sa_flags = 0;
 	(void) sigaction(SIGTERM, &sa, &(pv_sig_state->pv_sig_old_sigterm));
 
+#ifdef SA_SIGINFO
+	/*
+	 * Handle SIGUSR2 by setting a flag to say the signal has been
+	 * received, and storing the sending process's PID.
+	 */
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_sigaction = pv_sig_usr2;
+	(void) sigemptyset(&(sa.sa_mask));
+	/*@-unrecog@ *//* splint doesn't know about SA_SIGINFO */
+	sa.sa_flags = SA_SIGINFO;
+	/*@+unrecog@ */
+	(void) sigaction(SIGUSR2, &sa, &(pv_sig_state->pv_sig_old_sigusr2));
+	memset(&sa, 0, sizeof(sa));
+#endif
+
 	/*
 	 * Ensure that the TOSTOP terminal attribute is set, so that a
 	 * SIGTTOU signal will be raised if we try to write to the terminal
@@ -333,6 +385,9 @@ void pv_sig_fini( /*@unused@ */  __attribute__((unused)) pvstate_t state)
 	(void) sigaction(SIGINT, &(pv_sig_state->pv_sig_old_sigint), NULL);
 	(void) sigaction(SIGHUP, &(pv_sig_state->pv_sig_old_sighup), NULL);
 	(void) sigaction(SIGTERM, &(pv_sig_state->pv_sig_old_sigterm), NULL);
+#ifdef SA_SIGINFO
+	(void) sigaction(SIGUSR2, &(pv_sig_state->pv_sig_old_sigusr2), NULL);
+#endif
 
 	need_to_clear_tostop = pv_sig_state->pv_tty_tostop_added;