bug#58367: `package-install-file' rejects some .tar files (tentative patch)

bug#58367: `package-install-file' rejects some .tar files (tentative patch)

[Richard Hopkins]

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

`package-install-file' signals an error for some .tar packages based
on their structure - both file layout and tar format.

This is specific to the package routines for .tar files as Emacs can
understand them fine, and can also install the packages if they have
already been extracted.

There's 2 different issues that cause this:

1) An old style .tar file (autoconf tar-v7, `tar cof ...`) vs a new
style file (autoconf tar-ustar, `tar cf ...`)

2) A package where all files are in a single directory vs having
child directories.  A package with child directories currently only
installs successfully if it's already extracted; the manual also says
they are allowed.

Combining these issues the current state of package installation using
.tar files is:

* ustar-nosub - error
* ustar-withsub - error
* v7-nosub - success
* v7-withsub - error

The main issue is how the package functions try to calculate the root
of the package directory and from there it can locate the "-pkg.el"
file etc.  From this we get two different errors:

1) Cannot handle a plain directory being first and missing a path
separator.  e.g. This happens for ustar files with or without a sub
directory.

$ tar tf ustar-nosub-0.1.tar
ustar-nosub-0.1
ustar-nosub-0.1/ustar-nosub.el
ustar-nosub-0.1/ustar-nosub-pkg.el

M-: (package-install-file "ustar-nosub-0.1.tar")
Debugger entered--Lisp error: (wrong-type-argument stringp nil)
   directory-file-name(nil)
   package--description-file(nil)
   package-tar-file-info()
   package-install-from-buffer()
   package-install-file("~/tmp/pkgtest/tars/ustar-nosub-0.1.tar")
   funcall-interactively(package-install-file 
"~/tmp/pkgtest/tars/ustar-nosub-0.1.tar")
   call-interactively(package-install-file record nil)
   command-execute(package-install-file record)
   execute-extended-command(nil "package-install-file" 
"package-install-file")
   funcall-interactively(execute-extended-command nil 
"package-install-file" "package-install-file")
   call-interactively(execute-extended-command nil nil)
   command-execute(execute-extended-command)

2) Looks for "-pkg.el" in a sub directory when it won't be there.
This happens for a v7 file but may also happen for a ustar if it
wasn't for error (1).

$ tar tf v7-withsub-0.1.tar
v7-withsub-0.1/v7-sub/foo.txt
v7-withsub-0.1/v7-withsub.el
v7-withsub-0.1/v7-withsub-pkg.el

M-: (package-install-file "tar tf v7-withsub-0.1.tar")
Debugger entered--Lisp error: (wrong-type-argument arrayp nil)
   tar--describe-as-link(nil)
   tar--check-descriptor(nil)
   tar-get-file-descriptor("v7-withsub-0.1/v7-sub/v7-sub-pkg.el")
   package-tar-file-info()
   package-install-from-buffer()
   package-install-file("~/tmp/pkgtest/tars/v7-withsub-0.1.tar")
   funcall-interactively(package-install-file 
"~/tmp/pkgtest/tars/v7-withsub-0.1.tar")
   call-interactively(package-install-file record nil)
   command-execute(package-install-file record)
   execute-extended-command(nil "package-install-file" 
"package-install-file")
   funcall-interactively(execute-extended-command nil 
"package-install-file" "package-install-file")
   call-interactively(execute-extended-command nil nil)
   command-execute(execute-extended-command)

For comparison the only .tar file that works is an old style v7 file
with all files in a single directory

$ tar tf v7-nosub-0.1.tar
v7-nosub-0.1/v7-nosub.el
v7-nosub-0.1/v7-nosub-pkg.el

M-: (package-install-file "v7-nosub-0.1.tar")
success

I've attached the example files to help you investigate.

There is also a proof of concept patch as it fixes ustar files with or
without sub directories, but it still fails on v7-withsub making the
new summary:

* ustar-nosub - success
* ustar-withsub - success
* v7-nosub - success
* v7-withsub - error

The modified functions were `package-untar-buffer' and
`package-tar-file-info'.

p.s. These results are from OpenBSD 7.1 with their `tar` and
Emacs 28.2 and master.

A related issue #13136 - 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=13136

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tar-package-install-file.patch --]
[-- Type: text/x-diff; name=0001-tar-package-install-file.patch, Size: 2436 bytes --]

Teach `package-install-file' to install previously rejected tarballs

Whilst Emacs can open the varieties of tarballs with no problem, the
package management functions depended on a particular table layout in
the tarball to calculate the directory for the package.

For example, with BSD tar: `tar cf ...` would generate a rejected
tarball, and `tar cof ...` would generate an accepted one.

* lisp/emacs-lisp/package.el (package-untar-buffer): Compare against
the header name from the tarball rather than the filesystem
expanded form.
(package-tar-file-info): Also allow the first entry to be a plain
directory name without trailing path separator.
---
 lisp/emacs-lisp/package.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 4268f7d27a..cc371fbc50 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -926,11 +926,12 @@ package-untar-buffer
   (let ((regexp (concat "\\`" (regexp-quote (expand-file-name dir)) "/"))
         (case-fold-search (file-name-case-insensitive-p dir)))
     (dolist (tar-data tar-parse-info)
-      (let ((name (expand-file-name (tar-header-name tar-data))))
+      (let* ((header-name (tar-header-name tar-data))
+             (name (expand-file-name header-name)))
         (or (string-match regexp name)
             ;; Tarballs created by some utilities don't list
             ;; directories with a trailing slash (Bug#13136).
-            (and (string-equal dir name)
+            (and (string-equal dir header-name)
                  (eq (tar-header-link-type tar-data) 5))
             (error "Package does not untar cleanly into directory %s/" dir)))))
   (tar-untar-buffer))
@@ -1192,8 +1193,10 @@ package-tar-file-info
   "Find package information for a tar file.
 The return result is a `package-desc'."
   (cl-assert (derived-mode-p 'tar-mode))
-  (let* ((dir-name (file-name-directory
-                    (tar-header-name (car tar-parse-info))))
+  (let* ((header-name (tar-header-name (car tar-parse-info)))
+	 (dir-name (or (file-name-directory header-name)
+                       ;; Some tar utilities generate a plain directory name first
+                       (file-name-as-directory header-name)))
          (desc-file (package--description-file dir-name))
          (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
     (unless tar-desc
2.38.0


[-- Attachment #3: tar-package-install-file.tar.gz --]
[-- Type: application/gzip, Size: 2342 bytes --]

bug#58367: `package-install-file' rejects some .tar files (tentative patch)

[Lars Ingebrigtsen]

Richard Hopkins <emacs@unbit.co.uk> writes:

> `package-install-file' signals an error for some .tar packages based
> on their structure - both file layout and tar format.
>
> This is specific to the package routines for .tar files as Emacs can
> understand them fine, and can also install the packages if they have
> already been extracted.

Perhaps Stefan has some comments here; added to the CCs.

bug#58367: `package-install-file' rejects some .tar files (tentative patch)

[Richard Hopkins]

I've found another difference regarding `package-install-file' and a
package with subdirectories that I can reproduce with the previously
attached files.  This is the difference between installing a directory
or installing a .tar file.  For example

M-: (package-install-file "ustar-withsub-0.1")
;; * installs but ignores subdirectory

M-: (package-install-file "ustar-withsub-0.1.tar")
;; * 28.2 - error
;; * with patch - installs and includes subdirectory

I believe sub directories should be allowed from this part of the docs
and the patch now allows for it as well as fixing the error.

     "...Files may also extract into subdirectories of the content
     directory. ..."
     
https://www.gnu.org/software/emacs/manual/html_node/elisp/Multi_002dfile-Packages.html

It would be to good to see if anyone can reproduce these as it looks
like more patches are needed to support sub directories?

1) for v7-withsub-0.1.tar so it looks at the top level for "-pkg.el"

2) include subdirectories when installing from a directory to match
functionality of installing from .tar.

bug#58367: `package-install-file' rejects some .tar files (tentative patch)

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 2) A package where all files are in a single directory vs having
> child directories.  A package with child directories currently only
> installs successfully if it's already extracted; the manual also says
> they are allowed.

I'm pretty sure I've successfully installed the Hyperbole package
(which includes subdirectories) directly from GNU ELPA and I haven't
heard complaints about the Proof-General (M|NonGNU)ELPA package (which
relies even more heavily on subdirectories) either, so the problem is
likely linked to some other "detail".

[..time passes..]

Thanks for the tarballs for the tests, they were very helpful.
The patch also pointed in the right direction.

I installed the patch below (plus regression tests) into `master`.

[ Whether the name comparisons should be done on names that have gone
  through `expand-file-name` or not is a good question.
  Efficiency/simplicity suggests not to do that, and the code's history
  doesn't explain why it's done, but I have the vague recollection that
  it tries to handle some cases of funny file names, e.g. with .. in
  them.
  In the end I kept the `expand-file-name` version mostly so as to
  reduce the size of the change (and associated risk of regression).  ]


        Stefan


diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 4268f7d27a7..d619142d64c 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -930,7 +930,7 @@ package-untar-buffer
         (or (string-match regexp name)
             ;; Tarballs created by some utilities don't list
             ;; directories with a trailing slash (Bug#13136).
-            (and (string-equal dir name)
+            (and (string-equal (expand-file-name dir) name)
                  (eq (tar-header-link-type tar-data) 5))
             (error "Package does not untar cleanly into directory %s/" dir)))))
   (tar-untar-buffer))
@@ -1192,8 +1192,12 @@ package-tar-file-info
   "Find package information for a tar file.
 The return result is a `package-desc'."
   (cl-assert (derived-mode-p 'tar-mode))
-  (let* ((dir-name (file-name-directory
-                    (tar-header-name (car tar-parse-info))))
+  (let* ((dir-name (named-let loop
+                       ((filename (tar-header-name (car tar-parse-info))))
+                     (let ((dirname (file-name-directory filename)))
+                       ;; The first file can be in a subdir: look for the top.
+                       (if dirname (loop (directory-file-name dirname))
+                         (file-name-as-directory filename)))))
          (desc-file (package--description-file dir-name))
          (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
     (unless tar-desc

bug#58367: `package-install-file' rejects some .tar files (tentative patch)

[Richard Hopkins]

On 2022-10-08 17:25, Stefan Monnier wrote:
> Thanks for the tarballs for the tests, they were very helpful.
> The patch also pointed in the right direction.
> 
> I installed the patch below (plus regression tests) into `master`.

Great, thanks for taking a look at this and also fixing the top level
lookup for v7-withsub-0.1.tar.  I've pulled the latest changes and all
the original tar files now install.

I've managed to get the ustar-withsub-0.1 to install from a directory
and include the sub directories by using `copy-directory' instead of
only copying the .el files in the top level.  As more inspiration...

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index d619142d64..4144a12718 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -947,14 +947,10 @@ package-unpack
           (pkg-dir (expand-file-name dirname package-user-dir)))
      (pcase (package-desc-kind pkg-desc)
        ('dir
-       (make-directory pkg-dir t)
         (let ((file-list
-              (directory-files
-               default-directory 'full "\\`[^.].*\\.el\\'" 'nosort)))
-         (dolist (source-file file-list)
-           (let ((target-el-file
-                  (expand-file-name (file-name-nondirectory 
source-file) pkg-dir)))
-             (copy-file source-file target-el-file t)))
+              (directory-files-recursively
+               default-directory "." t)))
+         (copy-directory default-directory pkg-dir nil t 
'copy-contents)
           ;; Now that the files have been installed, this package is
           ;; indistinguishable from a `tar' or a `single'. Let's make
           ;; things simple by ensuring we're one of them.