bug#35739: Bad signature from GNU ELPA

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 4575 bytes --]

Recipe from 'emacs -Q': [M-x package-list-packages RET].

Symptoms: The package list is displayed but an *Error* buffer pops up:

Failed to verify signature archive-contents.sig:
Bad signature from 474F05837FBDEF9B GNU ELPA Signing Agent (2014) <
elpasign@elpa.gnu.org>
Command output:
gpg: Signature made 05/14/19 22:10:03 GMT Summer Time
gpg:                using DSA key CA442C00F91774F17F59D9B0474F05837FBDEF9B
gpg: BAD signature from "GNU ELPA Signing Agent (2014) <
elpasign@elpa.gnu.org>" [unknown]

GPG version:

gpg (GnuPG) 2.2.11
libgcrypt 1.8.4
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <
https://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Home: C:/Users/buster/AppData/Roaming/gnupg
Supported algorithms:
Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
        CAMELLIA128, CAMELLIA192, CAMELLIA256
Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
Compression: Uncompressed, ZIP, ZLIB, BZIP2


In GNU Emacs 27.0.50 (build 7, x86_64-w64-mingw32)
 of 2019-05-12 built on MACHINE
Repository revision: 9e1bb6a2f6c2462b9652ffce706c549269740307
Repository branch: buster
Windowing system distributor 'Microsoft Corp.', version 10.0.18890
System Description: Microsoft Windows 10 Pro (v10.0.1903.18890.1000)

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Importing package-keyring.gpg...done
Package refresh done
error in process filter: package--check-signature-content: Failed to verify
signature: "archive-contents.sig"
error in process filter: Failed to verify signature: "archive-contents.sig"

Configured using:
 'configure --config-cache --with-modules --without-pop --without-dbus
 --without-gconf --without-gsettings CFLAGS=-O3'

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2
ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER LCMS2 GMP

Important settings:
  value of $EMACSLOADPATH: c:\emacs-lisp;
  value of $LANG: ENG
  locale-coding-system: cp1252

Major mode: Package Menu

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug sendmail help-mode mm-archive message
dired dired-loaddefs format-spec rfc822 mml mml-sec epa derived
gnus-util rmail rmail-loaddefs text-property-search time-date mailabbrev
gmm-utils mailheader mm-decode mm-bodies mm-encode mail-utils gnutls
network-stream url-http mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr url-gw nsm rmc puny url-cache url-auth url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap epg finder-inf package easymenu epg-config
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib elec-pair mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win w32-vars
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads w32notify w32 lcms2 multi-tty make-network-process
emacs)

Memory information:
((conses 16 76305 10007)
 (symbols 48 8292 1)
 (strings 32 24152 2632)
 (string-bytes 1 719178)
 (vectors 16 12539)
 (vector-slots 8 154153 12968)
 (floats 8 25 327)
 (intervals 56 2362 126)
 (buffers 992 13))

[-- Attachment #2: Type: text/html, Size: 5332 bytes --]

bug#35739: Acknowledgement (Bad signature from GNU ELPA)

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

I wrote:

>Repository revision: 9e1bb6a2f6c2462b9652ffce706c549269740307
>Repository branch: buster

This is a local branch with some irrelevant patches on top of this recent
commit in the FSF master branch:

fsf/master 29531785a17acf519070b73b488ad87ddd94aff7
Author:     Noam Postavsky <npostavs@gmail.com>
AuthorDate: Sun May 5 13:24:15 2019 -0400
Commit:     Noam Postavsky <npostavs@gmail.com>
CommitDate: Sun May 12 08:05:01 2019 -0400

[-- Attachment #2: Type: text/html, Size: 635 bytes --]

bug#35739: Bad signature from GNU ELPA

[Noam Postavsky]

retitle 35739 [w32] Bad signature from GNU ELPA for archive-contents
quit

Richard Copley <rcopley@gmail.com> writes:

> Recipe from 'emacs -Q': [M-x package-list-packages RET].
>
> Symptoms: The package list is displayed but an *Error* buffer pops up:
>
> Failed to verify signature archive-contents.sig:
> Bad signature from 474F05837FBDEF9B GNU ELPA Signing Agent (2014) <
> elpasign@elpa.gnu.org>
> Command output:
> gpg: Signature made 05/14/19 22:10:03 GMT Summer Time
> gpg:                using DSA key CA442C00F91774F17F59D9B0474F05837FBDEF9B
> gpg: BAD signature from "GNU ELPA Signing Agent (2014) <
> elpasign@elpa.gnu.org>" [unknown]

> gpg (GnuPG) 2.2.11

> In GNU Emacs 27.0.50 (build 7, x86_64-w64-mingw32)
>  of 2019-05-12 built on MACHINE

I can reproduce on my Windows machine, but not on GNU/Linux.  Maybe some
line endings are getting converted?

I also noticed that doing (setq package-check-signature nil), M-x
package-refresh-contents, (setq package-check-signature t), lets
packages be installed successfully.  It's only the archive-contents that
fails to verify.

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1.1: Type: text/plain, Size: 285 bytes --]

On Tue, 14 May 2019 at 23:04, Noam Postavsky <npostavs@gmail.com> wrote:

> I can reproduce on my Windows machine, but not on GNU/Linux.  Maybe some
> line endings are getting converted?
>

Thanks, that seems to be it: the attached patch gets rid of the bug (but
obviously isn't TRT).

[-- Attachment #1.2: Type: text/html, Size: 611 bytes --]

[-- Attachment #2: 35739-test.patch --]
[-- Type: application/octet-stream, Size: 1109 bytes --]

diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
index e35d999e0f..f8af2ce88c 100644
--- a/lisp/url/url-handlers.el
+++ b/lisp/url/url-handlers.el
@@ -334,11 +334,11 @@ url-insert-buffer-contents
       (when replace
         (delete-region (point-min) start)
         (delete-region (point) (point-max)))
-      (unless (cadr size-and-charset)
-        ;; If the headers don't specify any particular charset, use the
-        ;; usual heuristic/rules that we apply to files.
-        (decode-coding-inserted-region (point-min) (point) url
-                                       visit beg end replace))
+      ;; (unless (cadr size-and-charset)
+      ;;   ;; If the headers don't specify any particular charset, use the
+      ;;   ;; usual heuristic/rules that we apply to files.
+      ;;   (decode-coding-inserted-region (point-min) (point) url
+      ;;                                  visit beg end replace))
       (let ((inserted (car size-and-charset)))
         (when (fboundp 'after-insert-file-set-coding)
           (let ((insval (after-insert-file-set-coding inserted visit)))

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Tue, 14 May 2019 at 23:26, Richard Copley <rcopley@gmail.com> wrote:

>
> On Tue, 14 May 2019 at 23:04, Noam Postavsky <npostavs@gmail.com> wrote:
>
>> I can reproduce on my Windows machine, but not on GNU/Linux.  Maybe some
>> line endings are getting converted?
>>
>
> Thanks, that seems to be it: the attached patch gets rid of the bug (but
> obviously isn't TRT).
>

... and (apologies) I don't have a copyright assignment on file, so I'll
have to
leave it to Somebody to do TRT.

TIA, Somebody!
Buster.

[-- Attachment #2: Type: text/html, Size: 1328 bytes --]

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Tue, 14 May 2019 23:26:27 +0100
> Cc: 35739@debbugs.gnu.org
> 
> Thanks, that seems to be it: the attached patch gets rid of the bug (but obviously isn't TRT).
> 
> diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
> index e35d999e0f..f8af2ce88c 100644
> --- a/lisp/url/url-handlers.el
> +++ b/lisp/url/url-handlers.el
> @@ -334,11 +334,11 @@ url-insert-buffer-contents
>        (when replace
>          (delete-region (point-min) start)
>          (delete-region (point) (point-max)))
> -      (unless (cadr size-and-charset)
> -        ;; If the headers don't specify any particular charset, use the
> -        ;; usual heuristic/rules that we apply to files.
> -        (decode-coding-inserted-region (point-min) (point) url
> -                                       visit beg end replace))
> +      ;; (unless (cadr size-and-charset)
> +      ;;   ;; If the headers don't specify any particular charset, use the
> +      ;;   ;; usual heuristic/rules that we apply to files.
> +      ;;   (decode-coding-inserted-region (point-min) (point) url
> +      ;;                                  visit beg end replace))
>        (let ((inserted (car size-and-charset)))
>          (when (fboundp 'after-insert-file-set-coding)
>            (let ((insval (after-insert-file-set-coding inserted visit)))

I don't see how disabling decoding could make sense, can you explain?
What does this code do on GNU/Linux?

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Tue, 14 May 2019 23:42:31 +0100
> Cc: 35739@debbugs.gnu.org
> 
> ... and (apologies) I don't have a copyright assignment on file, so I'll have to
> leave it to Somebody to do TRT.

The change is small enough for us not to be bothered by that.

Thanks.

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Wed, 15 May 2019 at 03:42, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Richard Copley <rcopley@gmail.com>
> > Date: Tue, 14 May 2019 23:26:27 +0100
> > Cc: 35739@debbugs.gnu.org
> >
> > Thanks, that seems to be it: the attached patch gets rid of the bug (but
> obviously isn't TRT).
> >
> > diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
> > index e35d999e0f..f8af2ce88c 100644
> > --- a/lisp/url/url-handlers.el
> > +++ b/lisp/url/url-handlers.el
> > @@ -334,11 +334,11 @@ url-insert-buffer-contents
> >        (when replace
> >          (delete-region (point-min) start)
> >          (delete-region (point) (point-max)))
> > -      (unless (cadr size-and-charset)
> > -        ;; If the headers don't specify any particular charset, use the
> > -        ;; usual heuristic/rules that we apply to files.
> > -        (decode-coding-inserted-region (point-min) (point) url
> > -                                       visit beg end replace))
> > +      ;; (unless (cadr size-and-charset)
> > +      ;;   ;; If the headers don't specify any particular charset, use
> the
> > +      ;;   ;; usual heuristic/rules that we apply to files.
> > +      ;;   (decode-coding-inserted-region (point-min) (point) url
> > +      ;;                                  visit beg end replace))
> >        (let ((inserted (car size-and-charset)))
> >          (when (fboundp 'after-insert-file-set-coding)
> >            (let ((insval (after-insert-file-set-coding inserted visit)))
>
> I don't see how disabling decoding could make sense, can you explain?
>

Not in detail, it's not an area of expertise of mine. We call
(decode-coding-region (point-min) (point-max) 'undecided) on the
payload of "https://elpa.gnu.org/packages/archive-contents",
which is raw text. The resulting buffer's buffer-file-coding-
system is iso-latin-1-dos.

>
> What does this code do on GNU/Linux?
>

The same. The resulting coding system is iso-latin-1-unix.

[-- Attachment #2: Type: text/html, Size: 2973 bytes --]

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1.1: Type: text/plain, Size: 804 bytes --]

On Wed, 15 May 2019 at 03:42, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Richard Copley <rcopley@gmail.com>
> > Date: Tue, 14 May 2019 23:42:31 +0100
> > Cc: 35739@debbugs.gnu.org
> >
> > ... and (apologies) I don't have a copyright assignment on file, so I'll
> have to
> > leave it to Somebody to do TRT.
>
> The change is small enough for us not to be bothered by that.
>

OK, then I have another reason to leave it to someone else: I don't
understand the code in "package.el" and "url.el" well enough to be
confident making changes there.

The attached patch is less stupid, in that it doesn't have the
potential to break all the users of url-retrieve. Still, I'm not
asserting it's a sound or complete fix, or recommending it for
inclusion in Emacs. It does get rid of the error from my recipe.

[-- Attachment #1.2: Type: text/html, Size: 1351 bytes --]

[-- Attachment #2: 35739.patch --]
[-- Type: application/octet-stream, Size: 1006 bytes --]

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 949ad711ae..6b70c5e2a3 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1225,7 +1225,7 @@ package--with-work-buffer
                                                                    (goto-char (point-min))
                                                                    (unless (search-forward-regexp "^\r?\n\r?" nil 'noerror)
                                                                      (error "Error retrieving: %s %S" ,url-sym "incomprehensible buffer")))
-                                                                 (url-insert-buffer-contents ,b-sym ,url-sym)
+                                                                 (url-insert ,b-sym)
                                                                  (kill-buffer ,b-sym)
                                                                  (goto-char (point-min)))))
                                                nil

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1225,7 +1225,7 @@ package--with-work-buffer
>                                                                     (goto-char (point-min))
>                                                                     (unless (search-forward-regexp "^\r?\n\r?" nil 'noerror)
>                                                                       (error "Error retrieving: %s %S" ,url-sym "incomprehensible buffer")))
> -                                                                 (url-insert-buffer-contents ,b-sym ,url-sym)
> +                                                                 (url-insert ,b-sym)
>                                                                   (kill-buffer ,b-sym)
>                                                                   (goto-char (point-min)))))
>                                                 nil

[ Boy, this macro looks awfully deeply indented.
  We need to rewrite this to fit into the usual 80 columns.  ]

That actually looks very good [it would also need to change the other
url-insert-file-contents in that macro, of course].

Eli, do you think this could also be a fix for bug#34909?

E.g. something like the patch below?


        Stefan
        

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 949ad711ae..8a16dba1c2 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1202,40 +1182,45 @@ package--with-work-buffer
     (let ((url-sym (make-symbol "url"))
           (b-sym (make-symbol "b-sym")))
       `(cl-macrolet ((unless-error (body-2 &rest before-body)
-                                   (let ((err (make-symbol "err")))
-                                     `(with-temp-buffer
-                                        (when (condition-case ,err
-                                                  (progn ,@before-body t)
-                                                ,(list 'error ',error-form
-                                                       (list 'unless ',noerror-1
-                                                             `(signal (car ,err) (cdr ,err)))))
-                                          ,@body-2)))))
+                        (let ((err (make-symbol "err")))
+                          `(with-temp-buffer
+                             (set-buffer-multibyte nil)
+                             (when (condition-case ,err
+                                       (progn ,@before-body t)
+                                     ,(list 'error ',error-form
+                                            (list 'unless ',noerror-1
+                                                  `(signal (car ,err)
+                                                           (cdr ,err)))))
+                               ,@body-2)))))
          (if (string-match-p "\\`https?:" ,url-1)
              (let ((,url-sym (concat ,url-1 ,file)))
                (if ,async
                    (unless-error nil
-                                 (url-retrieve ,url-sym
-                                               (lambda (status)
-                                                 (let ((,b-sym (current-buffer)))
-                                                   (require 'url-handlers)
-                                                   (unless-error ,body
-                                                                 (when-let* ((er (plist-get status :error)))
-                                                                   (error "Error retrieving: %s %S" ,url-sym er))
-                                                                 (with-current-buffer ,b-sym
-                                                                   (goto-char (point-min))
-                                                                   (unless (search-forward-regexp "^\r?\n\r?" nil 'noerror)
-                                                                     (error "Error retrieving: %s %S" ,url-sym "incomprehensible buffer")))
-                                                                 (url-insert-buffer-contents ,b-sym ,url-sym)
-                                                                 (kill-buffer ,b-sym)
-                                                                 (goto-char (point-min)))))
-                                               nil
-                                               'silent))
-                 (unless-error ,body (url-insert-file-contents ,url-sym))))
+                     (url-retrieve
+                      ,url-sym
+                      (lambda (status)
+                        (let ((,b-sym (current-buffer)))
+                          (require 'url-handlers)
+                          (unless-error ,body
+                            (when-let* ((er (plist-get status :error)))
+                              (error "Error retrieving: %s %S" ,url-sym er))
+                            (with-current-buffer ,b-sym
+                              (goto-char (point-min))
+                              (unless (search-forward-regexp "^\r?\n\r?" nil t)
+                                (error "Error retrieving: %s %S"
+                                       ,url-sym "incomprehensible buffer")))
+                            (url-insert ,b-sym)
+                            (kill-buffer ,b-sym)
+                            (goto-char (point-min)))))
+                      nil
+                      'silent))
+                 (unless-error ,body (url-insert ,url-sym))))
            (unless-error ,body
-                         (let ((url (expand-file-name ,file ,url-1)))
-                           (unless (file-name-absolute-p url)
-                             (error "Location %s is not a url nor an absolute file name" url))
-                           (insert-file-contents url))))))))
+             (let ((url (expand-file-name ,file ,url-1)))
+               (unless (file-name-absolute-p url)
+                 (error "Location %s is not a url nor an absolute file name"
+                        url))
+               (insert-file-contents url))))))))
 
 (define-error 'bad-signature "Failed to verify signature")
 
@@ -1294,7 +1279,8 @@ package--check-signature
     (package--with-response-buffer location :file sig-file
       :async async :noerror t
       ;; Connection error is assumed to mean "no sig-file".
-      :error-form (let ((allow-unsigned (eq package-check-signature 'allow-unsigned)))
+      :error-form (let ((allow-unsigned
+                         (eq package-check-signature 'allow-unsigned)))
                     (when (and callback allow-unsigned)
                       (funcall callback nil))
                     (when unwind (funcall unwind))
@@ -1303,8 +1289,9 @@ package--check-signature
       ;; OTOH, an error here means "bad signature", which we never
       ;; suppress.  (Bug#22089)
       (unwind-protect
-          (let ((sig (package--check-signature-content (buffer-substring (point) (point-max))
-                                                       string sig-file)))
+          (let ((sig (package--check-signature-content
+                      (buffer-substring (point) (point-max))
+                      string sig-file)))
             (when callback (funcall callback sig))
             sig)
         (when unwind (funcall unwind))))))
@@ -1581,15 +1568,18 @@ package--download-one-archive
                 (member name package-unsigned-archives))
             ;; If we don't care about the signature, save the file and
             ;; we're done.
-            (progn (let ((coding-system-for-write 'utf-8))
-                     (write-region content nil local-file nil 'silent))
-                   (package--update-downloads-in-progress archive))
+            (progn
+             (cl-assert (not enable-multibyte-characters))
+             (let ((coding-system-for-write 'binary))
+               (write-region content nil local-file nil 'silent))
+             (package--update-downloads-in-progress archive))
           ;; If we care, check it (perhaps async) and *then* write the file.
           (package--check-signature
            location file content async
            ;; This function will be called after signature checking.
            (lambda (&optional good-sigs)
-             (let ((coding-system-for-write 'utf-8))
+             (cl-assert (not enable-multibyte-characters))
+             (let ((coding-system-for-write 'binary))
                (write-region content nil local-file nil 'silent))
              ;; Write out good signatures into archive-contents.signed file.
              (when good-sigs
@@ -1903,7 +1893,8 @@ package-install-from-archive
                ;; Update the old pkg-desc which will be shown on the description buffer.
                (setf (package-desc-signed pkg-desc) t)
                ;; Update the new (activated) pkg-desc as well.
-               (when-let* ((pkg-descs (cdr (assq (package-desc-name pkg-desc) package-alist))))
+               (when-let* ((pkg-descs (cdr (assq (package-desc-name pkg-desc)
+                                                 package-alist))))
                  (setf (package-desc-signed (car pkg-descs)) t))))))))))
 
 (defun package-installed-p (package &optional min-version)
@@ -2477,10 +2468,12 @@ describe-package-1
               (replace-match ""))))
 
       (if (package-installed-p desc)
-          ;; For installed packages, get the description from the installed files.
+          ;; For installed packages, get the description from the
+          ;; installed files.
           (insert (package--get-description desc))
 
-        ;; For non-built-in, non-installed packages, get description from the archive.
+        ;; For non-built-in, non-installed packages, get description from
+        ;; the archive.
         (let* ((basename (format "%s-readme.txt" name))
                readme-string)
 
diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
index e35d999e0f..783466ca70 100644
--- a/lisp/url/url-handlers.el
+++ b/lisp/url/url-handlers.el
@@ -299,7 +299,8 @@ url-file-local-copy
 (defun url-insert (buffer &optional beg end)
   "Insert the body of a URL object.
 BUFFER should be a complete URL buffer as returned by `url-retrieve'.
-If the headers specify a coding-system, it is applied to the body before it is inserted.
+If the headers specify a coding-system (and current buffer is multibyte),
+it is applied to the body before it is inserted.
 Returns a list of the form (SIZE CHARSET), where SIZE is the size in bytes
 of the inserted text and CHARSET is the charset that was specified in the header,
 or nil if none was found.
@@ -311,12 +312,13 @@ url-insert
                      (buffer-substring (+ (point-min) beg)
                                        (if end (+ (point-min) end) (point-max)))
 		   (buffer-string))))
-         (charset (mail-content-type-get (mm-handle-type handle)
-                                          'charset)))
+         (charset (if enable-multibyte-characters
+                      (mail-content-type-get (mm-handle-type handle)
+                                             'charset))))
     (mm-destroy-parts handle)
-    (if charset
-        (insert (mm-decode-string data (mm-charset-to-coding-system charset)))
-      (insert data))
+    (insert (if charset
+                (mm-decode-string data (mm-charset-to-coding-system charset))
+              data))
     (list (length data) charset)))
 
 (defvar url-http-codes)

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Wed, 15 May 2019 07:46:12 +0100
> Cc: Noam Postavsky <npostavs@gmail.com>, 35739@debbugs.gnu.org
> 
>  I don't see how disabling decoding could make sense, can you explain?
> 
> Not in detail, it's not an area of expertise of mine. We call
> (decode-coding-region (point-min) (point-max) 'undecided) on the
> payload of "https://elpa.gnu.org/packages/archive-contents",
> which is raw text. The resulting buffer's buffer-file-coding-
> system is iso-latin-1-dos.
>  
> 
>  What does this code do on GNU/Linux?
> 
> The same. The resulting coding system is iso-latin-1-unix.

That URL seems to bring ASCII text.  Are you saying that GPG produces
a wrong signature because EOL format is significant for it?  (Please
forgive silly questions about GPG: I seldom if ever use it.)

In any case, if we don't want EOL conversion, we should bind
inhibit-eol-conversion to a non-nil value, and change nothing else.
But this should not be done in url-insert-buffer-contents, it should
be done in package.el, because the former is a general utility and not
necessarily needs to inhibit EOL conversion for all of its callers.

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  35739@debbugs.gnu.org,  Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 15 May 2019 10:03:57 -0400
> 
> That actually looks very good [it would also need to change the other
> url-insert-file-contents in that macro, of course].
> 
> Eli, do you think this could also be a fix for bug#34909?
> 
> E.g. something like the patch below?

I don't know.  I don't yet have a handle on what happens here, and
therefore I don't understand how replacing url-insert-buffer-contents
with url-insert should fix that.  I'm probably missing something.

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On Wed, 15 May 2019, 15:40 Eli Zaretskii, <eliz@gnu.org> wrote:

> > From: Richard Copley <rcopley@gmail.com>
> > Date: Wed, 15 May 2019 07:46:12 +0100
> > Cc: Noam Postavsky <npostavs@gmail.com>, 35739@debbugs.gnu.org
> >
> >  I don't see how disabling decoding could make sense, can you explain?
> >
> > Not in detail, it's not an area of expertise of mine. We call
> > (decode-coding-region (point-min) (point-max) 'undecided) on the
> > payload of "https://elpa.gnu.org/packages/archive-contents",
> > which is raw text. The resulting buffer's buffer-file-coding-
> > system is iso-latin-1-dos.
> >
> >
> >  What does this code do on GNU/Linux?
> >
> > The same. The resulting coding system is iso-latin-1-unix.
>
> That URL seems to bring ASCII text.  Are you saying that GPG produces
> a wrong signature because EOL format is significant for it?  (Please
> forgive silly questions about GPG: I seldom if ever use it.)
>

Getting the signature involves applying a hash function to the bytes
in question. It's desirable that two different byte sequences give rise
to two different signatures, even if the difference is a carriage return.

In any case, if we don't want EOL conversion, we should bind
> inhibit-eol-conversion to a non-nil value, and change nothing else.
> But this should not be done in url-insert-buffer-contents, it should
> be done in package.el, because the former is a general utility and not
> necessarily needs to inhibit EOL conversion for all of its callers.
>

Of course. I was confirming Noam's hunch, not suggesting a change.
Sorry that wasn't clear.

[-- Attachment #2: Type: text/html, Size: 2998 bytes --]

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> I don't know.  I don't yet have a handle on what happens here, and
> therefore I don't understand how replacing url-insert-buffer-contents
> with url-insert should fix that.  I'm probably missing something.

After playing some more with it, I found a few problems, tracked down
the origin of the decoding (which was introduced for the case where we
download the <pkg>-readme.txt description file) and installed a patch
into master which should fix this right.

Now the question is how to adapt the fix for emacs-26: the patch
I installed is too invasive for emacs-26, I think.

Maybe we can patch over the problem by using `last-coding-system` instead
of `utf-8`?


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Sat, 18 May 2019 18:36:50 -0400
> 
> > I don't know.  I don't yet have a handle on what happens here, and
> > therefore I don't understand how replacing url-insert-buffer-contents
> > with url-insert should fix that.  I'm probably missing something.
> 
> After playing some more with it, I found a few problems, tracked down
> the origin of the decoding (which was introduced for the case where we
> download the <pkg>-readme.txt description file) and installed a patch
> into master which should fix this right.
> 
> Now the question is how to adapt the fix for emacs-26: the patch
> I installed is too invasive for emacs-26, I think.
> 
> Maybe we can patch over the problem by using `last-coding-system` instead
> of `utf-8`?

I don't think I understand the change enough to say something
intelligent here.  The commit explains, o some extent, why the
original code failed, but it says nothing about the way the new code
solves the problem without introducing new ones.

I'm also mildly worried about the incompatible change in url-insert,
which is a general-purpose function not limited to package.el and its
signature verification.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> I don't think I understand the change enough to say something
> intelligent here.  The commit explains, o some extent, why the
> original code failed, but it says nothing about the way the new code
> solves the problem without introducing new ones.

It solves the problem by refraining from decoding until we know
positively that it needs to happen.  In the patch I installed, I only
end up decoding the readme.txt descriptions.  Maybe there are other
places where decoding is needed, if so we'll need to fix those cases
I missed when we find them.

> I'm also mildly worried about the incompatible change in url-insert,
> which is a general-purpose function not limited to package.el and its
> signature verification.

Yes, this part should definitely not be in emacs-26.

Luckily, I think the rest of the change still solves our problem in
practice in Emacs-26 (the change in url-insert only affects the case
where the HTTP headers returned by the server specify a particular
"charset", which is not the case when downloading .tar and .el files
from elpa.gnu.org AFAICT).


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Wed, 22 May 2019 12:20:01 -0400
> 
> It solves the problem by refraining from decoding until we know
> positively that it needs to happen.

You refrain from decoding what?  E.g., Lisp files must be in UTF-8,
right?  And what about the descriptions we show in lisp-packages?

Or maybe I don't really understand why we need to decode _anything_,
since we just download a .tar archive, right?

> > I'm also mildly worried about the incompatible change in url-insert,
> > which is a general-purpose function not limited to package.el and its
> > signature verification.
> 
> Yes, this part should definitely not be in emacs-26.

I'm actually asking why it should be on master.

> the change in url-insert only affects the case where the HTTP
> headers returned by the server specify a particular "charset", which
> is not the case when downloading .tar and .el files from
> elpa.gnu.org AFAICT

Why not? isn't that dangerous?

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> It solves the problem by refraining from decoding until we know
>> positively that it needs to happen.
> You refrain from decoding what?

The files we download.

> E.g., Lisp files must be in UTF-8, right?

At first we don't care: we gets files (tarballs, GPG signatures, Elisp
files, ...) and while some of them may need decoding later on, not all
do.  And for purposes of signature checking, for some of those files we
need to get the exact original sequence of bytes, which is easier to get
if we only decode *after* signature checking rather than before.
For this reason we don't want to let URL do the decoding for us.

It's true that for "simple packages" made of a single .el file, after
signature checking we should maybe decode them as utf-8.  The main thing
we do with those is to save them as files and for that we don't
need decoding.  I haven't checked whether we do anything else
significant with those undecoded .el buffers, so maybe I missed an
explicit decode somewhere in there.

> And what about the descriptions we show in lisp-packages?

Not sure what you mean by that (I already mentioned the *-readme.txt
files which we do decode explicitly now).

> Or maybe I don't really understand why we need to decode _anything_,
> since we just download a .tar archive, right?

There's more than just tarballs.

>> Yes, this part should definitely not be in emacs-26.
> I'm actually asking why it should be on master.

It seemed like a simple way to provide this new functionality.
The functionality is needed at least by package.el, and I see no reason
why it should be the only client of URL that needs this functionality.

>> the change in url-insert only affects the case where the HTTP
>> headers returned by the server specify a particular "charset", which
>> is not the case when downloading .tar and .el files from
>> elpa.gnu.org AFAICT
> Why not?

For tarballs, since it's not a text/* format, it wouldn't make much
sense to specify a charset.  For Elisp files, it might just be a happy
accident of the configuration of the HTTP server, but my impression is
that nowadays it's considered a bad idea to rely on the HTTP headers to
tell you about the encoding (instead, the data contents should specify
its own encoding), so it's only useful for text/plain files.

> isn't that dangerous?

Dangerous?  definitely not.  All it means is that if you try to view
something like https://elpa.gnu.org/packages/xclip-1.8.el in your
browser there's a possibility that it will not display the non-ASCII
characters properly.  Of course, most browsers I know won't display
the file at all anyway (they ask you where to save it instead because
they don't know what else to do with text/x-emacs-lisp).


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Wed, 22 May 2019 15:40:46 -0400
> 
> > You refrain from decoding what?
> 
> The files we download.
> 
> > E.g., Lisp files must be in UTF-8, right?
> 
> At first we don't care: we gets files (tarballs, GPG signatures, Elisp
> files, ...) and while some of them may need decoding later on, not all
> do.  And for purposes of signature checking, for some of those files we
> need to get the exact original sequence of bytes, which is easier to get
> if we only decode *after* signature checking rather than before.
> For this reason we don't want to let URL do the decoding for us.

Where is the decoding happening, then?  According to what you write,
URL should just save the files as is, and then decoding should happen
when we access the resulting files after saving them.  Is that what
happens after the changes?

> > And what about the descriptions we show in lisp-packages?
> 
> Not sure what you mean by that (I already mentioned the *-readme.txt
> files which we do decode explicitly now).
> 
> > Or maybe I don't really understand why we need to decode _anything_,
> > since we just download a .tar archive, right?

Sorry for the typo: I meant list-packages.  Does that display come
from the README files?

> >> Yes, this part should definitely not be in emacs-26.
> > I'm actually asking why it should be on master.
> 
> It seemed like a simple way to provide this new functionality.
> The functionality is needed at least by package.el, and I see no reason
> why it should be the only client of URL that needs this functionality.

Other clients might need it in other ways.  From what you explain, it
sounds like package.el doesn't need to decode at all when it
downloads, so it should disregard the 'charset' header and always
treat the stuff it gets as a raw byte stream.  Is that correct?  If it
is correct, then there's no need to make any changes in url*.el
routines.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> Where is the decoding happening, then?  According to what you write,
> URL should just save the files as is, and then decoding should happen
> when we access the resulting files after saving them.  Is that what
> happens after the changes?

That's right.

> Sorry for the typo: I meant list-packages.  Does that display come
> from the README files?

It can come from various places, but the only case affected by my change
is when it comes from the remote archive in which case it's indeed the
*-readme.txt file that I do decode explicitly.

> Other clients might need it in other ways.  From what you explain, it
> sounds like package.el doesn't need to decode at all when it
> downloads, so it should disregard the 'charset' header and always
> treat the stuff it gets as a raw byte stream.  Is that correct?

That's right.

> If it is correct, then there's no need to make any changes in
> url*.el routines.

There is, because the routine that extracts the "raw bytes" is url-insert
which (until my patch) also did the decoding according to the "charset"
specified in the HTTP headers returned by the server (if present).
IOW it didn't always return the raw bytes.

Note that other clients will only be negatively affected by the change if:
- the HTTP server has returned a "charset" in its headers.
- they url-insert into a unibyte buffer.
- they need the text to be decoded.
The last two points should be mutually exclusive in sane situations, so
I think the change is pretty safe.  Or to put it another way, I think
it's more likely to uncover or even fix a bug than to introduce one.


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Thu, 23 May 2019 00:06:19 -0400
> 
> > Where is the decoding happening, then?  According to what you write,
> > URL should just save the files as is, and then decoding should happen
> > when we access the resulting files after saving them.  Is that what
> > happens after the changes?
> 
> That's right.
> 
> > Sorry for the typo: I meant list-packages.  Does that display come
> > from the README files?
> 
> It can come from various places, but the only case affected by my change
> is when it comes from the remote archive in which case it's indeed the
> *-readme.txt file that I do decode explicitly.
> 
> > Other clients might need it in other ways.  From what you explain, it
> > sounds like package.el doesn't need to decode at all when it
> > downloads, so it should disregard the 'charset' header and always
> > treat the stuff it gets as a raw byte stream.  Is that correct?
> 
> That's right.

Sounds good, thanks.

> > If it is correct, then there's no need to make any changes in
> > url*.el routines.
> 
> There is, because the routine that extracts the "raw bytes" is url-insert
> which (until my patch) also did the decoding according to the "charset"
> specified in the HTTP headers returned by the server (if present).
> IOW it didn't always return the raw bytes.

Then how about an optional argument to disable 'charset' handling
instead?  That'd be backward-compatible.

> Note that other clients will only be negatively affected by the change if:
> - the HTTP server has returned a "charset" in its headers.
> - they url-insert into a unibyte buffer.
> - they need the text to be decoded.
> The last two points should be mutually exclusive in sane situations, so
> I think the change is pretty safe.  Or to put it another way, I think
> it's more likely to uncover or even fix a bug than to introduce one.

I'd prefer a backward-compatible change, because that saves us from
the need to be 100% right when estimating the collateral damage,
something that we have failed in several cases in the past.

Thanks.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> There is, because the routine that extracts the "raw bytes" is url-insert
>> which (until my patch) also did the decoding according to the "charset"
>> specified in the HTTP headers returned by the server (if present).
>> IOW it didn't always return the raw bytes.
>
> Then how about an optional argument to disable 'charset' handling
> instead?  That'd be backward-compatible.

I think people have already enough trouble dealing with character
encodings that we shouldn't encourage them to put decoded text into
unibyte buffers (I don't even know where/if the semantics of this is
described somewhere in our manuals or code).

IOW, from where I stand, the change in behavior is just fixing
a long-standing bug.


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Thu, 23 May 2019 08:10:41 -0400
> 
> > Then how about an optional argument to disable 'charset' handling
> > instead?  That'd be backward-compatible.
> 
> I think people have already enough trouble dealing with character
> encodings that we shouldn't encourage them to put decoded text into
> unibyte buffers (I don't even know where/if the semantics of this is
> described somewhere in our manuals or code).
> 
> IOW, from where I stand, the change in behavior is just fixing
> a long-standing bug.

I guess we are standing on different places, then.  I'd like us not to
change that behavior by default.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> I guess we are standing on different places, then.

We all do ;-)

> I'd like us not to change that behavior by default.

I think in this case we need to define it (i.e. document what it means
to insert decoded text into a unibyte buffer).


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Fri, 24 May 2019 10:22:05 -0400
> 
> > I'd like us not to change that behavior by default.
> 
> I think in this case we need to define it (i.e. document what it means
> to insert decoded text into a unibyte buffer).

Why do we have to define that only for that specific function?

And why is that relevant to the issue at hand, which is not to make
backward-incompatible changes in existing APIs?

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> > I'd like us not to change that behavior by default.
>> I think in this case we need to define it (i.e. document what it means
>> to insert decoded text into a unibyte buffer).
> Why do we have to define that only for that specific function?

I didn't mean for this specific function, I meant in general.

> And why is that relevant to the issue at hand, which is not to make
> backward-incompatible changes in existing APIs?

I consider myself fairly well versed in Emacs's coding systems.  Yet,
I don't know what decoding text into a unibyte buffer will do and can't
find any doc that describes it.

I such a context, I think preserving such a behavior is harmful in the
long term because it will keep fundamentally buggy code half-working
and keep spreading confusion about how coding systems work and people
copy&pasting broken code.

In the long term, I think we're better off introducing limited
breakage where it leads to clearer errors that help clear up such
confusions and fix bugs.


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Fri, 24 May 2019 15:31:42 -0400
> 
> > And why is that relevant to the issue at hand, which is not to make
> > backward-incompatible changes in existing APIs?
> 
> I consider myself fairly well versed in Emacs's coding systems.  Yet,
> I don't know what decoding text into a unibyte buffer will do and can't
> find any doc that describes it.
> 
> I such a context, I think preserving such a behavior is harmful in the
> long term because it will keep fundamentally buggy code half-working
> and keep spreading confusion about how coding systems work and people
> copy&pasting broken code.
> 
> In the long term, I think we're better off introducing limited
> breakage where it leads to clearer errors that help clear up such
> confusions and fix bugs.

I disagree.  This stuff worked for decades, and breaking it only
because it is under-documented is not a good idea.  I'm okay with
documenting what that produces somewhere, but not with breaking it.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> I'm okay with documenting what that produces somewhere

That would be a big help,


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Fri, 24 May 2019 15:51:28 -0400
> 
> > I'm okay with documenting what that produces somewhere
> 
> That would be a big help,

Where would you like this to be documented?

Btw, in two different messages in this thread you mentioned two
different situations.  One is this:

> document what it means to insert decoded text into a unibyte buffer

The other is this:

> I don't know what decoding text into a unibyte buffer will do

The former seems to imply the following:

  (with-current-buffer BUFFER
    (set-buffer-multibyte nil)
    (insert (decode-coding-string STRING CODING-SYSTEM)))

while the latter implies this:

  (with-current-buffer BUFFER
    (set-buffer-multibyte nil))
  (decode-coding-string STRING CODING-SYSTEM nil BUFFER)

The results of these 2 use cases are not the same.

So I don't yet have a clear idea what exactly you want to be
documented.  In particular, what 'insert' does in the 1st use case
seems to be documented in its doc string, but maybe that is not
enough?

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> > I'm okay with documenting what that produces somewhere
>> That would be a big help,
> Where would you like this to be documented?

Any place is fine by me (including a comment in a C file).
But I guess it would be better in Texinfo somewhere so we can point to
it more easily.

> Btw, in two different messages in this thread you mentioned two
> different situations.  One is this:
>> document what it means to insert decoded text into a unibyte buffer
> The other is this:
>> I don't know what decoding text into a unibyte buffer will do

[ Indeed, I corrected myself along the way.  ]
I'm mostly interested in the second, because I think I know what the
first does (tho I think what it does is basically flawed, a holdover
from the Emacs-20/21 days where most code was so hopelessly confused
about chars-vs-bytes that it was a good idea to try and patch things
over so it at least keeps working for environments like latin1 and
koi-r, but nowadays we'd be better off signaling errors to try and fix
the remaining confusion).


        Stefan

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rcopley@gmail.com,  35739@debbugs.gnu.org,  npostavs@gmail.com
> Date: Sat, 25 May 2019 14:38:56 -0400
> 
> >> > I'm okay with documenting what that produces somewhere
> >> That would be a big help,
> > Where would you like this to be documented?
> 
> Any place is fine by me (including a comment in a C file).
> But I guess it would be better in Texinfo somewhere so we can point to
> it more easily.

Done.

I hope we can now talk about removing the incompatible change from
url-insert.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

> Not in detail, it's not an area of expertise of mine. We call
> (decode-coding-region (point-min) (point-max) 'undecided) on the
> payload of "https://elpa.gnu.org/packages/archive-contents",
> which is raw text. The resulting buffer's buffer-file-coding-
> system is iso-latin-1-dos.

Indeed, it seems that url-insert-file-contents sets
buffer-file-coding-system.  Maybe we can use that in the emacs-26
branch.  Can you try the emacs-26 code with the patch below to see if it
fixes the current problem?


        Stefan


2019-05-29  Stefan Monnier  <monnier@iro.umontreal.ca>

	* lisp/emacs-lisp/package.el: Obey buffer-file-coding-system.
	`url-insert-file-contents` saves in buffer-file-coding-system
	the coding-system used to decode the contents.  Preserve this
	as the contents is moved from buffer to string to buffer, and use
	it when saving the contents to file, so as to try and better preserve
	the original byte sequence (bug#35739).
	(package--buffer-string, package--cs): New functions.
	(package--check-signature): Encode `string` if a coding-system
	was specified in buffer-file-coding-system.
	(package--download-one-archive, package-install-from-archive):
	Obey and preserve the buffer-file-coding-system if specified.


diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 1a185de4a5..46f7c91272 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1241,6 +1241,17 @@ package--check-signature-content
         (signal 'bad-signature (list sig-file)))
       good-signatures)))
 
+(defun package--buffer-string ()
+  (let ((string (buffer-string)))
+    (when (and buffer-file-coding-system
+               (> (length string) 0))
+      (put-text-property 0 1 'package--cs buffer-file-coding-system string))
+    string))
+
+(defun package--cs (string)
+  (and (> (length string) 0)
+       (get-text-property 0 'package--cs string)))
+
 (defun package--check-signature (location file &optional string async callback unwind)
   "Check signature of the current buffer.
 Download the signature file from LOCATION by appending \".sig\"
@@ -1260,8 +1271,12 @@ package--check-signature
 
 UNWIND, if provided, is a function to be called after everything
 else, even if an error is signaled."
-  (let ((sig-file (concat file ".sig"))
-        (string (or string (buffer-string))))
+  (let* ((sig-file (concat file ".sig"))
+         (string (or string (package--buffer-string)))
+         (cs (package--cs string)))
+    ;; Re-encode the downloaded file with the coding-system with which
+    ;; it was decoded, so we (hopefully) get the exact same bytes back.
+    (when cs (setq string (encode-coding-string string cs)))
     (package--with-response-buffer location :file sig-file
       :async async :noerror t
       ;; Connection error is assumed to mean "no sig-file".
@@ -1529,7 +1544,7 @@ package--download-one-archive
     :error-form (package--update-downloads-in-progress archive)
     (let* ((location (cdr archive))
            (name (car archive))
-           (content (buffer-string))
+           (content (package--buffer-string))
            (dir (expand-file-name (format "archives/%s" name) package-user-dir))
            (local-file (expand-file-name file dir)))
       (when (listp (read content))
@@ -1538,7 +1553,8 @@ package--download-one-archive
                 (member name package-unsigned-archives))
             ;; If we don't care about the signature, save the file and
             ;; we're done.
-            (progn (let ((coding-system-for-write 'utf-8))
+            (progn (let ((coding-system-for-write
+                          (or (package--cs content) 'utf-8)))
                      (write-region content nil local-file nil 'silent))
                    (package--update-downloads-in-progress archive))
           ;; If we care, check it (perhaps async) and *then* write the file.
@@ -1546,7 +1562,7 @@ package--download-one-archive
            location file content async
            ;; This function will be called after signature checking.
            (lambda (&optional good-sigs)
-             (let ((coding-system-for-write 'utf-8))
+             (let ((coding-system-for-write (or (package--cs content) 'utf-8)))
                (write-region content nil local-file nil 'silent))
              ;; Write out good signatures into archive-contents.signed file.
              (when good-sigs
@@ -1838,15 +1854,17 @@ package-install-from-archive
           (let ((save-silently t))
             (package-unpack pkg-desc))
         ;; If we care, check it and *then* write the file.
-        (let ((content (buffer-string)))
+        (let ((content (package--buffer-string)))
           (package--check-signature
            location file content nil
            ;; This function will be called after signature checking.
            (lambda (&optional good-sigs)
              ;; Signature checked, unpack now.
-             (with-temp-buffer (insert content)
-                               (let ((save-silently t))
-                                 (package-unpack pkg-desc)))
+             (with-temp-buffer
+               (insert content)
+               (setq buffer-file-coding-system (package--cs content))
+               (let ((save-silently t))
+                 (package-unpack pkg-desc)))
              ;; Here the package has been installed successfully, mark it as
              ;; signed if appropriate.
              (when good-sigs

bug#35739: Bad signature from GNU ELPA

[Richard Copley]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On Wed, 29 May 2019 at 19:45, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > Not in detail, it's not an area of expertise of mine. We call
> > (decode-coding-region (point-min) (point-max) 'undecided) on the
> > payload of "https://elpa.gnu.org/packages/archive-contents",
> > which is raw text. The resulting buffer's buffer-file-coding-
> > system is iso-latin-1-dos.
>
> Indeed, it seems that url-insert-file-contents sets
> buffer-file-coding-system.  Maybe we can use that in the emacs-26
> branch.  Can you try the emacs-26 code with the patch below to see if it
> fixes the current problem?
>

Yes, it seems to. At least, there's no error from [M-x
package-list-packages] in emacs -Q.

[-- Attachment #2: Type: text/html, Size: 1188 bytes --]

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> > Not in detail, it's not an area of expertise of mine. We call
>> > (decode-coding-region (point-min) (point-max) 'undecided) on the
>> > payload of "https://elpa.gnu.org/packages/archive-contents",
>> > which is raw text. The resulting buffer's buffer-file-coding-
>> > system is iso-latin-1-dos.
>>
>> Indeed, it seems that url-insert-file-contents sets
>> buffer-file-coding-system.  Maybe we can use that in the emacs-26
>> branch.  Can you try the emacs-26 code with the patch below to see if it
>> fixes the current problem?
>>
>
> Yes, it seems to. At least, there's no error from [M-x
> package-list-packages] in emacs -Q.

Thanks Richard for promptly testing it.

Noam, can you confirm that you can also still reproduce the problem in
`emacs-26` and that my patch fixes it when you try it?

Eli, do you think this is safe enough for the `emacs-26` branch?


        Stefan

bug#35739: Bad signature from GNU ELPA

[Noam Postavsky]

On Wed, 29 May 2019 at 16:07, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Noam, can you confirm that you can also still reproduce the problem in
> `emacs-26` and that my patch fixes it when you try it?

Yes and yes. I also tried installing ascii-art-to-unicode (since it
has some "interesting" characters) and it downloaded fine too.

bug#35739: Bad signature from GNU ELPA

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 35739@debbugs.gnu.org,  Richard Copley <rcopley@gmail.com>
> Date: Wed, 29 May 2019 16:07:10 -0400
> 
> Eli, do you think this is safe enough for the `emacs-26` branch?

Yes.

bug#35739: Bad signature from GNU ELPA

[Stefan Monnier]

>> Eli, do you think this is safe enough for the `emacs-26` branch?
> Yes.

Thanks, installed,


        Stefan