GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

5ec1cff · 2024-12-21T01:55:21Z

Issue: random access uncompressed unencrypted ZipExtFile #128131

bedevere-app · 2024-12-21T01:55:26Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

picnixz · 2024-12-21T01:56:27Z

Closing as a duplicate of #128132. Please do not copy PRs made by others. Thanks.

EDIT: It appears that this is either the same person or two people collaborating with each other. Anyway, please do not update two identical PRs. Thanks

5ec1cff · 2024-12-21T03:39:17Z

Closing as a duplicate of #128132. Please do not copy PRs made by others. Thanks.

EDIT: It appears that this is either the same person or two people collaborating with each other. Anyway, please do not update two identical PRs. Thanks

We discussed about this issue and decide that I submit the pr. vvb2060 has already closed his pr. Please reopen my pr, sorry for inconvenience.

picnixz

Can we have tests for that please?

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst

5ec1cff · 2024-12-21T14:42:53Z

Can we have tests for that please?

Added.

picnixz

I haven't looked at the actual implementation until the tests are (hopefully) simpler.

Lib/test/test_zipfile/test_core.py

…pPmNt.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

add unittest

Lib/test/test_zipfile/test_core.py

picnixz · 2024-12-22T12:46:53Z

Lib/test/test_zipfile/test_core.py

+                b = fp.read(100)
+                self.assertEqual(b, txt[:100])
+
+                # seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)


In this case, I would add something like self.assertLess(ZipExtFile.MIN_READ_SIZE, 5000) to be sure that we update the tests if needed.

EDIT: check this outside the "with".

Can we use ZipExtFile.MIN_READ_SIZE directly ?

There are other tests that modify this value to make it much smaller:

cpython/Lib/test/test_zipfile/test_core.py

Lines 2591 to 2602 in 831b6de

# Make sure that the second read after seeking back beyond

# _readbuffer returns the same content (ie. rewind to the start of

# the file to read forward to the required position).

old_read_size = fp.MIN_READ_SIZE

fp.MIN_READ_SIZE = 1

fp._readbuffer = b''

fp._offset = 0

fp.seek(0, os.SEEK_SET)

self.assertEqual(fp.tell(), 0)

fp.seek(bloc, os.SEEK_CUR)

self.assertEqual(fp.read(bloc_len), txt[bloc:bloc+bloc_len])

fp.MIN_READ_SIZE = old_read_size

You mean we can also modify this value?

Yep (but it's best to only change it on the instance so it doesn't leak to the rest of the tests :p )

Lib/test/test_zipfile/test_core.py

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst

picnixz

Please address this comment: https://github.com/python/cpython/pull/128143/files#r1894920324.

Lib/test/test_zipfile/test_core.py

danifus · 2024-12-22T23:43:12Z

Lib/zipfile/__init__.py

            # disable CRC checking after first seeking - it would be invalid
            self._expected_crc = None
            # seek actual file taking already buffered data into account
            read_offset -= len(self._readbuffer) - self._offset
            self._fileobj.seek(read_offset, os.SEEK_CUR)
            self._left -= read_offset
+            self._compress_left -= read_offset


This is a good catch that was a bug with read_offset > 0. Are you able to make a test for this too?

danifus

Sorry I missed these before

danifus · 2024-12-23T05:18:37Z

Lib/test/test_zipfile/test_core.py

+                d = sio.bytes_read - old_count
+                self.assertLessEqual(d, min_size)


I had to think about why we were doing this and a comment would here would help. Feel free to change this if this wasn't the intent of this test :)

Suggested change

d = sio.bytes_read - old_count

self.assertLessEqual(d, min_size)

# Test this optimized read hasn't rewound and read from the

# start of the file (as in the case of the unoptimized path)

d = sio.bytes_read - old_count

self.assertLessEqual(d, min_size)

danifus · 2024-12-23T05:19:31Z

Lib/test/test_zipfile/test_core.py

+                d = sio.bytes_read - old_count
+                self.assertLessEqual(d, min_size)


Same comment as above

Suggested change

d = sio.bytes_read - old_count

self.assertLessEqual(d, min_size)

# Test this optimized read hasn't rewound and read from the

# start of the file (as in the case of the unoptimized path)

d = sio.bytes_read - old_count

self.assertLessEqual(d, min_size)

danifus · 2024-12-23T05:37:56Z

Lib/test/test_zipfile/test_core.py

+        # 20000 bytes
+        txt = b'0123456789' * 2000
+
+        # seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)


Suggested change

# seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)

# seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)

# as `ZipExtFile._read2()` reads in blocks of this size and we need to

# seek out of the buffered data

Sorry, that comment wasn't very good. This one is better?

Suggested change

# seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)

# seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)

# as `ZipExtFile._read2()` reads in blocks of this size and this test relies

# on reads remaining within one of these blocks.

Just to clarify, it was my suggested change that was not very good. It didn’t describe what was happening in the code as well as it could :)

I think the first one is better

picnixz

The question is: should we treat this as a bug or a feature? it's half-half but I'm not a maintainer of this module so I don't really know. @jaraco WDYT?

picnixz · 2024-12-24T15:50:04Z

Lib/test/test_zipfile/test_core.py

+        # The seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)
+        # as `ZipExtFile._read2()` reads in blocks of this size and we need to
+        # seek out of the buffered data


Suggested change

# The seek length must be greater than ZipExtFile.MIN_READ_SIZE (4096)

# as `ZipExtFile._read2()` reads in blocks of this size and we need to

# seek out of the buffered data

# The seek length must be greater than ZipExtFile.MIN_READ_SIZE

# as `ZipExtFile._read2()` reads in blocks of this size and we

# need to seek out of the buffered data

picnixz · 2024-12-24T15:50:32Z

Lib/test/test_zipfile/test_core.py

+        self.assertGreaterEqual(10002, min_size)
+        self.assertGreaterEqual(5003, min_size)


Suggested change

self.assertGreaterEqual(10002, min_size)

self.assertGreaterEqual(5003, min_size)

self.assertGreaterEqual(10002, min_size) # for forward seek test

self.assertGreaterEqual(5003, min_size) # for backward seek test

picnixz · 2024-12-24T15:51:02Z

Lib/test/test_zipfile/test_core.py

+        self.assertGreaterEqual(5003, min_size)
+        # The read length must be less than MIN_READ_SIZE, since we assume that
+        # only 1 block is read in the test.
+        self.assertGreaterEqual(min_size, 100)


Suggested change

self.assertGreaterEqual(min_size, 100)

self.assertGreaterEqual(min_size, 100) # for read() calls

picnixz · 2024-12-24T15:52:24Z

Lib/test/test_zipfile/test_core.py

+                # backward seek
+                old_count = sio.bytes_read
+                fp.seek(-5003, os.SEEK_CUR)
+                self.assertEqual(fp.tell(), 5099)


Suggested change

self.assertEqual(fp.tell(), 5099)

self.assertEqual(fp.tell(), 5099) # 5099 = 10102 - 5003

picnixz · 2024-12-24T15:53:15Z

Lib/test/test_zipfile/test_core.py

+        from _pyio import BytesIO
+        class StatIO(BytesIO):
+            def __init__(self):
+                super().__init__()
+                self.bytes_read = 0
+
+            def read(self, size=-1):
+                bs = super().read(size)
+                self.bytes_read += len(bs)
+                return bs


This can be made a non-local class so that you can re-use it in future tests.

picnixz · 2024-12-24T15:53:33Z

Lib/test/test_zipfile/test_core.py

@@ -3447,6 +3447,71 @@ def test_too_short(self):
        self.assertEqual(
            b"zzz", zipfile._Extra.strip(b"zzz", (self.ZIP64_EXTRA,)))

+class StoredZipExtFileRandomReadTest(unittest.TestCase):


Separate classes by 2 blank lines instead of 1.

The PR addressed the requested changes.

bedevere-app bot mentioned this pull request Dec 21, 2024

random access uncompressed unencrypted ZipExtFile #128131

Open

bedevere-app bot added the awaiting review label Dec 21, 2024

picnixz closed this Dec 21, 2024

5ec1cff deleted the 5ec1cff-patch-1 branch December 21, 2024 01:59

5ec1cff restored the 5ec1cff-patch-1 branch December 21, 2024 03:19

picnixz reopened this Dec 21, 2024

picnixz reviewed Dec 21, 2024

View reviewed changes

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst Outdated Show resolved Hide resolved

5ec1cff changed the title ~~GH-128131: Support forward seek in uncompressed unencrypted ZipExtFile~~ GH-128131: Support fast forward seek in uncompressed unencrypted ZipExtFile Dec 21, 2024

picnixz reviewed Dec 21, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

5ec1cff and others added 5 commits December 22, 2024 11:18

Random access uncompressed unencrypted ZipExtFile

f18239d

📜🤖 Added by blurb_it.

f90340f

Update Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.Q…

5d23be6

…pPmNt.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Update test_core.py

1a64610

add unittest

update unittest

57cb51c

5ec1cff force-pushed the 5ec1cff-patch-1 branch from 2c34891 to 57cb51c Compare December 22, 2024 03:19

5ec1cff requested a review from picnixz December 22, 2024 04:23

picnixz reviewed Dec 22, 2024

View reviewed changes

update unittest

4d9cea4

5ec1cff changed the title ~~GH-128131: Support fast forward seek in uncompressed unencrypted ZipExtFile~~ GH-128131: Completely support random access of uncompressed unencrypted ZipExtFile Dec 22, 2024

picnixz reviewed Dec 22, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Show resolved Hide resolved

Misc/NEWS.d/next/Library/2024-12-21-03-20-12.gh-issue-128131.QpPmNt.rst Outdated Show resolved Hide resolved

update unittest

3d37f31

5ec1cff changed the title ~~GH-128131: Completely support random access of uncompressed unencrypted ZipExtFile~~ GH-128131: Completely support random access of uncompressed unencrypted files inside ZipFile Dec 22, 2024

5ec1cff changed the title ~~GH-128131: Completely support random access of uncompressed unencrypted files inside ZipFile~~ GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile Dec 22, 2024

5ec1cff requested a review from picnixz December 22, 2024 14:59

picnixz previously requested changes Dec 22, 2024

View reviewed changes

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

Lib/test/test_zipfile/test_core.py Outdated Show resolved Hide resolved

bedevere-app bot removed the awaiting review label Dec 22, 2024

bedevere-app bot added the awaiting core review label Dec 22, 2024

danifus reviewed Dec 22, 2024

View reviewed changes

update unittest & zipfile

67f05de

5ec1cff force-pushed the 5ec1cff-patch-1 branch from 692109a to 67f05de Compare December 23, 2024 02:14

5ec1cff requested review from picnixz and danifus December 23, 2024 02:39

danifus reviewed Dec 23, 2024

View reviewed changes

add comments

25f0a7e

5ec1cff requested a review from danifus December 24, 2024 06:17

picnixz reviewed Dec 24, 2024

View reviewed changes

bedevere-app bot added awaiting review and removed awaiting core review labels Dec 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

5ec1cff commented Dec 21, 2024 •

edited by bedevere-app bot

Loading

bedevere-app bot commented Dec 21, 2024

picnixz commented Dec 21, 2024 •

edited

Loading

5ec1cff commented Dec 21, 2024 •

edited

Loading

picnixz left a comment

5ec1cff commented Dec 21, 2024

picnixz left a comment

picnixz Dec 22, 2024 •

edited

Loading

5ec1cff Dec 23, 2024

danifus Dec 23, 2024

5ec1cff Dec 23, 2024

danifus Dec 23, 2024

picnixz left a comment

danifus Dec 22, 2024

5ec1cff Dec 23, 2024

danifus left a comment

danifus Dec 23, 2024

danifus Dec 23, 2024

danifus Dec 23, 2024

danifus Dec 23, 2024

danifus Dec 24, 2024

5ec1cff Dec 24, 2024

picnixz left a comment

picnixz Dec 24, 2024

picnixz Dec 24, 2024

picnixz Dec 24, 2024

picnixz Dec 24, 2024

picnixz Dec 24, 2024

picnixz Dec 24, 2024

	# Make sure that the second read after seeking back beyond
	# _readbuffer returns the same content (ie. rewind to the start of
	# the file to read forward to the required position).
	old_read_size = fp.MIN_READ_SIZE
	fp.MIN_READ_SIZE = 1
	fp._readbuffer = b''
	fp._offset = 0
	fp.seek(0, os.SEEK_SET)
	self.assertEqual(fp.tell(), 0)
	fp.seek(bloc, os.SEEK_CUR)
	self.assertEqual(fp.read(bloc_len), txt[bloc:bloc+bloc_len])
	fp.MIN_READ_SIZE = old_read_size

		d = sio.bytes_read - old_count
		self.assertLessEqual(d, min_size)

		self.assertGreaterEqual(10002, min_size)
		self.assertGreaterEqual(5003, min_size)

	self.assertGreaterEqual(min_size, 100)
	self.assertGreaterEqual(min_size, 100) # for read() calls

	self.assertEqual(fp.tell(), 5099)
	self.assertEqual(fp.tell(), 5099) # 5099 = 10102 - 5003

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

Are you sure you want to change the base?

GH-128131: Completely support random read access of uncompressed unencrypted files in ZipFile #128143

Conversation

5ec1cff commented Dec 21, 2024 • edited by bedevere-app bot Loading

bedevere-app bot commented Dec 21, 2024

picnixz commented Dec 21, 2024 • edited Loading

5ec1cff commented Dec 21, 2024 • edited Loading

picnixz left a comment

Choose a reason for hiding this comment

5ec1cff commented Dec 21, 2024

picnixz left a comment

Choose a reason for hiding this comment

picnixz Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danifus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5ec1cff commented Dec 21, 2024 •

edited by bedevere-app bot

Loading

picnixz commented Dec 21, 2024 •

edited

Loading

5ec1cff commented Dec 21, 2024 •

edited

Loading

picnixz Dec 22, 2024 •

edited

Loading