Repository navigation
Fix wds2idx offsets of members with GNU long names - #6547
Conversation
`tar --list --block-num` reports the block of the first header of an archive member. In the GNU tar format, a member whose name does not fit in the 100-byte name field of the header is preceded by a `././@LongLink` header holding the name, and GNU tar reports the block of that header. wds2idx assumed the reported block is the member header itself, so for such members the index pointed at the stored name instead of the file data and fn.readers.webdataset returned the wrong bytes. Index inference inside the reader is not affected. Skip the GNU long name and long link headers when computing the data offset in the GNU tar path of wds2idx. Signed-off-by: Max Freedom Pollard <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
 [Medium impact] The change reaches GNU-tar index generation and indexed WebDataset reads. The error messages must meet the repository’s explicit requirements before merging. Findings
|
| header = archive.read(self.tar_block_size) | ||
| offset += self.tar_block_size | ||
| # the type flag is stored in the byte 156 of a tar header | ||
| if header[156:157] not in (tarfile.GNUTYPE_LONGNAME, tarfile.GNUTYPE_LONGLINK): |
There was a problem hiding this comment.
[Nit] GNUTYPE_LONGLINK can't precede an entry that reaches this code: _get_data_tar only passes regular files (entry_type == b"-"), and a long link target is only stored for symlinks/hardlinks. Harmless to keep for robustness, but then it's untested - the new tests only exercise GNUTYPE_LONGNAME. Either drop it or add a short comment saying it's defensive.
| return offset | ||
| # skip the blocks holding the long name | ||
| size = tarfile.TarInfo.frombuf(header, tarfile.ENCODING, "surrogateescape").size | ||
| offset += (size + self.tar_block_size - 1) // self.tar_block_size * self.tar_block_size |
There was a problem hiding this comment.
[Major] This loop can spin forever on a crafted archive. TarInfo.frombuf accepts a negative base-256 size (Python 3.10 parses a size field of -512 as -512), so offset advances by tar_block_size and then goes back by 512: net zero, and the same header is read again on every iteration. main doesn't have this problem for such an archive (it just produces a garbage index), so this is a regression; shards are often downloaded from third parties, so a hung indexer is worth guarding against.
Suggestion: reject a negative size (raise a clear error), and/or require the offset to strictly increase; optionally also stop when offset reaches the file size, so a truncated archive can't make it read past EOF either. A small test with a hand-crafted L header with a negative size would pin this down.
TarInfo.frombuf accepts a negative base-256 size, so a crafted `././@LongLink` header with a size of -512 moved the offset back to the same header and _get_data_offset looped forever. Raise a ValueError for a negative size, and for a header cut short by the end of the archive, so every iteration either moves forward by at least one block or stops. Add tests with hand-crafted long name headers for both cases, and note that skipping a long link header is defensive, since only links, which are filtered out before, have one. Signed-off-by: Max Freedom Pollard <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| archive.seek(offset) | ||
| header = archive.read(self.tar_block_size) | ||
| if len(header) < self.tar_block_size: | ||
| raise ValueError(f"Truncated tar header at offset {offset} in {self.uri}") |
There was a problem hiding this comment.
[Style] The new errors in _get_data_offset omit details required by the repository’s error-message rules. The truncated-header error does not show expected versus actual byte counts. Neither error ends with a period or tells users how to fix the damaged archive.
Include the byte counts, state that sizes must be nonnegative, and suggest replacing or rebuilding the archive.
Rule Used: Error messages must name the affected parameter/operator, show expected vs. actual values, and end as a complete sentence. Not "is 5" but "got 5 channels, expected 1 or 3.". Use make_string(...) for C++ concatenation; f-strings in Python. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks, good catch. A negative long name size now raises a ValueError, and so does a header cut short by the end of the archive, so every pass either moves forward at least one block or stops. Two tests with a hand-crafted L header cover both cases. I kept the GNUTYPE_LONGLINK skip with a comment that it is defensive, since GNU tar also counts K blocks when it prints the block number. |
|
!build |
|
CI MESSAGE: [77269466]: BUILD STARTED |
| "sample/" + "a" * 150 + ".jpg": b"\xff\xd8\xff\xe0first payload", | ||
| "short.cls": b"1", | ||
| "b" * 600 + ".jpg": b"\xff\xd8\xff\xe0second payload", |
There was a problem hiding this comment.
Nit: those fake JPEG magics don't help much here - tbh, I see no reason to even have JPEG files here in the first place. If we're only testing the script, why not have text files with easy to inspect payloads?
mzient
left a comment
mzient
left a comment
There was a problem hiding this comment.
OK, with a slight remark about the "JPEG" files in the test archive.
|
CI MESSAGE: [77269466]: BUILD PASSED |
|
@MaxFreedomPollard thank you for your contribution. |
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
The GNU tar path of
tools/wds2idx.pycomputes the data offset of a member as(block + 1) * 512, whereblockcomes fromtar --list --block-num. GNU tar prints the block of the first header of a member. In the GNU format, a name that does not fit in the 100-byte name field is stored in a././@LongLinkheader, followed by the name, placed before the member header, and that is the block GNU tar prints:simple_print_header()in GNU tar'ssrc/list.csubtractsrecent_long_name_blocksandrecent_long_link_blocksfrom the block of the member header (the code is the same in tar 1.35).For such members the index points at the stored name instead of the file data, so
fn.readers.webdatasetwithindex_pathsreturns the name, the member header and the start of the payload. GNU is GNU tar's default format and was Python'starfiledefault before 3.8, sotar cfon files with long paths is enough to hit this. Three things are already correct: the index the reader infers itself (libtar opened withTAR_GNU), thetarfilefallback of wds2idx, and PAX archives. For PAX, GNU tar prints the block of the member header, which is what #4226 relied on.The fix reads the header at the printed block and skips GNU long name and long link headers before adding the header size. For a member without a long name this adds one 512-byte read and a type flag check, about 1.6 µs per member on a 100k-member archive.
TarInfo.frombufis only called to get the size of a long name header.Index of a GNU-format shard with a sample
images/<100 x>/000001.{jpg,cls}, before and after:Additional information:
Affected modules and functionalities:
tools/wds2idx.py: offsets in the GNU tar path (_get_data_tar), new_get_data_offsethelper.dali/test/python/reader/test_wds2idx.py: two new tests.Key points relevant for the review:
_get_data_offset(). Most of the diff in_get_data_taris the loop being re-indented underwith open(self.uri, "rb"), so it is easier to read with whitespace changes hidden.Tests:
The tests build an archive with names of 161 and 604 characters plus a short one, run
_get_data_tar()and read each member back at the returned offset. The GNU test fails onmain. The PAX test passes before and after and covers the case fixed in #4226.What I could not run: I am on macOS, where
taris bsdtar (no--block-num), with no GPU and no DALI build. So I rantest_wds2idxwith nose2 on Python 3.10, 3.12 and 3.13 against a small stand-intarscript. The script prints GNU tar's--list --block-numand--verbose --list --numeric-owneroutput, with block numbers computed as insrc/list.c. With it, the GNU test fails onmainand all four tests pass with the fix. For all members of a 100k-member GNU archive (10% with long names),_get_data_offset()matchesTarInfo.offset_data. black 26 and flake8 with the repo config are clean. The new tests have not run against a real GNU tar, and I did not run the L0 suites or the reader end to end.Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A
🤖 Generated with Claude Code