[Rpm-maint] [rpm-software-management/rpm] Stop blocking when GPG process dies prematurely (RhBug:1746353) (#938)

[Panu Matilainen]

pmatilai requested changes on this pull request.

Trivia aside, save-and-restore of the previous SIGCHLD handler is a must.

> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
     rpmPopMacro(NULL, "__plaintext_filename");
     rpmPopMacro(NULL, "__signature_filename");
 
+    /* The child GPG process may terminate without ever opening the pipe (such
+     * as when the key is expired), causing our parent process to get forever
+     * stuck on the Fopen() call below (see fifo(7) for details).  To fix that,
+     * we need to let the underlying open(2) unblock when SIGCHLD is received,
+     * by registering a no-op handler, so here we go: */
+    struct sigaction act;
+    memset(&act, '\0', sizeof(act));

memset() second argument is an int, not a character. Which seems kinda bizarre if you start thinking about bytes and all, but that's the way it is, and so the argument should be just plain zero, not the nul character. Not that it makes any difference functionality-wise.

> @@ -276,7 +282,26 @@ static int runGPG(sigTarget sigt, const char *sigfile)
     rpmPopMacro(NULL, "__plaintext_filename");
     rpmPopMacro(NULL, "__signature_filename");
 
+    /* The child GPG process may terminate without ever opening the pipe (such
+     * as when the key is expired), causing our parent process to get forever
+     * stuck on the Fopen() call below (see fifo(7) for details).  To fix that,
+     * we need to let the underlying open(2) unblock when SIGCHLD is received,
+     * by registering a no-op handler, so here we go: */
+    struct sigaction act;
+    memset(&act, '\0', sizeof(act));
+    act.sa_handler = &gpg_sa_handler;
+    act.sa_flags = 0;  /* Make sure we don't have SA_RESTART */
+    sigaction(SIGCHLD, &act, NULL);

As we're in a shared library here, we'll need to take care to save the old handler and restore it after we're done. Pay attention to error paths.

Specifically setting sa_flags to 0 seems a bit superfluous as we just memset() the struct to all zero two lines earlier. Not saying it's wrong, sometimes it's useful to signify something this way, whether that's called for here I dunno.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/938#pullrequestreview-318142487
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20191117/01cc71e7/attachment.html>