[2.7] bpo-33038: gzip.GzipFile assumes non-None name attribute #6095

bbayles · 2018-03-13T01:49:30Z

In Python 2.7 gzip.GzipFile does hasattr(fileobj, 'name') to see if fileobj has a .name attribute, but then assumes that attribute is not None.

This isn't always a good assumption - for example, tempfile.SpooledTemporaryFile defines .name, but it can be None:
https://github.com/python/cpython/blob/2.7/Lib/tempfile.py#L597

Python 3's GzipFile doesn't have this issue, thankfully.

https://bugs.python.org/issue33038

serhiy-storchaka · 2018-05-03T07:30:10Z

Lib/gzip.py

@@ -95,7 +95,11 @@ def __init__(self, filename=None, mode=None,
        if filename is None:
            # Issue #13781: os.fdopen() creates a fileobj with a bogus name
            # attribute. Avoid saving this in the gzip header's filename field.
-            if hasattr(fileobj, 'name') and fileobj.name != '<fdopen>':
+            if (


I suggest to backport the code from 3.x (with adding a special case for '<fdopen>').

serhiy-storchaka · 2018-05-03T07:38:32Z

Misc/NEWS.d/next/Library/2018-03-10-20-14-36.bpo-33038.yA6CP5.rst

@@ -0,0 +1,2 @@
+Using a tempfile.SpooledTemporaryFile object with gzip.GzipFile no longer
+produces an AttributeError exception.


Add "Patch by yourname."

serhiy-storchaka · 2018-05-03T07:43:46Z

Lib/test/test_gzip.py

@@ -359,6 +360,13 @@ def test_read_with_extra(self):
        with gzip.GzipFile(fileobj=io.BytesIO(gzdata)) as f:
            self.assertEqual(f.read(), b'Test')

+    def test_fileobj_without_name(self):


I suggest to backport test_fileobj_from_fdopen from 3.x.

And add a similar test with fileobj=io.open(fd, 'wb') instead of fileobj=os.fdopen(fd, 'wb').

I suggest to backport test_fileobj_from_fdopen from 3.x.

I'm not sure I follow. The only difference between 2.7's test_fileobj_from_fdopen and 3.7's is the assert:
2.7
3.7

Could you clarify?

All right then.

serhiy-storchaka · 2018-05-05T11:29:49Z

Lib/gzip.py

-            else:
+            filename = getattr(fileobj, 'name', '')
+            if (
+                not isinstance(filename, (str, unicode)) or


basestring

serhiy-storchaka · 2018-05-05T11:35:05Z

Lib/gzip.py

-                filename = fileobj.name
-            else:
+            filename = getattr(fileobj, 'name', '')
+            if (


This formatting looks uncommon. I suggest to remove suspicious newlines. Could it fit in one line?

Even with basestring, it's one char too long to fit on a line. This format is in PEP8 (and flake8 and company are fine with it). I prefer it to this:

if (not isinstance(filename, basestring) or filename == '<fdopen>'): filename = ''

because it is hard to see from the next line. If you'd rather some other style that's fine, but there doesn't seem to be a precedent in this file?

I can write it in the line of length 79. This is in the PEP 8 limit. Even if it be out of the limit, PEP 8 suggests the following options:

No extra indentation.

if (not isinstance(filename, basestring) or filename == '<fdopen>'): filename = ''

Add a comment, which will provide some distinction in editors supporting syntax highlighting.

if (not isinstance(filename, basestring) or filename == '<fdopen>'): # comment filename = ''

Add some extra indentation on the conditional continuation line.

if (not isinstance(filename, basestring) or filename == '<fdopen>'): filename = ''

Yes, I'm familiar with the PEP. I got rid of the parens and put everything on one line.

serhiy-storchaka · 2018-05-05T11:41:02Z

Lib/test/test_gzip.py

@@ -359,6 +360,13 @@ def test_read_with_extra(self):
        with gzip.GzipFile(fileobj=io.BytesIO(gzdata)) as f:
            self.assertEqual(f.read(), b'Test')

+    def test_fileobj_without_name(self):


All right then.

serhiy-storchaka · 2018-05-05T11:42:15Z

Lib/test/test_gzip.py

+        # a .name attribute use a non-None value.
+        with tempfile.SpooledTemporaryFile() as f:
+            with gzip.GzipFile(fileobj=f, mode='wb') as archive:
+                archive.write(b'data')


Maybe add an assertion for name here?

serhiy-storchaka · 2018-05-05T11:44:48Z

Misc/NEWS.d/next/Library/2018-03-10-20-14-36.bpo-33038.yA6CP5.rst

@@ -0,0 +1,2 @@
+Using a tempfile.SpooledTemporaryFile object with gzip.GzipFile no longer


Mention also the case of io.open() with a file descriptor.

Hm, I don't think this is a new addition? That wasn't what the bug was aimed at - only for special file objects that (a) have a name attribute, but (b) don't set it.

This PR fixes a case when fileobj has a non-string name attribute. tempfile.SpooledTemporaryFile is one example. io.open() with a file descriptor is other example. Maybe there are other examples in the stdlib. And it can fix also cases with third-party file-like objects.

Now I understand - in that case the name attribute is an int.

serhiy-storchaka · 2018-05-05T11:46:51Z

Misc/NEWS.d/next/Library/2018-03-10-20-14-36.bpo-33038.yA6CP5.rst

@@ -0,0 +1,2 @@
+Using a tempfile.SpooledTemporaryFile object with gzip.GzipFile no longer
+produces an AttributeError exception. Patch by Bo Bayles.


I have noticed that your name is not in Misc/ACKS in this branch. Please add it.

bbayles added 2 commits March 10, 2018 20:15

bpo-33038: gzip.GzipFile assumes non-None name attribute

abb9ca0

Note issue in test

112b3ee

the-knights-who-say-ni added the CLA signed label Mar 13, 2018

bedevere-bot added the awaiting review label Mar 13, 2018

benjaminp force-pushed the 2.7 branch from 42b480c to 01a0fd4 Compare April 14, 2018 22:00

bbayles added 2 commits May 1, 2018 14:28

Merge branch '2.7' into gzip-fileobj-name

7be4b8d

Merge branch '2.7' into gzip-fileobj-name

dee6d1b

serhiy-storchaka reviewed May 3, 2018

View reviewed changes

bbayles added 3 commits May 3, 2018 21:27

Use Python 3-style workaround

639a871

Add additional test for io.open(fd)

865bfa3

Update NEWS item

7b4507e

serhiy-storchaka reviewed May 5, 2018

View reviewed changes

bbayles added 5 commits May 5, 2018 07:03

Add basestring

ec55e8b

Add additional assert in test

e74b9f8

Add to Misc/ACKS

8a259be

All on one line

006e588

Mention file descriptor case

7fac6c1

serhiy-storchaka approved these changes May 6, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels May 6, 2018

serhiy-storchaka added 2 commits May 6, 2018 15:33

Merge branch '2.7' into gzip-fileobj-name

f2793e9

Merge branch '2.7' into gzip-fileobj-name

b0f3fb1

serhiy-storchaka merged commit afe5f63 into python:2.7 May 9, 2018

bedevere-bot removed the awaiting merge label May 9, 2018

		@@ -0,0 +1,2 @@
		Using a tempfile.SpooledTemporaryFile object with gzip.GzipFile no longer
		produces an AttributeError exception.

		@@ -0,0 +1,2 @@
		Using a tempfile.SpooledTemporaryFile object with gzip.GzipFile no longer
		produces an AttributeError exception. Patch by Bo Bayles.

Uh oh!

[2.7] bpo-33038: gzip.GzipFile assumes non-None name attribute #6095

[2.7] bpo-33038: gzip.GzipFile assumes non-None name attribute #6095

Uh oh!

Conversation

bbayles commented Mar 13, 2018 • edited by bedevere-bot 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!

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!

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!

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!

Uh oh!

bbayles commented Mar 13, 2018 •

edited by bedevere-bot

Loading