bug#19481: package.el: support .tar archives featuring a pax_global_header entry

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Ivan Shmakov]

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

Package:  emacs
Severity: minor

	As currently implemented, package.el signals an error trying to
	process a .tar archive which has a pax_global_header entry, –
	such as the archives produced with $ git archive --format=tar.

	Please thus consider the patch MIMEd.

	* lisp/emacs-lisp/package.el (package-untar-buffer): Ignore
	archive entries with link type being 55.
	(package-tar-file-info): Consider the second file name in the
	archive if the first one has no directory component.

	* lisp/tar-mode.el (tar-untar-buffer): Ignore archive entries
	with link type being 55 (pax global extended header.)

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 1981 bytes --]

--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -739,6 +739,8 @@ defun package-untar-buffer (dir)
     (dolist (tar-data tar-parse-info)
       (let ((name (expand-file-name (tar-header-name tar-data))))
 	(or (string-match regexp name)
+	    ;; Ignore the pax_global_header entry.
+	    (eq 55 (tar-header-link-type tar-data))
 	    ;; Tarballs created by some utilities don't list
 	    ;; directories with a trailing slash (Bug#13136).
 	    (and (string-equal dir name)
@@ -1245,8 +1247,10 @@ defun 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
+	  ;; Take care of pax_global_header, if any.
+	  (or (file-name-directory (tar-header-name (car  tar-parse-info)))
+	      (file-name-directory (tar-header-name (cadr tar-parse-info)))))
          (desc-file (package--description-file dir-name))
          (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
     (unless tar-desc
--- a/lisp/tar-mode.el
+++ b/lisp/tar-mode.el
@@ -479,10 +550,12 @@ defun tar-untar-buffer ()
             (message "Extracting %s" name)
             (if (and dir (not (file-exists-p dir)))
                 (make-directory dir t))
-            (unless (file-directory-p name)
-	      (let ((coding-system-for-write 'no-conversion))
-		(write-region start end name)))
-            (set-file-modes name (tar-header-mode descriptor))))))))
+	    ;; Ignore the pax_global_header entry.
+	    (unless (eq 55 (tar-header-link-type descriptor))
+	      (unless (or (file-directory-p name))
+		(let ((coding-system-for-write 'no-conversion))
+		  (write-region start end name)))
+	      (set-file-modes name (tar-header-mode descriptor)))))))))
 
 (defun tar-summarize-buffer ()
   "Parse the contents of the tar file in the current buffer."

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Thu, 01 Jan 2015 17:55:46 +0000
> 
> 	As currently implemented, package.el signals an error trying to
> 	process a .tar archive which has a pax_global_header entry, –
> 	such as the archives produced with $ git archive --format=tar.
> 
> 	Please thus consider the patch MIMEd.
> 
> 	* lisp/emacs-lisp/package.el (package-untar-buffer): Ignore
> 	archive entries with link type being 55.
> 	(package-tar-file-info): Consider the second file name in the
> 	archive if the first one has no directory component.
> 
> 	* lisp/tar-mode.el (tar-untar-buffer): Ignore archive entries
> 	with link type being 55 (pax global extended header.)

What about type 72?  (See tar-mode.el for the details.)

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Dmitry Gutov]

I think this patch should include a new test as well.

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Thu, 01 Jan 2015 17:55:46 +0000

[…]

 >> * lisp/emacs-lisp/package.el (package-untar-buffer): Ignore archive
 >> entries with link type being 55.
 >> (package-tar-file-info): Consider the second file name in the
 >> archive if the first one has no directory component.

 >> * lisp/tar-mode.el (tar-untar-buffer): Ignore archive entries with
 >> link type being 55 (pax global extended header.)

 > What about type 72?

	Frankly, – no idea; I don’t seem to recall seeing one so far.

	In order not to place such checks all over the code, we may
	want either a separate tar-header-service-p function for the
	tar-untar-buffer callers to use to decide whether the file will
	be extracted or not, /or/ an optional filter function argument
	to tar-untar-buffer for the /caller/ to specify which files to
	filter out.

 > (See tar-mode.el for the details.)

	It doesn’t seem to provide anything beyond the name of the type.

-- 
FSF associate member #7257  np. Helden — Apocalyptica   … 3013 B6A0 230E 334A

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Ivan Shmakov]

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

>>>>> Ivan Shmakov <ivan@siamics.net> writes:
>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Thu, 01 Jan 2015 17:55:46 +0000

[…]

 >>> * lisp/tar-mode.el (tar-untar-buffer): Ignore archive entries with
 >>> link type being 55 (pax global extended header.)

 >> What about type 72?

 > Frankly, – no idea; I don’t seem to recall seeing one so far.

 > In order not to place such checks all over the code, we may want
 > either a separate tar-header-service-p function for the
 > tar-untar-buffer callers to use to decide whether the file will be
 > extracted or not, /or/ an optional filter function argument to
 > tar-untar-buffer for the /caller/ to specify which files to filter
 > out.

	Please consider the revised patch MIMEd.

	* lisp/tar-mode.el (tar-header-extractable-p): New function.
	(tar-untar-buffer): Use it; or use the value of the new optional
	argument instead.

	* lisp/emacs-lisp/package.el (package-untar-buffer): Use
	tar-header-extractable-p.
	(package-tar-file-info): Consider the second file name in the
	archive if the first one has no directory component.

[…]

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/diff, Size: 3165 bytes --]

--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -757,6 +757,8 @@ defun package-untar-buffer (dir)
     (dolist (tar-data tar-parse-info)
       (let ((name (expand-file-name (tar-header-name tar-data))))
 	(or (string-match regexp name)
+	    ;; Ignore non-extractable entries (e. g., pax_global_header.)
+	    (not (tar-header-extractable-p tar-data))
 	    ;; Tarballs created by some utilities don't list
 	    ;; directories with a trailing slash (Bug#13136).
 	    (and (string-equal dir name)
@@ -1291,4 +1293,3 @@ defun 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
e-                    (tar-header-name (car tar-parse-info))))
+  (let* ((dir-name
+	  ;; Take care of pax_global_header, if any.
+	  (or (file-name-directory (tar-header-name (car  tar-parse-info)))
+	      (file-name-directory (tar-header-name (cadr tar-parse-info)))))
          (desc-file (package--description-file dir-name))
          (tar-desc (tar-get-file-descriptor (concat dir-name desc-file))))
     (unless tar-desc
--- a/lisp/tar-mode.el
+++ b/lisp/tar-mode.el
@@ -331,6 +331,17 @@
            (tar-roundup-512 size)
          0))))
 
+(defun tar-header-extractable-p (descriptor)
+  "Return non-nil if DESCRIPTOR refers to a file we can extract.
+Currently tar-mode only supports extracting regular files and (to a
+limited extent) directories.
+
+If DESCRIPTOR is an integer, it is handled as a link type."
+  (let ((type (if (integerp descriptor)
+		  descriptor
+		(tar-header-link-type descriptor))))
+    (memq '(nil 0 5) type)))
+
 (defun tar-parse-octal-integer (string &optional start end)
   (if (null start) (setq start 0))
   (if (null end) (setq end (length string)))
@@ -531,9 +542,17 @@
 		(concat (if (= type 1) " ==> " " --> ") link-name)
 	      ""))))
 
-(defun tar-untar-buffer ()
-  "Extract all archive members in the tar-file into the current directory."
+(defun tar-untar-buffer (&optional filter)
+  "Extract all archive members in the tar-file into the current directory.
+
+Optional FILTER is a function called with the Tar header (descriptor)
+as its only argument for each of archive members in turn.  Any given
+member will only be extracted if the function returns non-nil.
+
+If FILTER is not given or nil, use `tar-header-extractable-p'."
   (interactive)
+  (unless filter
+    (setq filter 'tar-header-extractable-p))
   ;; FIXME: make it work even if we're not in tar-mode.
   (let ((descriptors tar-parse-info))   ;Read the var in its buffer.
     (with-current-buffer
@@ -546,7 +565,8 @@ defun tar-untar-buffer ()
                       (file-name-directory name)))
                (start (tar-header-data-start descriptor))
                (end (+ start (tar-header-size descriptor))))
-          (unless (file-directory-p name)
+          (when (and (not (file-directory-p name))
+                     (funcall filter descriptor))
             (message "Extracting %s" name)
             (if (and dir (not (file-exists-p dir)))
                 (make-directory dir t))

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Wed, 28 Jan 2015 20:54:33 +0000
> 
> 	Please consider the revised patch MIMEd.
> 
> 	* lisp/tar-mode.el (tar-header-extractable-p): New function.
> 	(tar-untar-buffer): Use it; or use the value of the new optional
> 	argument instead.
> 
> 	* lisp/emacs-lisp/package.el (package-untar-buffer): Use
> 	tar-header-extractable-p.
> 	(package-tar-file-info): Consider the second file name in the
> 	archive if the first one has no directory component.

Thanks.

Can we have a test suite for this file, and in particular for these
changes?  Failing that, please describe how you tested these changes.

Otherwise, I have no comments, and no objections to commit this, once
we are sure it doesn't break anything.

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Wed, 28 Jan 2015 20:54:33 +0000

 >> Please consider the revised patch MIMEd.

 >> * lisp/tar-mode.el (tar-header-extractable-p): New function.
 >> (tar-untar-buffer): Use it; or use the value of the new optional
 >> argument instead.

 >> * lisp/emacs-lisp/package.el (package-untar-buffer): Use
 >> tar-header-extractable-p.
 >> (package-tar-file-info): Consider the second file name in the
 >> archive if the first one has no directory component.

 > Thanks.

 > Can we have a test suite for this file,

	Which of the two?  (Or three, to count the .tar file produced by
	$ git archive.)

 > and in particular for these changes?

	I hope to come with a suitable addition for
	test/automated/package-test.el shortly.  Or do you suggest
	adding a test suite for tar-mode as well?

	FTR, the two changes I suggest are not /inter/dependent, so I
	intend to push them as two separate commits.  (And I wonder if
	eit makes sense to now file a separate Severity: wishlist bug
	report for tar-header-extractable-p, so to provide a somewhat
	extended background for the feature.)

 > Failing that, please describe how you tested these changes.

	I’ve tested these using my MW package, like:

	• C-x b *new-buffer* RET;

	• M-x cd RET ~/devel/mw-el-2015/ RET;

	• C-u M-! git archive --prefix=mw-0.2/ --format=tar
	  HEAD 2> /dev/null RET;

	• M-x tar-mode RET;

	• M-x package-install-from-buffer RET.

 > Otherwise, I have no comments, and no objections to commit this, once
 > we are sure it doesn't break anything.

	ACK, thanks.

-- 
FSF associate member #7257  np. La vita — Roberto Ricioppo   … B6A0 230E 334A

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Sat, 31 Jan 2015 11:05:48 +0000
> 
>  >> * lisp/tar-mode.el (tar-header-extractable-p): New function.
>  >> (tar-untar-buffer): Use it; or use the value of the new optional
>  >> argument instead.
> 
>  >> * lisp/emacs-lisp/package.el (package-untar-buffer): Use
>  >> tar-header-extractable-p.
>  >> (package-tar-file-info): Consider the second file name in the
>  >> archive if the first one has no directory component.
> 
>  > Thanks.
> 
>  > Can we have a test suite for this file,
> 
> 	Which of the two?  (Or three, to count the .tar file produced by
> 	$ git archive.)

I meant tar-mode.el, and specifically where the new function is used.

> 	FTR, the two changes I suggest are not /inter/dependent, so I
> 	intend to push them as two separate commits.

Does the tar-header-extractable-p function make any sense without its
use in package.el?  If so, then yes, these should be 2 separate
commits.

>       (And I wonder if
> 	eit makes sense to now file a separate Severity: wishlist bug
> 	report for tar-header-extractable-p, so to provide a somewhat
> 	extended background for the feature.)

Not sure it's worth your while.

>  > Failing that, please describe how you tested these changes.
> 
> 	I’ve tested these using my MW package, like:
> 
> 	• C-x b *new-buffer* RET;
> 
> 	• M-x cd RET ~/devel/mw-el-2015/ RET;
> 
> 	• C-u M-! git archive --prefix=mw-0.2/ --format=tar
> 	  HEAD 2> /dev/null RET;
> 
> 	• M-x tar-mode RET;
> 
> 	• M-x package-install-from-buffer RET.

Did you try unpacking a "normal" tar archive, to see nothing is
broken?

bug#19481: new tar-header-extractable-p, to use in tar-untar-buffer

[Ivan Shmakov]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>> From: Ivan Shmakov  Date: Sat, 31 Jan 2015 11:05:48 +0000

[…]

 >>> Can we have a test suite for this file,

 >> Which of the two?  (Or three, to count the .tar file produced by
 >> $ git archive.)

 > I meant tar-mode.el, and specifically where the new function is used.

	ACK.  How do I provide a sample .tar file to use in the test?

 >> FTR, the two changes I suggest are not /inter/dependent, so I intend
 >> to push them as two separate commits.

 > Does the tar-header-extractable-p function make any sense without its
 > use in package.el?  If so, then yes, these should be 2 separate
 > commits.

	I believe that this function may indeed be useful to any
	third-party code which needs to examine a Tar file before
	passing it to tar-untar-buffer.  Also, the changes suggested
	make it easier to use the latter to extract only the members
	matching specific (i. e., defined by the caller) criteria.

[…]

-- 
FSF associate member #7257  np. Fly by Night — Rush     … 3013 B6A0 230E 334A

bug#19481: new tar-header-extractable-p, to use in tar-untar-buffer

[Eli Zaretskii]

> From: Ivan Shmakov <ivan@siamics.net>
> Date: Sat, 07 Feb 2015 20:10:41 +0000
> 
> >>>>> Eli Zaretskii <eliz@gnu.org> writes:
> >>>>> From: Ivan Shmakov  Date: Sat, 31 Jan 2015 11:05:48 +0000
> 
> […]
> 
>  >>> Can we have a test suite for this file,
> 
>  >> Which of the two?  (Or three, to count the .tar file produced by
>  >> $ git archive.)
> 
>  > I meant tar-mode.el, and specifically where the new function is used.
> 
> 	ACK.  How do I provide a sample .tar file to use in the test?

Just add it to the repository, under test/automated/.

bug#19481: new tar-header-extractable-p, to use in tar-untar-buffer

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ivan Shmakov <ivan@siamics.net>
>> Date: Sat, 07 Feb 2015 20:10:41 +0000
>> 
>> >>>>> Eli Zaretskii <eliz@gnu.org> writes:
>> >>>>> From: Ivan Shmakov  Date: Sat, 31 Jan 2015 11:05:48 +0000
>> 
>> […]
>> 
>>  >>> Can we have a test suite for this file,
>> 
>>  >> Which of the two?  (Or three, to count the .tar file produced by
>>  >> $ git archive.)
>> 
>>  > I meant tar-mode.el, and specifically where the new function is used.
>> 
>> 	ACK.  How do I provide a sample .tar file to use in the test?
>
> Just add it to the repository, under test/automated/.

From what I can see, this patch was not applied?  

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

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Lars Ingebrigtsen]

Ivan Shmakov <ivan@siamics.net> writes:

>  >> * lisp/emacs-lisp/package.el (package-untar-buffer): Use
>  >> tar-header-extractable-p.
>  >> (package-tar-file-info): Consider the second file name in the
>  >> archive if the first one has no directory component.
>
>  > Thanks.
>
>  > Can we have a test suite for this file,
>
> 	Which of the two?  (Or three, to count the .tar file produced by
> 	$ git archive.)
>
>  > and in particular for these changes?
>
> 	I hope to come with a suitable addition for
> 	test/automated/package-test.el shortly.  Or do you suggest
> 	adding a test suite for tar-mode as well?
>
> 	FTR, the two changes I suggest are not /inter/dependent, so I
> 	intend to push them as two separate commits.  (And I wonder if
> 	eit makes sense to now file a separate Severity: wishlist bug
> 	report for tar-header-extractable-p, so to provide a somewhat
> 	extended background for the feature.)

It doesn't look like these changes were applied -- are they still of
interest, or is the pax_global_header entry not supported any more?

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

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Stefan Monnier]

> It doesn't look like these changes were applied -- are they still of
> interest, or is the pax_global_header entry not supported any more?

Yes, they are definitely still relevant and should be applied.  I think
they've not been applied simply because we were waiting for the tests.

BTW, I do have some minor comment about the suggested patch: while
I think the `filter` argument makes sense as a new feature for
`tar-untar-buffer`, I'm not completely sure that exposing pax's internal
details to this filter is a good idea.  Part of the reason I'm not sure
is because I'm not sufficiently familiar with the content of these "type
55" entries.  But if it's used for extra metadata (or something like
that), then it should be ignored by default rather than only if you pass
an appropriate filter function.


        Stefan

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Lars Ingebrigtsen]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> It doesn't look like these changes were applied -- are they still of
>> interest, or is the pax_global_header entry not supported any more?
>
> Yes, they are definitely still relevant and should be applied.  I think
> they've not been applied simply because we were waiting for the tests.

I think this was fixed in a different way in 2016 by Noam:

(defun tar--describe-as-link (descriptor)
  (let ((link-p (tar-header-link-type descriptor)))
    (if link-p
	(cond ((eq link-p 5) "a directory")
              ((eq link-p 20) "a tar directory header")
              ((eq link-p 28) "a next has longname")
              ((eq link-p 29) "a multivolume-continuation")
              ((eq link-p 35) "a sparse entry")
              ((eq link-p 38) "a volume header")
              ((eq link-p 55) "a pax global extended header")
              ((eq link-p 72) "a pax extended header")
              (t "a link")))))

(defun tar--check-descriptor (descriptor)
  (let ((link-desc (tar--describe-as-link descriptor)))
    (when link-desc
      (error "This is %s, not a real file" link-desc))))

So tar-mode displays these headers like this:

 Hrw-rw-rw-    root/root         52 pax_global_header
 drwxrwxr-x    root/root          0 pax-test/
 -rw-rw-r--    root/root          5 pax-test/foo

Which isn't unreasonable...

For reference, I've included a tar file that has one of these headers
here.

So is this still something that should be applied?  If I read Ivan's
patch correctly, it will make tar-mode just ignore those pax headers,
which seems a bit heavy-handed...

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

[-- Attachment #2: pax-test.tar --]
[-- Type: application/x-tar, Size: 10240 bytes --]

bug#19481: package.el: support .tar archives featuring a pax_global_header entry

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think this was fixed in a different way in 2016 by Noam:

[...]

> So tar-mode displays these headers like this:
>
>  Hrw-rw-rw-    root/root         52 pax_global_header
>  drwxrwxr-x    root/root          0 pax-test/
>  -rw-rw-r--    root/root          5 pax-test/foo
>
> Which isn't unreasonable...
>
> For reference, I've included a tar file that has one of these headers
> here.
>
> So is this still something that should be applied?  If I read Ivan's
> patch correctly, it will make tar-mode just ignore those pax headers,
> which seems a bit heavy-handed...

There was no response to this, so I'm closing this bug report.  If
there is more to be fixed in this area, please respond and we'll reopen
the bug report.

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