GPG: Switch back to pipe(7) for signing

When it comes to IPC with the GPG subprocess that we spawn to sign
packages for us, there has been an evolution of changes over the years.

Initially, we used temporary files to dump package data (header+payload)
to disk, and pointed GPG to those.  That being an insanely expensive
operation, we later opted for a pipe bound to stdin on the reader side
[1] to avoid the unnecessary I/O.  When GPG 2.1 came along, our own
passphrase entry logic stopped working so we decided to offload [3] that
to GPG instead, for which we also needed to swap the pipe for a named
pipe (FIFO), to free [2] stdin for GPG's own passphrase entry program
(pinentry).

Fast forward to the present, it has become apparent that the FIFO
semantics is not the right choice for us either, as it brings about a
handful of synchronization issues, all of which stem from the fact that
the GPG subprocess, which we cannot control, may or may not open the
input file (our FIFO) for reading before it terminates, causing either
process to hang.  That's because an open(2) call on a FIFO blocks until
the other end is also opened.  One particular case where such a deadlock
can happen is an expired key [4]; GPG would error out without ever
opening the input file (and rightly so), making RPM stuck.

In a nutshell, here's what the two processes do:

RPM parent:
    * spawn GPG child
    * open FIFO for writing (blocks)
    * write data
    * close FIFO

GPG child:
    * check key validity    <-- may terminate here
    * open FIFO for reading (blocks)
    * read data
    * close FIFO

Which may result in the following execution order:

* parent:   spawn GPG child
* parent:   open FIFO for writing (blocks)
* child:    check key validity (key expired!)
* child:    terminate with an error (sends SIGCHLD)
* parent:   (continues blocking...)

Now, one way to unblock the parent would be to install a SIGCHLD handler
with SA_RESTART and make it open the FIFO for reading (with O_NONBLOCK)
iff the signal comes from our GPG child (note that RPM can also be used
as a library) so that the restarted open call unblocks right away.  This
would, however, still not be entirely safe from race conditions; if
there were other subprocesses, all terminating about the same time,
their SIGCHLD instances could prevent us from receiving the one from
GPG; this is because standard signals do not queue (see signal(7) for a
detailed explanation).

Another way would be to spawn GPG from a wrapper process whose sole
purpose would be to wait(2) for GPG and to open the FIFO if it failed
(without O_NONBLOCK, to ensure the wrapper doesn't exit before the
parent's open call).  This would work as long as we could tell exactly
which errors cause GPG to not open the input file, but since that's not
documented, we would have to rely on implementation details, not to
mention the added complexity and ugliness of such a solution, so that's
also a no-go.

Lastly, why not just use O_NONBLOCK in the parent, to prevent it from
blocking in the first place?  Since GPG does not use O_NONBLOCK, this
would just push the problem down to the child; if the parent is too fast
and manages to close the FIFO before the child gets a chance to open it,
the child would get stuck once it gets there (thus making the parent
stuck on the wait call).

Luckily, it turns out we actually can take over stdin *and* have GPG use
its pinentry program at the same time!  All we need is to make sure the
GPG_TTY environment variable is set to the current terminal (see
gpg-agent(1)).  This variable is used [5] by GPG as a fall-back when it
fails to obtain the terminal connected to stdin using ttyname(3), such
as when it is redirected to a pipe.

Thus, in this commit (mostly adapted from the reverse of [2]), we revert
to using the good old pipe(7) which not only avoids race conditions (as
it does not cause open to block), but is also more suitable for this
task in general.  (For the record, GPGme also uses [6] a regular pipe
for the IPC.)  Since we have access to the original stdin before
redirection, we set GPG_TTY accordingly (if previously unset), to keep
pinentry working.

This commit also resolves the RHBZ linked in [4].  Note that, in the
case of key expiration, GPG isn't particularly specific in the error
message or exit code; all it says is "Unusable secret key" and returns
code 2.  For that reason, we can't print anything helpful to stderr
either.

[1] commit 1aace27fb9541c1d176d8795a1d7872949f8f551
[2] commit 6a8924b4c9df8e3597f7b4aa3de46498d390c5a8
[3] commit 0bce5fcf270711a2e077fba0fb7c5979ea007eb5
[4] https://bugzilla.redhat.com/show_bug.cgi?id=1746353
[5] https://github.com/gpg/gnupg/blob/f3df8dbb696fed192501fa7f741c2e0e0936a3d5/agent/gpg-agent.c#L1115
[6] https://github.com/gpg/gpgme/blob/52f930c1ed7eee6336a41598c90ef3605b7ed02b/src/engine-gpg.c#L1133

1 files changed, 30 insertions, 82 deletions

diff --git a/sign/rpmgensig.c b/sign/rpmgensig.c
index 899aeb73d..864038008 100644
--- a/sign/rpmgensig.c
+++ b/sign/rpmgensig.c
@@ -8,7 +8,6 @@
 #include <errno.h>
 #include <sys/wait.h>
 #include <popt.h>
-#include <libgen.h>
 #include <fcntl.h>
 
 #include <rpm/rpmlib.h>			/* RPMSIGTAG & related */
@@ -34,68 +33,6 @@ typedef struct sigTarget_s {
     rpm_loff_t size;
 } *sigTarget;
 
-/*
- * There is no function for creating unique temporary fifos so create
- * unique temporary directory and then create fifo in it.
- */
-static char *mkTempFifo(void)
-{
-    char *tmppath = NULL, *tmpdir = NULL, *fifofn = NULL;
-    mode_t mode;
-
-    tmppath = rpmExpand("%{_tmppath}", NULL);
-    if (rpmioMkpath(tmppath, 0755, (uid_t) -1, (gid_t) -1))
-	goto exit;
-
-
-    tmpdir = rpmGetPath(tmppath, "/rpm-tmp.XXXXXX", NULL);
-    mode = umask(0077);
-    tmpdir = mkdtemp(tmpdir);
-    umask(mode);
-    if (tmpdir == NULL) {
-	rpmlog(RPMLOG_ERR, _("error creating temp directory %s: %m\n"),
-	    tmpdir);
-	tmpdir = _free(tmpdir);
-	goto exit;
-    }
-
-    fifofn = rpmGetPath(tmpdir, "/fifo", NULL);
-    if (mkfifo(fifofn, 0600) == -1) {
-	rpmlog(RPMLOG_ERR, _("error creating fifo %s: %m\n"), fifofn);
-	fifofn = _free(fifofn);
-    }
-
-exit:
-    if (fifofn == NULL && tmpdir != NULL)
-	unlink(tmpdir);
-
-    free(tmppath);
-    free(tmpdir);
-
-    return fifofn;
-}
-
-/* Delete fifo and then temporary directory in which it was located */
-static int rpmRmTempFifo(const char *fn)
-{
-    int rc = 0;
-    char *dfn = NULL, *dir = NULL;
-
-    if ((rc = unlink(fn)) != 0) {
-	rpmlog(RPMLOG_ERR, _("error delete fifo %s: %m\n"), fn);
-	return rc;
-    }
-
-    dfn = xstrdup(fn);
-    dir = dirname(dfn);
-
-    if ((rc = rmdir(dir)) != 0)
-	rpmlog(RPMLOG_ERR, _("error delete directory %s: %m\n"), dir);
-    free(dfn);
-
-    return rc;
-}
-
 static int closeFile(FD_t *fdp)
 {
     if (fdp == NULL || *fdp == NULL)
@@ -242,27 +179,38 @@ exit:
 static int runGPG(sigTarget sigt, const char *sigfile)
 {
     int pid = 0, status;
-    FD_t fnamedPipe = NULL;
-    char *namedPipeName = NULL;
+    int pipefd[2];
+    FILE *fpipe = NULL;
     unsigned char buf[BUFSIZ];
     ssize_t count;
     ssize_t wantCount;
     rpm_loff_t size;
     int rc = 1; /* assume failure */
 
-    namedPipeName = mkTempFifo();
+    if (pipe(pipefd) < 0) {
+        rpmlog(RPMLOG_ERR, _("Could not create pipe for signing: %m\n"));
+        goto exit;
+    }
 
-    rpmPushMacro(NULL, "__plaintext_filename", NULL, namedPipeName, -1);
+    rpmPushMacro(NULL, "__plaintext_filename", NULL, "-", -1);
     rpmPushMacro(NULL, "__signature_filename", NULL, sigfile, -1);
 
     if (!(pid = fork())) {
 	char *const *av;
 	char *cmd = NULL;
-	const char *gpg_path = rpmExpand("%{?_gpg_path}", NULL);
+	const char *tty = ttyname(STDIN_FILENO);
+	const char *gpg_path = NULL;
 
+	if (!getenv("GPG_TTY") && (!tty || setenv("GPG_TTY", tty, 0)))
+	    rpmlog(RPMLOG_WARNING, _("Could not set GPG_TTY to stdin: %m\n"));
+
+	gpg_path = rpmExpand("%{?_gpg_path}", NULL);
 	if (gpg_path && *gpg_path != '\0')
 	    (void) setenv("GNUPGHOME", gpg_path, 1);
 
+	dup2(pipefd[0], STDIN_FILENO);
+	close(pipefd[1]);
+
 	unsetenv("MALLOC_CHECK_");
 	cmd = rpmExpand("%{?__gpg_sign_cmd}", NULL);
 	rc = poptParseArgvString(cmd, NULL, (const char ***)&av);
@@ -277,9 +225,10 @@ static int runGPG(sigTarget sigt, const char *sigfile)
     rpmPopMacro(NULL, "__plaintext_filename");
     rpmPopMacro(NULL, "__signature_filename");
 
-    fnamedPipe = Fopen(namedPipeName, "w");
-    if (!fnamedPipe) {
-	rpmlog(RPMLOG_ERR, _("Fopen failed\n"));
+    close(pipefd[0]);
+    fpipe = fdopen(pipefd[1], "w");
+    if (!fpipe) {
+	rpmlog(RPMLOG_ERR, _("Could not open pipe for writing: %m\n"));
 	goto exit;
     }
 
@@ -292,8 +241,8 @@ static int runGPG(sigTarget sigt, const char *sigfile)
     size = sigt->size;
     wantCount = size < sizeof(buf) ? size : sizeof(buf);
     while ((count = Fread(buf, sizeof(buf[0]), wantCount, sigt->fd)) > 0) {
-	Fwrite(buf, sizeof(buf[0]), count, fnamedPipe);
-	if (Ferror(fnamedPipe)) {
+	fwrite(buf, sizeof(buf[0]), count, fpipe);
+	if (ferror(fpipe)) {
 	    rpmlog(RPMLOG_ERR, _("Could not write to pipe\n"));
 	    goto exit;
 	}
@@ -305,8 +254,10 @@ static int runGPG(sigTarget sigt, const char *sigfile)
 		sigt->fileName, Fstrerror(sigt->fd));
 	goto exit;
     }
-    Fclose(fnamedPipe);
-    fnamedPipe = NULL;
+    fclose(fpipe);
+    fpipe = NULL;
+    close(pipefd[1]);
+    pipefd[1] = NULL;
 
     (void) waitpid(pid, &status, 0);
     pid = 0;
@@ -318,17 +269,14 @@ static int runGPG(sigTarget sigt, const char *sigfile)
 
 exit:
 
-    if (fnamedPipe)
-	Fclose(fnamedPipe);
+    if (fpipe)
+	fclose(fpipe);
+    if (pipefd[1])
+	close(pipefd[1]);
 
     if (pid)
 	waitpid(pid, &status, 0);
 
-    if (namedPipeName) {
-	rpmRmTempFifo(namedPipeName);
-	free(namedPipeName);
-    }
-
     return rc;
 }