Repository navigation
import/export-tar: harden the PAX header handling - #10589
Open
ThomasWaldmann wants to merge 7 commits into
Open
ThomasWaldmann wants to merge 7 commits into
ThomasWaldmann wants to merge 7 commits into
Conversation
A PAX timestamp header with a huge exponent made import-tar crash or hang: - "1e3000000": Decimal.scaleb() raises decimal.Overflow, which is an ArithmeticError, but not an OverflowError, so it was not caught. - "1e999999": int() of the scaled Decimal builds an integer with a million digits before safe_ns() clamps it, taking ~15s per header. Now the Decimal is clamped to [0, MAX_S] before the int conversion (same result as safe_ns), NaN/Infinity are rejected and all decimal exceptions are caught. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A pax header keyword must be valid UTF-8: the tarfile module encodes it strictly, only the value may be arbitrary bytes (hdrcharset=BINARY). Linux xattr names are arbitrary bytes, so export-tar --tar-format=PAX crashed with a UnicodeEncodeError on such an xattr. Now the xattr is skipped with a warning (rc 1). With the BORG format, it is skipped silently, because BORG.item.meta has it anyway. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
tar_acl_to_borg() passed entries it did not understand through unchanged. An entry with too few fields (e.g. "user:foo") was stored and made borg extract crash on Linux: acl_use_local_uid_gid() indexes fields[3], raising an IndexError, which is not an OSError and thus not turned into a per-file warning. Now tar_acl_to_borg() raises ValueError for an entry with an unknown tag or a wrong number of fields and import-tar ignores such an ACL header with a warning. TarfileObjectProcessors got a warning callback for that. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The tarfile module parses the pax mtime header with float() and does not check the result, so a header like "1e999" or "nan" gave a float inf or nan, and int() of that raised OverflowError or ValueError in create_helper(). Now such a value falls back to 0 (a valid pax mtime still overrides it via pax_time_to_ns, which clamps "1e999" to MAX_NS). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
When a named ACL entry from a tar PAX header has no numeric id (GNU tar writes them that way) and the name is unknown locally, the name is stored in place of the id, like borg create does. Unlike with borg create, where the name always comes from the same host, this makes every later borg extract of that file fail (also with --numeric-ids, as the "stored id" is the name). Now import-tar warns once per unknown name, so the user learns about it when it can still be fixed (create the user/group, or use --noacls). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The --tar-format table only listed what PAX adds over GNU; the items' BSD flags, NFSv4 ACLs (FreeBSD), extended ACLs (macOS), birthtime and content digests are not exported with either format. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The test created the archive without --atime, so the atime header was never produced and the only header checked was mtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10589 +/- ##
========================================
Coverage 89.35% 89.36%
========================================
Files 104 105 +1
Lines 20091 20368 +277
Branches 3154 3198 +44
========================================
+ Hits 17953 18202 +249
- Misses 1481 1507 +26
- Partials 657 659 +2 ☔ View full report in Codecov by Harness. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #10499 (and the 1.4 backport #10500), from a review of that code. One commit per issue:
pax_time_to_ns: crash and slowness on huge exponents.Decimal("1e3000000").scaleb(9)raisesdecimal.Overflow, anArithmeticErrorbut not anOverflowError, so it was not caught and import-tar crashed. In-range huge exponents like1e999999madeint()build a million-digit integer beforesafe_ns()clamped it (~15 s per header). Now the Decimal is clamped to[0, MAX_S]before the conversion (same result assafe_ns), NaN/Infinity are rejected and all decimal exceptions are caught.hdrcharset=BINARY), Linux xattr names are arbitrary bytes. Such an xattr is now skipped with a warning; silently with--tar-format=BORG, whereBORG.item.metahas it anyway.borg extracton Linux.tar_acl_to_borg()passed e.g.user:foothrough unchanged, andacl_use_local_uid_gid()then indexedfields[3]→IndexError, not anOSError, so no per-file warning but a traceback.tar_acl_to_borg()now raisesValueErrorfor an unknown tag or a wrong field count and import-tar ignores such a header with a warning (TarfileObjectProcessorsgot a warning callback for that).1e999ornancrashed import-tar. tarfile parses it withfloat()unchecked,int(inf)/int(nan)raised increate_helper(). Falls back to 0 now (a valid pax mtime still overrides it).--numeric-ids. import-tar now warns once per unknown name.--tar-formattable did not say what PAX/GNU still lose (BSD flags, NFSv4/extended ACLs, birthtime, content digests).test_roundtrip_pax_timestampsnever produced an atime header (no--atime); now checks all three.Tests for 1–5 added (a crafted-tar test for each crash). Run on macOS and in a Linux container (where the ACL and non-UTF-8 xattr tests actually execute).
Not done here:
acl_use_local_uid_gid()inplatform/linux.pyxitself still indexesfields[3]unguarded, and the BORG-format import path trustsBORG.item.metablindly (assert on the version, nomake_path_safe, tracebacks on bad base64/msgpack) — separate PRs.🤖 Generated with Claude Code