bpo-37144: Convert path-like object to regular path #13817

0x29a · 2019-06-04T16:49:49Z

Steps to reproduce the issue:

Create gzipped tar.

echo "test" > test
tar -cvzf test.tar.gz test

Try to pass path-like object in tarfile.open:

import tarfile
import pathlib
path = pathlib.Path('test.tar.gz')
tarfile.open(path, 'w|gz')

Result will be:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/tarfile.py", line 1599, in open
    stream = _Stream(name, filemode, comptype, fileobj, bufsize)
  File "/usr/lib/python3.6/tarfile.py", line 382, in __init__
    self._init_write_gz()
  File "/usr/lib/python3.6/tarfile.py", line 430, in _init_write_gz
    if self.name.endswith(".gz"):
AttributeError: 'PosixPath' object has no attribute 'endswith'

https://bugs.python.org/issue37144

tirkarthi · 2019-06-04T17:42:37Z

Lib/tarfile.py

@@ -349,7 +349,7 @@ def __init__(self, name, mode, comptype, fileobj, bufsize):
            fileobj = _StreamProxy(fileobj)
            comptype = fileobj.getcomptype()

-        self.name     = name or ""
+        self.name     = os.path.abspath(name) if name else ""


os.fspath looks like a good solution to me. Please add a test in Lib/test/test_tarfile.py that opens gzipname with mode 'w|gz' flag to make sure there is no AttributeError

@tirkarthi after opening gzipname with mode w|gz all remaining tests, that need to extract data from gzipname will not pass, because opening rewrites file content.

I added test to StreamWriteTest class, where I open for write tmpname, because it's designed for writing and insensitive to content change. Also, since this test inside of StreamWriteTest, opening by path-like filename will be performed for each mode (gz, lz, bz).

asvetlov · 2019-06-05T06:06:47Z

Lib/tarfile.py

@@ -1468,7 +1468,7 @@ def __init__(self, name=None, mode="r", fileobj=None, format=None,
            if hasattr(fileobj, "mode"):
                self._mode = fileobj.mode
            self._extfileobj = True
-        self.name = os.path.abspath(name) if name else None
+        self.name = os.fspath(name) if name else None


these lines are not equal.
The correct code should be something like:

name = os.fspath(name) self.name = os.path.abspath(name)

I've skipped the check for empty/None name.

asvetlov · 2019-06-05T06:07:15Z

Lib/tarfile.py

@@ -1943,7 +1943,7 @@ def add(self, name, arcname=None, recursive=True, *, filter=None):
            arcname = name

        # Skip if somebody tries to archive the archive...
-        if self.name is not None and os.path.abspath(name) == self.name:
+        if self.name is not None and os.fspath(name) == self.name:


bedevere-bot · 2019-06-05T06:07:24Z

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.

tirkarthi · 2019-06-05T11:37:35Z

Lib/test/test_tarfile.py

@@ -1421,6 +1421,11 @@ def test_file_mode(self):
        finally:
            os.umask(original_umask)

+    def test_open_by_path_object(self):
+        # Test for issue #37144: broken open for stream write by path-like object
+        tar = tarfile.open(pathlib.Path(tmpname), self.mode)


You can use a context manager that automatically calls close on the object.

with tarfile.open(pathlib.Path(tmpname), self.mode) as f: pass

0x29a · 2019-06-05T20:59:11Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-05T20:59:14Z

Thanks for making the requested changes!

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

asvetlov · 2019-06-05T22:06:01Z

Lib/test/test_tarfile.py

@@ -1421,6 +1421,11 @@ def test_file_mode(self):
        finally:
            os.umask(original_umask)

+    def test_open_by_path_object(self):
+        # Test for issue #37144: broken open for stream write by path-like object


Too long line as well.
Please break it into the two-lines comment.
The comment is valuable, thanks

bedevere-bot · 2019-06-05T22:06:12Z

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.

asvetlov · 2019-06-05T22:26:39Z

Lib/tarfile.py

@@ -1943,7 +1945,7 @@ def add(self, name, arcname=None, recursive=True, *, filter=None):
            arcname = name

        # Skip if somebody tries to archive the archive...
-        if self.name is not None and os.path.abspath(name) == self.name:
+        if self.name is not None and os.path.abspath(os.fspath(name)) == self.name:


Please move os.fspath(name) to the very beginning of the function, just after self._check(...).

0x29a · 2019-06-06T16:24:41Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-06T16:24:44Z

Thanks for making the requested changes!

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

asvetlov · 2019-06-07T11:47:37Z

Lib/tarfile.py

@@ -1468,6 +1468,8 @@ def __init__(self, name=None, mode="r", fileobj=None, format=None,
            if hasattr(fileobj, "mode"):
                self._mode = fileobj.mode
            self._extfileobj = True
+
+        name = os.fspath(name) if name else None


Move this line before if not fileobj: the code calls os.path.exists() and bltn_open() with name.

Both functions acceptpathlib.Path object but let's be consistent in Path -> str conversion, please

@asvetlov
Oh, finally I got It. Updated.

bedevere-bot · 2019-06-07T11:47:49Z

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.

asvetlov · 2019-06-07T15:59:26Z

Now looks much better!

After briefly looking in tarfile.py I see that gettarinfo(), extractall, extract, _getmember should convert their name and path arguments too.

mangrisano · 2019-06-07T23:02:46Z

Lib/test/test_tarfile.py

@@ -1368,7 +1368,7 @@ def write(self, data):

            f = BadFile()
            with self.assertRaises(exctype):
-                tar = tarfile.open(tmpname, self.mode, fileobj=f,
+                tarfile.open(tmpname, self.mode, fileobj=f,
                                   format=tarfile.PAX_FORMAT,
                                   pax_headers={'non': 'empty'})


These two arguments should be indented under tmpname.

you need to keep the same indent.

matrixise · 2019-09-13T13:12:56Z

Lib/tarfile.py

@@ -1468,6 +1469,7 @@ def __init__(self, name=None, mode="r", fileobj=None, format=None,
            if hasattr(fileobj, "mode"):
                self._mode = fileobj.mode
            self._extfileobj = True
+


remove this line, we have to get a minimalist diff for the merge. Thank you

matrixise · 2019-09-13T13:13:28Z

Lib/test/test_tarfile.py

@@ -1368,7 +1368,7 @@ def write(self, data):

            f = BadFile()
            with self.assertRaises(exctype):
-                tar = tarfile.open(tmpname, self.mode, fileobj=f,
+                tarfile.open(tmpname, self.mode, fileobj=f,
                                   format=tarfile.PAX_FORMAT,
                                   pax_headers={'non': 'empty'})


you need to keep the same indent.

matrixise · 2019-09-13T13:13:58Z

Lib/test/test_tarfile.py

@@ -1421,6 +1421,12 @@ def test_file_mode(self):
        finally:
            os.umask(original_umask)

+    def test_open_by_path_object(self):
+        # Test for issue #37144:


remove these comments.

csabella · 2020-01-26T00:05:42Z

@0x29a, please address the code reviews. Thank you!

Convert path-like object to regular path

7cfe617

the-knights-who-say-ni added the CLA signed label Jun 4, 2019

bedevere-bot added the awaiting review label Jun 4, 2019

📜🤖 Added by blurb_it.

a307164

tirkarthi reviewed Jun 4, 2019

View reviewed changes

0x29a added 4 commits June 4, 2019 21:13

Replace os.path.abspath with os.fspath and add test

9d34011

Change os.path.abspath on os.fspath

cd73f1e

Perform open check against temp tar file

cb85402

Remove unused variable

dd30c0d

0x29a force-pushed the fix-issue-37144 branch from cab805b to dd30c0d Compare June 5, 2019 01:48

asvetlov requested changes Jun 5, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jun 5, 2019

bedevere-bot added the awaiting changes label Jun 5, 2019

tirkarthi reviewed Jun 5, 2019

View reviewed changes

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 5, 2019

asvetlov requested changes Jun 5, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jun 5, 2019

asvetlov reviewed Jun 5, 2019

View reviewed changes

Perform os.path.abspath to result of os.fspath, use context manager

82882dd

0x29a force-pushed the fix-issue-37144 branch from 3f02bda to 82882dd Compare June 6, 2019 15:20

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 6, 2019

asvetlov requested changes Jun 7, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jun 7, 2019

Convert path object to raw path for all usages

14ae149

mangrisano reviewed Jun 7, 2019

View reviewed changes

matrixise requested changes Sep 13, 2019

View reviewed changes

DarkaMaul mannequin mentioned this pull request Apr 10, 2022

tarfile.open: improper handling of path-like object #81325

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

0x29a closed this Oct 1, 2023

0x29a deleted the fix-issue-37144 branch October 1, 2023 22:47

Uh oh!

bpo-37144: Convert path-like object to regular path #13817

bpo-37144: Convert path-like object to regular path #13817

Uh oh!

Conversation

0x29a commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x29a commented Jun 5, 2019

Uh oh!

bedevere-bot commented Jun 5, 2019

Uh oh!

asvetlov Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x29a commented Jun 6, 2019

Uh oh!

bedevere-bot commented Jun 6, 2019

Uh oh!

asvetlov Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 7, 2019

Uh oh!

asvetlov commented Jun 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented Jan 26, 2020

Uh oh!

Uh oh!

0x29a commented Jun 4, 2019 •

edited

Loading

asvetlov Jun 5, 2019 •

edited

Loading

asvetlov Jun 7, 2019 •

edited

Loading