10000 bpo-44439: _ZipWriteFile.write() handle buffer protocol correctly · Pull Request #29468 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-44439: _ZipWriteFile.write() handle buffer protocol correctly #29468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from Mar 8, 2022
Merged

bpo-44439: _ZipWriteFile.write() handle buffer protocol correctly #29468

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f53ae11
1. _ZipWriteFile.write() handle buffer protocol correctly
wjssz Nov 8, 2021
d103eb0
2. zlib: fix thread lock may go wrong in rare cases
wjssz Nov 8, 2021
d6d5548
Revert "2. zlib: fix thread lock may go wrong in rare cases"
wjssz Nov 14, 2021
6ba267a
address comments
wjssz Mar 7, 2022
bdb8487
add a newline to NEWS
wjssz Mar 7, 2022
451e24f
fix NEWS
wjssz Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Lib/test/test_zipfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import array
import contextlib
import importlib.util
import io
Expand Down Expand Up @@ -1718,6 +1719,14 @@ def test_non_existent_file_raises_OSError(self):
# quickly.
self.assertRaises(OSError, zipfile.ZipFile, TESTFN)

def test_issue44439(self):
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to move this test to AbstractWriterTests and test with different compressions.

q = array.array('Q', [1, 2, 3, 4, 5])
LENGTH = len(q) * q.itemsize
with zipfile.ZipFile(io.BytesIO(), 'w') as zip:
with zip.open('data', 'w') as data:
self.assertEqual(data.write(q), LENGTH)
self.assertEqual(data._file_size, LENGTH)
Copy link
Member

Choose a reason for hiding this comment

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

_file_size is a private attribute. It is better to use a public API in tests. For example read the content of the file.


def test_empty_file_raises_BadZipFile(self):
f = open(TESTFN, 'w', encoding='utf-8')
f.close()
Expand Down
9 changes: 8 additions & 1 deletion Lib/zipfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,8 +1147,15 @@ def writable(self):
def write(self, data):
if self.closed:
raise ValueError('I/O operation on closed file.')
nbytes = len(data)

# Accept any data that supports the buffer protocol
if isinstance(data, (bytes, bytearray)):
nbytes = len(data)
else:
data = memoryview(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's recommended to manually release a memoryview. Implementations other than CPython may use a garbage collector that doesn't immediately finalize unreferenced objects. Maybe it's simpler to always use a memoryview:

        # Accept any data that supports the buffer protocol
        with memoryview(data) as data:
            nbytes = data.nbytes
            self._file_size += nbytes
            self._crc = crc32(data, self._crc)
            if self._compressor:
                data = self._compressor.compress(data)
                self._compress_size += len(data)
            self._fileobj.write(data)
            return nbytes

Copy link
Author
@ghost ghost Nov 8, 2021 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

Good point.
There are some sites in stdlib have this problem, maybe they can be solved in another issue together.
In addition, I suspect if other Python implementations don't release the underlying buffer of memoryview, the program will run abnormally.

Let @serhiy-storchaka decide.

update: I ran a benchmark, the performances are no significant different.

Copy link
Member

Choose a reason for hiding this comment

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

@eryksun is right. But it may be simpler to merge the current solution and fix issues with releasing memoryviews in all places uniformly later.

nbytes = data.nbytes
self._file_size += nbytes

self._crc = crc32(data, self._crc)
if self._compressor:
data = self._compressor.compress(data)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2021-11-08-20-27-41.bpo-44439.I_8qro.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix in `_ZipWriteFile.write()` method, when the input data is an object that
Copy link
Member

Choose a reason for hiding this comment

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

_ZipWriteFile is a private class. It would be better to reword the NEWS entry in terms of the public API.

supports the buffer protocol, the file length may be wrong.
4 changes: 2 additions & 2 deletions Modules/zlibmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,12 +1269,12 @@ zlib_Decompress_flush_impl(compobject *self, PyTypeObject *cls,
return NULL;
}

ENTER_ZLIB(self);

if (PyObject_GetBuffer(self->unconsumed_tail, &data, PyBUF_SIMPLE) == -1) {
return NULL;
}

ENTER_ZLIB(self);

self->zst.next_in = data.buf;
ibuflen = data.len;

Expand Down
0