bug#28202: 26.0.50; Loading package.el should not start a subprocess

[Lars Ingebrigtsen]

Philipp <p.stephani2@gmail.com> writes:

> Loading package.el initializes the variable `package-check-signature',
> which starts a GnuPG subprocess.  This process might then be affected by
> https://bugs.launchpad.net/ubuntu/+source/gnupg/+bug/1285390, causing
> infinite hangs that can only be worked around by restarting the
> machine.  I think that in general loading packages should not start
> subprocesses to increase robustness.  Possible the initialization of
> `package-check-signature' should be delayed until signature checks are
> actually attempted.

Yes, definitely.  Packages should never execute anything when loaded --
and especially not something as complicated as gpg.

Does the following patch make sense?  It defaults the value to
allow-unsigned, which will then lead to the epg checking being run
(which will execute gpg).  The execution is cached in epg, though, so
it'll just be run once anyway.

This does mean though, that if you don't have gpg installed, the
`package-check-signature' value will still be `allow-signature', but
it'll act as if it's nil.  Currently, it would default to nil, and that
may be confusing.

Perhaps I could change the default to 'check-available or something and
then actually set the variable if it is that?  Opinions?

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 9a350aadac..c4309b700e 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -331,10 +331,7 @@ package-gnupghome-dir
   :risky t
   :version "26.1")
 
-(defcustom package-check-signature
-  (if (and (require 'epg-config)
-           (epg-find-configuration 'OpenPGP))
-      'allow-unsigned)
+(defcustom package-check-signature 'allow-unsigned
   "Non-nil means to check package signatures when installing.
 More specifically the value can be:
 - nil: package signatures are ignored.
@@ -353,6 +350,14 @@ package-check-signature
   :risky t
   :version "27.1")
 
+(defun package-check-signature ()
+  (if (eq package-check-signature 'allow-unsigned)
+      (progn
+        (require 'epg-config)
+        (and (epg-find-configuration 'OpenPGP)
+             'allow-unsigned))
+    package-check-signature))
+
 (defcustom package-unsigned-archives nil
   "List of archives where we do not check for package signatures."
   :type '(repeat (string :tag "Archive name"))
@@ -1279,15 +1284,15 @@ package--check-signature-content
       (dolist (sig (epg-context-result-for context 'verify))
         (if (eq (epg-signature-status sig) 'good)
             (push sig good-signatures)
-          ;; If package-check-signature is allow-unsigned, don't
+          ;; If `package-check-signature' is allow-unsigned, don't
           ;; signal error when we can't verify signature because of
           ;; missing public key.  Other errors are still treated as
           ;; fatal (bug#17625).
-          (unless (and (eq package-check-signature 'allow-unsigned)
+          (unless (and (eq (package-check-signature) 'allow-unsigned)
                        (eq (epg-signature-status sig) 'no-pubkey))
             (setq had-fatal-error t))))
       (when (or (null good-signatures)
-                (and (eq package-check-signature 'all)
+                (and (eq (package-check-signature) 'all)
                      had-fatal-error))
         (package--display-verify-error context sig-file)
         (signal 'bad-signature (list sig-file)))
@@ -1318,7 +1323,7 @@ package--check-signature
       :async async :noerror t
       ;; Connection error is assumed to mean "no sig-file".
       :error-form (let ((allow-unsigned
-                         (eq package-check-signature 'allow-unsigned)))
+                         (eq (package-check-signature) 'allow-unsigned)))
                     (when (and callback allow-unsigned)
                       (funcall callback nil))
                     (when unwind (funcall unwind))
@@ -1602,7 +1607,7 @@ package--download-one-archive
            (local-file (expand-file-name file dir)))
       (when (listp (read content))
         (make-directory dir t)
-        (if (or (not package-check-signature)
+        (if (or (not (package-check-signature))
                 (member name package-unsigned-archives))
             ;; If we don't care about the signature, save the file and
             ;; we're done.
@@ -1654,7 +1659,7 @@ package-refresh-contents
   (let ((default-keyring (expand-file-name "package-keyring.gpg"
                                            data-directory))
         (inhibit-message (or inhibit-message async)))
-    (when (and package-check-signature (file-exists-p default-keyring))
+    (when (and (package-check-signature) (file-exists-p default-keyring))
       (condition-case-unless-debug error
           (package-import-keyring default-keyring)
         (error (message "Cannot import default keyring: %S" (cdr error))))))
@@ -1901,7 +1906,7 @@ package-install-from-archive
          (file (concat (package-desc-full-name pkg-desc)
                        (package-desc-suffix pkg-desc))))
     (package--with-response-buffer location :file file
-      (if (or (not package-check-signature)
+      (if (or (not (package-check-signature))
               (member (package-desc-archive pkg-desc)
                       package-unsigned-archives))
           ;; If we don't care about the signature, unpack and we're

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no