gh-86094 Add support for Unicode Path Extra Field in ZipFile #102566

yeojin-dev · 2023-03-09T20:18:38Z

Issue: ZipFile does not supports Unicode Path Extra Field (0x7075) zip header field #86094

corona10 · 2023-03-10T01:39:58Z

Lib/zipfile/__init__.py

+                # Unicode Path Extra Field
+                try:
+                    up_version, up_name_crc = unpack('<BL', data[:5])
+                    if up_version == 1 and up_name_crc == self.orig_filename_crc:


Please check that it will be enough to calculate crc32 on the fly.

Suggested change

if up_version == 1 and up_name_crc == self.orig_filename_crc:

if up_version == 1 and up_name_crc == crc32(self.filename):

In my opinion, orig_filename_crc is necessary. filename in ZipInfo is the decoded value from ZipFile. But orig_filename_crc is the result of the CRC on filename before decoding. So it should be stored separately.

@yeojin-dev

Okay, how about passing the orig_filename_crc to _decodeExtra?

cpython/Lib/zipfile/__init__.py

Lines 1436 to 1463 in c75e016

filename = fp.read(centdir[_CD_FILENAME_LENGTH])

orig_filename_crc = crc32(filename)

flags = centdir[_CD_FLAG_BITS]

if flags & _MASK_UTF_FILENAME:

# UTF-8 file names extension

filename = filename.decode('utf-8')

else:

# Historical ZIP filename encoding

filename = filename.decode(self.metadata_encoding or 'cp437')

# Create ZipInfo instance to store file information

x = ZipInfo(filename)

x.extra = fp.read(centdir[_CD_EXTRA_FIELD_LENGTH])

x.comment = fp.read(centdir[_CD_COMMENT_LENGTH])

x.header_offset = centdir[_CD_LOCAL_HEADER_OFFSET]

(x.create_version, x.create_system, x.extract_version, x.reserved,

x.flag_bits, x.compress_type, t, d,

x.CRC, x.compress_size, x.file_size) = centdir[1:12]

if x.extract_version > MAX_EXTRACT_VERSION:

raise NotImplementedError("zip file version %.1f" %

(x.extract_version / 10))

x.volume, x.internal_attr, x.external_attr = centdir[15:18]

# Convert date/time code to (year, month, day, hour, min, sec)

x._raw_time = t

x.date_time = ( (d>>9)+1980, (d>>5)&0xF, d&0x1F,

t>>11, (t>>5)&0x3F, (t&0x1F) * 2 )

x.orig_filename_crc = orig_filename_crc

x._decodeExtra()

Looks like _decodeExtra is only used at this moment. WDYT?

That's a good idea! I will update. :)

bedevere-bot · 2023-03-10T01:40:56Z

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.

corona10 · 2023-03-10T02:12:52Z

Lib/zipfile/__init__.py

+                try:
+                    up_version, up_name_crc = unpack('<BL', data[:5])
+                    if up_version == 1 and up_name_crc == self.orig_filename_crc:
+                        up_unicode_name = data[5:].decode('utf-8')


According to the spec, please check the TSize if possible.

As UnicodeName is defined to be UTF-8, no UTF-8 byte order mark (BOM) is used. The length of this field is determined by subtracting the size of the previous fields from TSize.

The TSize is stored in the ln variable. I don't think we need to check it because we are storing data of the size defined by ln. If there is a problem, an exception would be thrown.

corona10 · 2023-03-10T02:13:46Z

Lib/zipfile/__init__.py

+                except struct.error:
+                    import warnings
+                    warnings.warn("Corrupt unicode path extra field (0x7075)", stacklevel=2)
+                except UnicodeDecodeError:
+                    import warnings
+                    warnings.warn('Corrupt unicode path extra field (0x7075): invalid utf-8 bytes', stacklevel=2)


Please raise BadZipFile error to follow other conventions.

I modified it in a way that throws an exception, but given that this is an extra field, wouldn't it be okay to ignore it and let the 'original' filename field be used if an exception is thrown?

This reverts commit 7630074.

yeojin-dev · 2023-03-16T12:37:51Z

I have made the requested changes; please review again

bedevere-bot · 2023-03-16T12:37:55Z

Thanks for making the requested changes!

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

yeojin-dev · 2023-04-05T11:19:36Z

I have made the requested changes; please review again

bedevere-bot · 2023-04-05T11:19:39Z

Thanks for making the requested changes!

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

corona10

lgtm

…ythongh-102566)

agiudiceandrea · 2023-08-11T16:31:20Z

@yeojin-dev, thank you very much!

agiudiceandrea and others added 3 commits March 10, 2023 01:44

Add support for Unicode Path Extra Field in ZipFile

671e241

Add testcase for Unicode Path Extra Field in ZipFile

045806c

Add news

e389807

bedevere-bot mentioned this pull request Mar 9, 2023

ZipFile does not supports Unicode Path Extra Field (0x7075) zip header field #86094

Closed

bedevere-bot added the awaiting review label Mar 9, 2023

Do not create a redundant file

a9502d7

corona10 self-requested a review March 9, 2023 23:42

corona10 requested changes Mar 10, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Mar 10, 2023

corona10 reviewed Mar 10, 2023

View reviewed changes

yeojin-dev added 5 commits March 15, 2023 22:07

Raise BadZipFile instead of warning

a094a35

Delete orig_filename_crc

7630074

Fix testcase

b7167c9

Revert "Delete orig_filename_crc"

80b94e9

This reverts commit 7630074.

Add import

c75e016

bedevere-bot added awaiting change review and removed awaiting changes labels Mar 16, 2023

bedevere-bot requested a review from corona10 March 16, 2023 12:37

arhadthedev added the stdlib Python modules in the Lib dir label Mar 22, 2023

yeojin-dev added 2 commits April 5, 2023 18:08

Use orig_filename_crc as local variable

7056bf2

Rollback structCentralDir

84f7359

corona10 self-assigned this Apr 5, 2023

corona10 approved these changes Apr 5, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 5, 2023

corona10 merged commit 8f70b16 into python:main Apr 5, 2023

bedevere-bot removed the awaiting merge label Apr 5, 2023

yeojin-dev deleted the gh-86094 branch April 5, 2023 11:58

gaogaotiantian pushed a commit to gaogaotiantian/cpython that referenced this pull request Apr 8, 2023

pythongh-86094: Add support for Unicode Path Extra Field in ZipFile (p…

e32cc99

…ythongh-102566)

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023

pythongh-86094: Add support for Unicode Path Extra Field in ZipFile (p…

f1c999a

…ythongh-102566)

ArcticLampyrid mentioned this pull request Aug 11, 2023

bpo-41928: Add support for Unicode Path Extra Field in ZipFile #23736

Closed

	if up_version == 1 and up_name_crc == self.orig_filename_crc:
	if up_version == 1 and up_name_crc == crc32(self.filename):

	filename = fp.read(centdir[_CD_FILENAME_LENGTH])
	orig_filename_crc = crc32(filename)
	flags = centdir[_CD_FLAG_BITS]
	if flags & _MASK_UTF_FILENAME:
	# UTF-8 file names extension
	filename = filename.decode('utf-8')
	else:
	# Historical ZIP filename encoding
	filename = filename.decode(self.metadata_encoding or 'cp437')
	# Create ZipInfo instance to store file information
	x = ZipInfo(filename)
	x.extra = fp.read(centdir[_CD_EXTRA_FIELD_LENGTH])
	x.comment = fp.read(centdir[_CD_COMMENT_LENGTH])
	x.header_offset = centdir[_CD_LOCAL_HEADER_OFFSET]
	(x.create_version, x.create_system, x.extract_version, x.reserved,
	x.flag_bits, x.compress_type, t, d,
	x.CRC, x.compress_size, x.file_size) = centdir[1:12]
	if x.extract_version > MAX_EXTRACT_VERSION:
	raise NotImplementedError("zip file version %.1f" %
	(x.extract_version / 10))
	x.volume, x.internal_attr, x.external_attr = centdir[15:18]
	# Convert date/time code to (year, month, day, hour, min, sec)
	x._raw_time = t
	x.date_time = ( (d>>9)+1980, (d>>5)&0xF, d&0x1F,
	t>>11, (t>>5)&0x3F, (t&0x1F) * 2 )

	x.orig_filename_crc = orig_filename_crc
	x._decodeExtra()

Uh oh!

gh-86094 Add support for Unicode Path Extra Field in ZipFile #102566

gh-86094 Add support for Unicode Path Extra Field in ZipFile #102566

Uh oh!

Conversation

yeojin-dev commented Mar 9, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

yeojin-dev Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

yeojin-dev Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Mar 10, 2023

Uh oh!

corona10 Mar 10, 2023 • 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!

yeojin-dev Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

corona10 Mar 10, 2023 • 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!

yeojin-dev Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

yeojin-dev commented Mar 16, 2023

Uh oh!

bedevere-bot commented Mar 16, 2023

Uh oh!

yeojin-dev commented Apr 5, 2023

Uh oh!

bedevere-bot commented Apr 5, 2023

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

agiudiceandrea commented Aug 11, 2023

Uh oh!

Uh oh!

yeojin-dev commented Mar 9, 2023 •

edited by bedevere-bot

Loading

corona10 Mar 10, 2023 •

edited

Loading

corona10 Mar 10, 2023 •

edited

Loading