Skip to content

tar: make members with duplicate slashes reachable via ls/find/glob (#1947)#2063

Open
Sanjays2402 wants to merge 1 commit into
fsspec:masterfrom
Sanjays2402:fix/tar-duplicate-slashes
Open

tar: make members with duplicate slashes reachable via ls/find/glob (#1947)#2063
Sanjays2402 wants to merge 1 commit into
fsspec:masterfrom
Sanjays2402:fix/tar-duplicate-slashes

Conversation

@Sanjays2402

Copy link
Copy Markdown

Summary

Fixes #1947.

A tar member whose name contains redundant consecutive slashes (e.g. path/with/extra/slash//test.txt) was silently invisible to ls, find, glob and walk. As a result fsspec.open_files("tar://**/*::archive.tar") returned zero files for such archives, even though the file could be opened by its exact name.

Reproducer (from the issue)

import fsspec, tarfile

with tarfile.open("test_w_extraslash.tar", "w") as tar:
    tar.add("test.txt", arcname="path/with/extra/slash//test.txt")

of = fsspec.open_files("tar://**/*::test_w_extraslash.tar")
# before: <List of 0 OpenFile instances>
# after:  <List of 1 OpenFile instances>

Root cause

The raw member name (including the //) was used both as the self.index key and to derive the directory structure in _get_dirs. AbstractFileSystem._parent("a/b//c.txt") returns "a/b/"with a trailing slash — because rsplit("/", 1) splits on the second slash of the //. So:

  • the inferred parent-directory key became path/with/extra/slash/ (trailing slash), which never matched a listing of path/with/extra/slash, and
  • the member's own computed root carried the same trailing slash.

The entry therefore fell out of every directory-traversal comparison in AbstractArchiveFileSystem and was unreachable through ls/find/glob/walk — while still openable by its exact name (which is why direct access appeared to work).

Fix

Normalise the filesystem-facing name by collapsing runs of slashes (new _collapse_slashes helper) when building both self.index and dir_cache, while retaining the original member name so tar.extractfile() still receives the exact archive key. _open normalises its lookup path too, so a member is openable by either:

  • its normalised name (path/with/extra/slash/test.txt, as now returned by find/glob), or
  • its original name (path/with/extra/slash//test.txt) — backwards compatible.

This matches the approach the maintainer agreed with in the issue thread ("normalizing the look-up name, while keeping the original tar key for the actual extraction").

Before / after

call before after
fs.find("/") [] ['path/with/extra/slash/test.txt']
fs.glob("**/*") ['path', 'path/with', 'path/with/extra'] [..., 'path/with/extra/slash', 'path/with/extra/slash/test.txt']
fs.ls("path/with/extra/slash") [] ['path/with/extra/slash/test.txt']
open_files("tar://**/*::…") 0 files 1 file
fs.open(".../slash//test.txt") works works (unchanged)

Normal single-slash archives are unaffected.

Tests

Adds test_ls_with_duplicate_slashes, parametrized over all four tar compressions (tar, tar.gz, tar.bz2, tar.xz), asserting discoverability via find/glob/ls, directory inference via isdir, and openability by both name forms.

Proven to guard the bug (source stashed, test kept):

# without the fix
FAILED fsspec/implementations/tests/test_tar.py::test_ls_with_duplicate_slashes[tar]
FAILED ...[tar-gz]  FAILED ...[tar-bz2]  FAILED ...[tar-xz]
4 failed
# with the fix
4 passed

Full local verification (py3.13):

  • test_tar.py, test_archive.py, test_zip.py + memory/local/api: 328 passed, 32 skipped, 1 xfailed, no regressions.
  • ruff check and ruff format --check clean on both changed files.

Diff is +76/−6 across fsspec/implementations/tar.py and its test module.

A tar member whose name contains redundant consecutive slashes
(e.g. "path/with/extra/slash//test.txt") was silently invisible to
`ls`, `find`, `glob` and `walk`, so `open_files("tar://**/*::archive.tar")`
returned nothing for such archives even though the file could be opened by
its exact name.

Root cause: the raw member name (including the "//") was used both as the
index key and to derive the directory structure. `_parent()` of
"a/b//c.txt" is "a/b/" (with a trailing slash), so the inferred parent
directory key never matched a listing of "a/b", and the member's own
computed root likewise carried the trailing slash. The entry therefore fell
out of every directory-traversal comparison in the archive base class.

Fix: normalise the filesystem-facing name by collapsing runs of slashes
(new `_collapse_slashes` helper) when building both `self.index` and the
`dir_cache`, while retaining the original member name so extraction still
works. `_open` normalises its lookup path too, so the file can be opened by
either its normalised name (as now returned by find/glob) or its original
name (backwards compatible).

Adds test_ls_with_duplicate_slashes (parametrized over all four tar
compressions), which fails without the fix and passes with it.

Fixes fsspec#1947
Comment on lines +15 to +25
def _collapse_slashes(name):
"""Collapse runs of consecutive slashes in a tar member name.

Some tar archives contain members with redundant duplicate slashes
(e.g. ``"a/b//c.txt"``). Left as-is these break the derived directory
structure: ``_parent("a/b//c.txt")`` yields ``"a/b/"`` (trailing slash),
so the entry becomes unreachable through ``ls``/``find``/``glob``/``walk``
even though it can be opened by its exact name. Normalising the
filesystem-facing name keeps ``"a/b//c.txt"`` reachable as ``"a/b/c.txt"``.
"""
return re.sub("/+", "/", name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just inline this with a one-line comment

dirname: {"name": dirname, "size": 0, "type": "directory"}
for dirname in self._all_dirnames(self.tar.getnames())
for dirname in self._all_dirnames(
[_collapse_slashes(name) for name in self.tar.getnames()]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tar.getnames and the normalisation happens twice here, we should probably only do it once.

Comment on lines +114 to +117
orig_name = ti.get_info()["name"].rstrip("/")
name = _collapse_slashes(orig_name)
info["name"] = name
out[name] = (info, ti.offset_data, orig_name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is also iterating the same information as in _get_dirs(), please see if it could be simplified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TarFileSystem: using wildcards together with // in tar

2 participants