GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45863: tarfile: don't zero out header fields unnecessarily by jmroot · Pull Request #29693 · python/cpython

Conversation

@jmroot

@jmroot jmroot commented Nov 22, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

https://bugs.python.org/issue45863

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jmroot
jmroot requested a review from ethanfurman as a code owner November 22, 2021 08:23
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 22, 2021
@bedevere-bot bedevere-bot added the awaiting review label Nov 22, 2021
@jmroot
jmroot force-pushed the tarfile-headers branch from b6a4c39 to 8aedc8a Compare November 22, 2021 09:27
@github-actions

github-actions Bot commented Dec 23, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Dec 23, 2021
@jmroot

jmroot commented Jan 13, 2022

Copy link
Copy Markdown
Contributor Author

Commenting to clear stale flag since this is still awaiting review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jmroot
jmroot force-pushed the tarfile-headers branch from 8aedc8a to 8e5afe3 Compare February 5, 2022 05:27
@jmroot

jmroot commented Feb 5, 2022

Copy link
Copy Markdown
Contributor Author

@vstinner Changes made to address concerns brought up in our conversation on IRC.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Feb 7, 2022
View reviewed changes
Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@jmroot
bpo-45863: tarfile: don't zero out header fields unnecessarily
0684c9a
Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
@jmroot
jmroot force-pushed the tarfile-headers branch from 8e5afe3 to 0684c9a Compare February 8, 2022 10:22
@jmroot
jmroot requested a review from vstinner February 8, 2022 10:27
vstinner
vstinner reviewed Feb 8, 2022
View reviewed changes
Comment thread Lib/test/test_tarfile.py
b'././@PaxHeader' + bytes(86) \
+ b'0000000\x000000000\x000000000\x0000000000020\x0000000000000\x00010205\x00 x' \
+ bytes(100) + b'ustar\x0000'+ bytes(247) \
+ b'16 mtime=1000.1\n' + bytes(496) + b'foo' + bytes(97) \

@vstinner vstinner Feb 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

info contains mtime rounded to an int, but in the binary header, mtime can be a floatting point number?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jmroot jmroot Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Correct. The mtime is rounded in the info after calling create_pax_header, and appears in the overall header twice: unrounded in the pax extended header (formatted as a decimal number in a UTF-8 string) and rounded in the ustar header (octal 1750, seen in the next line).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/tarfile.py

# The existing pax header has priority.
if needs_pax and name not in pax_headers:
pax_headers[name] = str(val)

@vstinner vstinner Feb 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Does PAX have a limit for the number of digits after the dot for a floating point number?

For example, mtime=1/7 is formatted as 0.14285714285714285. The number 0.142,857,142,857,142,85 has 17 digits after the dot. A resolution of 10^-17 seconds, whereas the most accurate clocks have a resolution of 1 ns (10^-9) on Linux.

$ echo Hello > document.txt
$ tar -H posix -cf archive.tar document.txt
$ hexdump -C archive.tar 
(...)00000200  33 30 20 6d 74 69 6d 65  3d 31 36 34 34 33 32 35  |30 mtime=1644325|
00000210  30 33 33 2e 31 38 32 37  34 34 32 32 38 0a 33 30  |033.182744228.30|
00000220  20 61 74 69 6d 65 3d 31  36 34 34 33 32 35 30 33  | atime=164432503|
00000230  33 2e 31 38 32 37 34 34  32 32 38 0a 33 30 20 63  |3.182744228.30 c|
00000240  74 69 6d 65 3d 31 36 34  34 33 32 35 30 33 33 2e  |time=1644325033.|
00000250  31 38 32 37 34 34 32 32  38 0a 00 00 00 00 00 00  |182744228.......|
00000260  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
(...)

$ python3
>>> import os
>>> st=os.stat("document.txt")
>>> st.st_atime_ns
1644325037796763404
>>> st.st_mtime_ns
1644325033182744228
>>> st.st_ctime_ns
1644325033182744228

I get:

  • mtime=1644325033.182744228
  • atime=1644325033.182744228
  • ctime=1644325033.182744228

9 digits after the dot for mtime, atime and ctime.

Maybe for these 9 header names, round to 9 digits if there are more than 9 digits?

Maybe it's ok to have more digits. I don't know.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jmroot jmroot Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The standard doesn't specify a maximum number of digits. It suggests to use enough digits to allow the timestamp to be reconstructed exactly (if extracting on a system with the same timestamp precision as the originating system). Most implementations use nanosecond precision.

Doing additional rounding of the value in the pax extended header would be a behavioural change beyond what I'm setting out to do here. I suspect any potential issues with using more digits than an extractor can handle would be rare anyway, since you would have to deliberately use a made-up number rather than just dividing st_mtime_ns by 10^9.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_tarfile.py
# values that should be kept
t = tarfile.TarInfo()
t.name = "foo"
t.mtime = 1000.1

@vstinner vstinner Feb 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

1000.1 cannot be represented exactly in binary IEEE 754:

>>> (1001.1).hex()
'0x1.f48cccccccccdp+9'

Maybe to avoid any rounding issue in the test, we could use 1000.5 number which can be represented exaclty?

>>> (1000.5).hex()
'0x1.f440000000000p+9'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jmroot jmroot Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I can make that change if you want, but there are going to be quite a few real-world timestamps that don't have an exact representation in floating point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@jmroot jmroot Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

And come to think of it, 1000.5 has the disadvantage of being exactly halfway between two integers, so the result of rounding it changes depending on the prevailing rounding strategy. I think round() is supposed to guarantee round-to-even, but it still adds a little ambiguity for humans.

All in all, switching to fixed-point would probably be a good idea. But that's again rather out of scope for what I'm trying to accomplish here. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner

vstinner commented Feb 9, 2022

Copy link
Copy Markdown
Member

Can you please add a NEWS entry for this change? See for blurb or blurb-it.

With no NEWS, I'm ok to merge the change in main, but not to backport it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jmroot

jmroot commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Happy to write a NEWS entry, but TBH I was put off by the tool dependencies apparently needed to generate the required filename.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jmroot

jmroot commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

OK, I see now that blurb-it is an option, which is at least a little more convenient than installing blurb locally.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@blurb-it
📜🤖 Added by blurb_it.
e233e91
@jmroot

jmroot commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Sorry for manually squashing and force pushing BTW, I initially missed the part in the docs where it says not to.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jmroot

jmroot commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

@vstinner News entry added and CI has passed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit bf2d44f into python:main Feb 9, 2022
@bedevere-bot bedevere-bot removed the awaiting review label Feb 9, 2022
@vstinner vstinner added needs backport to 3.9 labels Feb 9, 2022
@miss-islington

miss-islington commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

Thanks @jmroot for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

Thanks @jmroot for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Feb 9, 2022

Copy link
Copy Markdown

GH-31232 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Feb 9, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 9, 2022
@jmroot @miss-islington
bpo-45863: tarfile: don't zero out header fields unnecessarily (pytho…
26456af
…nGH-29693)

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
(cherry picked from commit bf2d44f)

Co-authored-by: Joshua Root <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 9, 2022
@jmroot @miss-islington
bpo-45863: tarfile: don't zero out header fields unnecessarily (pytho…
9b186e7
…nGH-29693)

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
(cherry picked from commit bf2d44f)

Co-authored-by: Joshua Root <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Feb 9, 2022
@bedevere-bot

bedevere-bot commented Feb 9, 2022

Copy link
Copy Markdown

GH-31233 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington added a commit that referenced this pull request Feb 9, 2022
@miss-islington @jmroot
bpo-45863: tarfile: don't zero out header fields unnecessarily (GH-29693
42f87d4
)

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
(cherry picked from commit bf2d44f)

Co-authored-by: Joshua Root <[email protected]>
miss-islington added a commit that referenced this pull request Feb 9, 2022
@miss-islington @jmroot
bpo-45863: tarfile: don't zero out header fields unnecessarily (GH-29693
b0517a1
)

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
(cherry picked from commit bf2d44f)

Co-authored-by: Joshua Root <[email protected]>
@jmroot
jmroot deleted the tarfile-headers branch February 9, 2022 21:36
@jmroot

jmroot commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Feb 10, 2022
Fix sphinx-lint after #31097 and b878b3a #31248
Merged
Include Newline at end of NEWS entry python/blurb_it#256
Closed
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
@miss-islington @jmroot @hello-adam
bpo-45863: tarfile: don't zero out header fields unnecessarily (pytho…
1afb1a8
…nGH-29693)

Numeric fields of type float, notably mtime, can't be represented
exactly in the ustar header, so the pax header is used. But it is
helpful to set them to the nearest int (i.e. second rather than
nanosecond precision mtimes) in the ustar header as well, for the
benefit of unarchivers that don't understand the pax header.

Add test for tarfile.TarInfo.create_pax_header to confirm correct
behaviour.
(cherry picked from commit bf2d44f)

Co-authored-by: Joshua Root <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@jmroot @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.