bpo-18819: tarfile: only set device fields for device files #18080

wchargin · 2020-01-20T18:08:02Z

The GNU docs describe the devmajor and devminor fields of the tar
header struct only in the context of character and block special files,
suggesting that in other cases they are not populated. Typical utilities
behave accordingly; this patch teaches tarfile to do the same.

Test Plan:
Unit tests added; they fail before this commit and pass after it. Also
verified that this enables output that is bit-for-bit compatible with
GNU tar. In particular, this program now passes on my Ubuntu 16.04,
whereas it failed before this patch:

import os
import subprocess
import tarfile
import tempfile

filename = "important_data"
contents = b"The quick brown fox jumps over the lazy dog"

with tempfile.TemporaryDirectory() as tmpdir:
    os.chdir(tmpdir)
    with open(filename, "wb") as outfile:
        outfile.write(contents)
    with tarfile.open("py.tar", "x", format=tarfile.GNU_FORMAT) as outfile:
        outfile.add(filename)
    subprocess.check_call(["tar", "cf", "gnu.tar", filename])
    subprocess.check_call(["cmp", "-b", "py.tar", "gnu.tar"])

(The exact contents of the files depend on the calling user and current
time, but should always be the same across both output archives.)

wchargin-branch: tarfile-limit-device-headers

https://bugs.python.org/issue18819


        bpo-18819: tarfile: only set device fields for device files

The GNU docs describe the `devmajor` and `devminor` fields of the tar header struct only in the context of character and block special files, suggesting that in other cases they are not populated. Typical utilities behave accordingly; this patch teaches `tarfile` to do the same. Test Plan: No tests added because none appear to exist for this module. Manually verified that this enables output that is bit-for-bit compatible with GNU tar. In particular, this program now passes on my Ubuntu 16.04, whereas it failed before this patch: ```python import os import subprocess import tarfile import tempfile filename = "important_data" contents = b"The quick brown fox jumps over the lazy dog" with tempfile.TemporaryDirectory() as tmpdir: os.chdir(tmpdir) with open(filename, "wb") as outfile: outfile.write(contents) with tarfile.open("py.tar", "x", format=tarfile.GNU_FORMAT) as outfile: outfile.add(filename) subprocess.check_call(["tar", "cf", "gnu.tar", filename]) subprocess.check_call(["shasum", "-a", "256", "py.tar", "gnu.tar"]) subprocess.check_call(["cmp", "-b", "py.tar", "gnu.tar"]) ``` (The exact hashes depend on the calling user and current time but should always be the same across both output archives.) wchargin-branch: tarfile-limit-device-headers

ethanfurman · 2020-01-22T02:12:47Z

The code looks good. Question, though: why use both cmp and shasum? If cmp passes aren't the files identical?

Also, the tests for tarfile are in Lib/test/test_tarfile.py. A quick perusal suggests that MemberReadTest might be a good place for your test above (appropriately modified, of course). Feel free to suggest a different class in that file, though.

ethanfurman

This looks good. Add your test and we should be ready to commit.

bedevere-bot · 2020-01-22T02:18:31Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

wchargin

Question, though: why use both cmp and shasum? If cmp passes aren't the files identical?

Yes; I had the shasum just as a sanity check that the files weren’t
empty, but you’re right that the cmp is certainly sufficient. I’ll go
ahead and remove this from the description to avoid confusion.

Also, the tests for tarfile are in Lib/test/test_tarfile.py.

Great, thanks. Not sure how I missed this.

Add your test and we should be ready to commit.

Will add a test to test_tarfile.py, probably more like the existing
tests in that file than like the end-to-end test in the description
(which would only work on systems with GNU tar installed).

Thanks for the speedy review! I’ll add a test and get back to you.


        review: add unit test

Test Plan: This passes as written, and fails if the previous commit is reverted. wchargin-branch: tarfile-limit-device-headers

wchargin · 2020-01-22T06:25:16Z

I’ve put the test in a new test class because it needs to run only in
the uncompressed case (or, we’d need special handling to decompress the
compressed archives, but I don’t see any benefit to doing that).

I have made the requested changes; please review again.

bedevere-bot · 2020-01-22T06:25:19Z

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

ethanfurman

Looking good. Nice tests.

Please add yourself to Misc/ACKS and we should be good to go. (Sorry for not noticing that earlier.)

bedevere-bot · 2020-01-23T02:32:15Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.


        review: add wchargin to Misc/ACKS

(Per request of Ethan Furman.) wchargin-branch: tarfile-limit-device-headers

wchargin · 2020-01-23T04:19:53Z

Done; thanks!

I have made the requested changes; please review again.

bedevere-bot · 2020-01-23T04:19:57Z

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

wchargin requested a review from ethanfurman as a code owner Jan 20, 2020

the-knights-who-say-ni added the CLA signed label Jan 20, 2020

bedevere-bot added the awaiting review label Jan 20, 2020

ethanfurman self-assigned this Jan 22, 2020

ethanfurman requested changes Jan 22, 2020

View changes

bedevere-bot removed the awaiting review label Jan 22, 2020

bedevere-bot added the awaiting changes label Jan 22, 2020

wchargin reviewed Jan 22, 2020

View changes

review: add unit test

Loading status checks…

9bc0431

Test Plan: This passes as written, and fails if the previous commit is reverted. wchargin-branch: tarfile-limit-device-headers

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 22, 2020

bedevere-bot requested a review from ethanfurman Jan 22, 2020

ethanfurman requested changes Jan 23, 2020

View changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 23, 2020

review: add wchargin to Misc/ACKS

Loading status checks…

d52cbc6

(Per request of Ethan Furman.) wchargin-branch: tarfile-limit-device-headers

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 23, 2020

bedevere-bot requested a review from ethanfurman Jan 23, 2020

ethanfurman approved these changes Jan 23, 2020

View changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 23, 2020

python / cpython

bpo-18819: tarfile: only set device fields for device files #18080

bpo-18819: tarfile: only set device fields for device files #18080

wchargin commented Jan 20, 2020 •

edited

This comment has been minimized.

ethanfurman commented Jan 22, 2020

ethanfurman left a comment

This comment has been minimized.

bedevere-bot commented Jan 22, 2020

wchargin left a comment

This comment has been minimized.

wchargin commented Jan 22, 2020

This comment has been minimized.

bedevere-bot commented Jan 22, 2020

ethanfurman left a comment

This comment has been minimized.

bedevere-bot commented Jan 23, 2020

This comment has been minimized.

wchargin commented Jan 23, 2020

This comment has been minimized.

bedevere-bot commented Jan 23, 2020

python / cpython

Sponsor python/cpython

Join GitHub today

bpo-18819: tarfile: only set device fields for device files #18080

bpo-18819: tarfile: only set device fields for device files #18080

Conversation

wchargin commented Jan 20, 2020 • edited

This comment has been minimized.

ethanfurman commented Jan 22, 2020

ethanfurman left a comment

This comment has been minimized.

bedevere-bot commented Jan 22, 2020

wchargin left a comment

This comment has been minimized.

wchargin commented Jan 22, 2020

This comment has been minimized.

bedevere-bot commented Jan 22, 2020

ethanfurman left a comment

This comment has been minimized.

bedevere-bot commented Jan 23, 2020

This comment has been minimized.

wchargin commented Jan 23, 2020

This comment has been minimized.

bedevere-bot commented Jan 23, 2020

wchargin commented Jan 20, 2020 •

edited