Add support for PAX Format, Version 1.0 by ncihnegn · Pull Request #298 · composefs/tar-rs

ncihnegn · 2022-08-12T09:27:24Z

ncihnegn · 2022-08-13T14:55:51Z

Windows CI is failing as #299.

NobodyXu · 2022-08-19T11:37:49Z

@ncihnegn Thank you very much for this PR!

NobodyXu · 2022-09-06T04:57:08Z

@alexcrichton Can you review this PR please?
It is really important for cargo-binstall as users of it encounters PAX format in practice and lack of PAX format means cargo-binstall gives errors even if a pre-built binary is available.

alexcrichton · 2022-09-12T14:59:39Z

-                let off = block.offset()?;
-                let len = block.length()?;
-                if len != 0 && (size - remaining) % 512 != 0 {
+            let mut add_block = |block: &SparseEntry| -> io::Result<_> {


I think it's ok to avoid a new SparseEntry type here and just take two parameters for offset/size

alexcrichton · 2022-09-12T15:01:03Z

-            Some(gnu) => gnu,
-            None => return Err(other("sparse entry type listed but not GNU header")),
-        };
+        let mut sparse_map = Vec::<SparseEntry>::new();


One of the main goals I tried to keep for the tar crate is to minimize internal allocations. Instead of having a temporary list here could this be refactored to avoid the intermediate allocation?

I don't think so. Here we need to convert strings to numbers and the number of pairs is not fixed.

alexcrichton · 2022-09-12T15:01:35Z

    }
 }

+#[allow(unused_assignments)] // https://github.com/rust-lang/rust/issues/22630


I think it would be best to remove this or move the comment to where the warning is printed instead.

alexcrichton · 2022-09-12T15:02:32Z

+            if is_recognized_header && fields.is_pax_sparse() {
+                gnu_longname = fields.pax_sparse_name();
+            }


This feels different than the current organization. Instead of pretending that the gnu_longname field was present if a pax-specified field is present could the accessor which looks at long_pathname be updated to consult the pax extensions if they're present?

alexcrichton · 2022-09-12T15:03:08Z

+                // Not an entry
+                // Keep pax_extensions for the next ustar header
+                processed -= 1;


I'm not sure what this is doing? An entry was consumed here so I don't know why this value would be decremented?

In PAX format, each entry has two headers: pax and ustar.

alexcrichton · 2022-09-12T15:05:30Z

+            let mut reader = io::BufReader::with_capacity(BLOCK_SIZE, &self.archive.inner);
+            let mut read_decimal_line = || -> io::Result<u64> {
+                let mut str = String::new();
+                num_bytes_read += reader.read_line(&mut str)?;
+                str.strip_suffix("\n")
+                    .and_then(|s| s.parse::<u64>().ok())
+                    .ok_or_else(|| other("failed to read a decimal line"))
+            };
+
+            let num_entries = read_decimal_line()?;
+            for _ in 0..num_entries {
+                let offset = read_decimal_line()?;
+                let size = read_decimal_line()?;
+                sparse_map.push(SparseEntry { offset, size });
+            }


I don't think I understand how this could work and pass tests. This is creating a temporary buffer to read from the inner underlying data stream but then the buffer is discarded outside of this scope. That means that more data than necessary could be consumed from the inner data stream and accidentally discarded.

I don't think that this should create a temporary buffer here but instead, if necessary, use a stack-local buffer and then do byte-searching within since presumably the entry here is typically small enough for that.

It will consume exactly a block of 512 bytes. After reading the necessary data, the remaining filler will be discarded.

alexcrichton · 2022-09-12T15:08:16Z

-            None => return Err(other("sparse entry type listed but not GNU header")),
-        };
+        let mut sparse_map = Vec::<SparseEntry>::new();
+        let mut real_size = 0;


Something about this doesn't feel quite right because real_size isn't set in all branches of the if statement below whereas prior it was always set to a particular value.

Not sure what you mean. It is set in both branches, line 428 and 452.

We could do though:
let real_size = if entry.is_pax_sparse() { ... } else { ... }

akesson · 2022-12-16T15:15:36Z

Did this go stale?

davidanthoff · 2024-06-05T00:59:31Z

Is there any chance that this PR could be revived? Or maybe it is ready to be merged, given that it looks like @ncihnegn responded to all the change requests?

Presumably the first step would be to resolve the merge conflicts, @ncihnegn is that something you could do?

ncihnegn · 2024-06-09T22:02:30Z

Updated.

ararslan · 2024-07-29T18:10:08Z

@alexcrichton, could you please re-review this? The Julia language's version multiplexer/installer uses this crate but we're blocked on porting the installer to all of Julia's supported platforms until this is merged.

alexcrichton · 2024-08-26T14:47:36Z

This is quite an old PR and I've unfortunately lost context on this. I'd also understand if @ncihnegn here wouldn't want to sheperd thing along after it's been 2 years since it's been opened. Despite that though this is somewhat tricky code and I'm hesitant to merge as-is. Merging this would require me to get a better understanding of PAX and how this crate works again (it's been awhile) so that's a fair bit of context to boot back up on. The test coverage also looks relatively light here and additionally there's not a ton of documentation internally about what's going on either.

There's also pieces I don't fully understand like creating intermediate BufReader values which feel like they can easily go wrong but I haven't fully investigated them yet.

Overall I unfortunately do not have the time to myself personally help push this across the finish line, but I can try to outline what would make this easier to land if others are interested in helping to contribute.

cgwalters · 2024-10-24T19:21:51Z

As I understand things, #375 added support for GNU sparse, but this would be the PAX version?

ncihnegn · 2024-10-25T02:45:17Z

As I understand things, #375 added support for GNU sparse, but this would be the PAX version?

Yes. See
https://www.gnu.org/software/tar/manual/html_section/Sparse-Formats.html

s-m-e · 2025-02-20T09:36:25Z

@ncihnegn @alexcrichton Hi there - any chance this gets finished / merged?

I am working my way through adding PAX archive read/write support to a Rust app. If you guys need help, feel free to ping me. I'd love to move/remove this logic out of my app (and have it in a crate instead, ideally this one).

alexcrichton · 2025-02-22T00:57:02Z

I'm strapped pretty thin right now, but @cgwalters or @xzfc would one of y'all be up for helping to push this over the finish line?

cgwalters

I just did a mostly superficial pass so far, haven't looked at correctness of sparse handling (I did glance at the gnu tar docs a while ago and just recall it's a mess)

cgwalters · 2025-02-25T00:56:37Z


+#[test]
+fn pax_sparse() {
+    let rdr = Cursor::new(tar!("pax_sparse.tar"));


Post the xz fiasco let's be a bit more sensitive about committing binary data to git. Can you add the script that generates this at least? Or probably better honestly for tests, just assume we have a working external tar binary that can generate the relevant data.

cgwalters · 2025-02-25T01:00:00Z

-            None => return Err(other("sparse entry type listed but not GNU header")),
-        };
+        let mut sparse_map = Vec::<SparseEntry>::new();
+        let mut real_size = 0;


We could do though:
let real_size = if entry.is_pax_sparse() { ... } else { ... }

cgwalters · 2025-02-25T01:01:23Z

-            fields.long_linkname = gnu_longlink;
            fields.pax_extensions = pax_extensions;
+            // False positive: unused assignment
+            // https://github.com/rust-lang/rust/issues/22630


It looks like this has been fixed so we should be able to drop the assignment.

We don't have extra assignments here.

Can't we address this by doing:

fields.pax_extensions = pax_extensions.take();

?

cgwalters · 2025-02-25T01:01:39Z

    }
 }

+#[allow(unused_assignments)]


Hopefully we can drop this now

No, rustc 1.87.0-nightly still complains.

cgwalters · 2025-02-25T01:02:33Z

+    pub fn pax_sparse_name(&mut self) -> Option<Vec<u8>> {
+        if let Some(ref pax) = self.pax_extensions {
+            return PaxExtensions::new(pax)
+                .filter_map(|f| f.ok())


I'm not a big fan of "swallowing" errors like this, my preference would be to make this function return Result<Option<Vec<u8>>> and propagate this error instead.

I don't understand. To propagate the error using Result we will be dropping the Vec.

cgwalters · 2025-02-25T01:02:54Z


+/// Description of a spare entry.
+pub struct SparseEntry {
+    pub offset: u64,


Let's also document the fields please

Offset and size names are self explanatory.

That's definitely true, but I think there's a general principle here that everything pub should have a docstring, even if trivial.

In some other crates I maintain we use deny(missing_docs).

s-m-e · 2025-04-17T06:55:07Z

Ping.

s-m-e · 2025-05-07T12:01:59Z

Yet another ping - what's still left to do to get this one over the finish line? I've wrapped good old GNU tar in my app, and its about as horrible as one might expect. I'd rather help to get this PR done.

yjqg6666 · 2025-08-22T00:54:45Z

Any update for this PR?

ararslan · 2025-09-11T16:48:27Z

@cgwalters, are you able to take another look at some of the replies to and changes since your last review? @ncihnegn, I believe some of the comments from the initial review have not yet been addressed, e.g. the ones about documentation.

cgwalters · 2025-10-02T17:34:23Z

OK I rebased this, dropped the prebuilt binary from the tests and instead significantly expanded the tests (assisted by Claude): https://github.com/cgwalters/tar-rs/tree/pax - any objections to replacing this PR with the contents of that branch?

cgwalters · 2025-10-08T20:28:57Z

I happened to see golang/go#75677 go by. A key bit looks like https://github.com/golang/go/blob/e1ca1de1234aa0f6be85c97db5492a94b099a305/src/archive/tar/format.go#L149

djc · 2026-03-17T09:24:22Z

@cgwalters since it seems like the PRs have been flowing recently, would it be feasible to get something like this merged? If the OP is unresponsive, it's probably fine to submit a new PR with your changes (ideally maintaining authorship of the OP here).

ncihnegn · 2026-03-18T18:48:55Z

I am following this thread and will do anything to get it merge. I don't mind if you choose to merge the PR from @cgwalters .

djc · 2026-03-18T19:17:51Z

OK I rebased this, dropped the prebuilt binary from the tests and instead significantly expanded the tests (assisted by Claude): https://github.com/cgwalters/tar-rs/tree/pax - any objections to replacing this PR with the contents of that branch?

Maybe look at incorporating these changes?

cgwalters · 2026-03-18T20:06:33Z

Hi sorry I meant to followup here, and I'll post more about this soon but basically I recently created https://github.com/composefs/tar-core and https://github.com/cgwalters/tar-rs/tree/main is PoC patches for rebasing this crate on it.

And I did verify that rebasing on tar-core fixes this issue for free. tar-core also has (made a bit easier by its strict sans-io architecture) metadata limits that fix #298 (comment)

Prove that the tar-core parser integration fixes issues #286 (incorrect file size for PAX sparse entries) and #295 (files extracted under GNUSparseFile.0/ instead of their real name). The test archive is from ncihnegn's PR #298, which this work obsoletes. Co-authored-by: Haowen Ning <ncihnegn@gmail.com> Assisted-by: OpenCode (Claude Opus 4) Signed-off-by: Colin Walters <walters@verbum.org>

cgwalters · 2026-03-20T13:11:32Z

Now up in #447

ncihnegn force-pushed the master branch from 05b9b13 to ad88beb Compare August 13, 2022 14:50

This was referenced Aug 19, 2022

Tracking of upstream bug in tar cargo-bins/cargo-binstall#177

Closed

Do not use gnu sparse extension please! cargo-bins/cargo-quickinstall#87

Closed

NobodyXu mentioned this pull request Aug 19, 2022

Unpacking file size is incorrect (with reproduced repo) #286

Open

Add support for PAX Format, Version 1.0

2f5ea4f

ncihnegn force-pushed the master branch from ad88beb to 2f5ea4f Compare August 22, 2022 05:39

NobodyXu mentioned this pull request Aug 29, 2022

Bug: Extracting tar failed due to GNU Sparse File extension not properly supported #295

Open

alexcrichton reviewed Sep 12, 2022

View reviewed changes

ncihnegn added 2 commits September 12, 2022 14:08

Addressing comments

95d44da

Update comments

3340fc2

kpcyrd mentioned this pull request Nov 25, 2022

Calling set_size writes wrong value #305

Open

Phault mentioned this pull request Mar 2, 2024

starbase_archive cannot extract tar.gz created using macos' tar moonrepo/starbase#56

Closed

ararslan mentioned this pull request Jun 6, 2024

FreeBSD support JuliaLang/juliaup#872

Merged

ncihnegn added 2 commits June 9, 2024 14:45

Merge branch 'main' into pax

23684c8

Fix typos

827a3a0

xzfc mentioned this pull request Aug 22, 2024

feat: Support adding sparse files to archives #375

Merged

zertosh mentioned this pull request Sep 11, 2024

[Bugfix] Switch tar crate to make tar.gz decompression work facebook/dotslash#28

Closed

Resolve conflict

d08f7df

xzfc mentioned this pull request Dec 9, 2024

Replace hard-coded 512 with BLOCK_SIZE #394

Merged

Merge branch 'main' into pax

16d4dbd

cgwalters requested changes Feb 25, 2025

View reviewed changes

ncihnegn added 2 commits February 28, 2025 00:53

Address comments

8ad8efd

Address comments

880f591

ncihnegn requested a review from cgwalters February 28, 2025 09:05

djc mentioned this pull request Mar 17, 2026

Test on FreeBSD is failing due to tar entry kind 'GNUSparse' is not supported rust-lang/rustup#4751

Open

Conversation

ncihnegn commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncihnegn commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NobodyXu commented Aug 19, 2022

Uh oh!

NobodyXu commented Sep 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncihnegn Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncihnegn Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesson commented Dec 16, 2022

Uh oh!

davidanthoff commented Jun 5, 2024

Uh oh!

ncihnegn commented Jun 9, 2024

Uh oh!

ararslan commented Jul 29, 2024

Uh oh!

alexcrichton commented Aug 26, 2024

Uh oh!

cgwalters commented Oct 24, 2024

Uh oh!

ncihnegn commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-m-e commented Feb 20, 2025

Uh oh!

alexcrichton commented Feb 22, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncihnegn commented Aug 12, 2022 •

edited

Loading

ncihnegn commented Aug 13, 2022 •

edited

Loading

ncihnegn Sep 12, 2022 •

edited

Loading

ncihnegn Sep 12, 2022 •

edited

Loading

ncihnegn commented Oct 25, 2024 •

edited

Loading