Skip to content

Conversation

@Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Oct 13, 2023

@JelleZijlstra
Copy link
Member

Looks good, could you check which versions this needs to be fixed on and add the appropriate backport labels?

@corona10 corona10 added the needs backport to 3.12 only security fixes label Oct 13, 2023
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for catching

@JelleZijlstra
Yeah we need to backport this patch to 3.12

@AlexWaygood
Copy link
Member

Seems like this code path isn't covered -- should we also add a test? Or would that be impractical?

@corona10 corona10 changed the title gh-110715: Add missing import gh-110715: Add missing import in zipfile Oct 13, 2023
@corona10
Copy link
Member

corona10 commented Oct 13, 2023

Seems like this code path isn't covered -- should we also add a test? Or would that be impractical?

It can be easily added something like this way.

    @requires_zlib()
    def test_read_zipfile_warning(self):
        with zipfile.ZipFile(TESTFN, mode='w') as zf:
            # create a file with a non-ASCII name
            filename = '이름.txt'
            filename_encoded = filename.encode('utf-8')

            # create a ZipInfo object with Unicode path extra field
            zip_info = zipfile.ZipInfo(filename)

            tag_for_unicode_path = b'\x75\x70'
            version_of_unicode_path = b'\x01'

            import zlib
            filename_crc = struct.pack('<L', zlib.crc32(filename_encoded))

            extra_data = version_of_unicode_path + filename_crc + b''
            tsize = len(extra_data).to_bytes(2, 'little')

            zip_info.extra = tag_for_unicode_path + tsize + extra_data

            # add the file to the ZIP archive
            zf.writestr(zip_info, b'Hello World!')

        with zipfile.ZipFile(TESTFN, "r") as zf:
            self.assertEqual(zf.filelist[0].filename, "이름.txt")

@corona10
Copy link
Member

@Eclips4 Would you like to add a test code for this?

@Eclips4
Copy link
Member Author

Eclips4 commented Oct 13, 2023

@Eclips4 Would you like to add a test code for this?

Yeah, I'll give a try!

@Eclips4
Copy link
Member Author

Eclips4 commented Oct 13, 2023

@Eclips4 Would you like to add a test code for this?

Done.
I write a helper for it (to reduce the size of the code, since it is repeated several times).
Sadly, but I doesn't find a way to trigger struct.error (550 line in Lib/zipfile/__init__.py is trying to catch this exception).

@Eclips4 Eclips4 requested a review from corona10 October 13, 2023 16:38
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM!

@corona10 corona10 merged commit 4110cfe into python:main Oct 14, 2023
@miss-islington
Copy link
Contributor

Thanks @Eclips4 for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 14, 2023
(cherry picked from commit 4110cfe)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2023

GH-110861 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 14, 2023
@Eclips4
Copy link
Member Author

Eclips4 commented Oct 14, 2023

Thanks for the review @corona10! 🚀

@Eclips4 Eclips4 deleted the issue-110715 branch October 14, 2023 07:24
corona10 pushed a commit that referenced this pull request Oct 14, 2023
gh-110715: Add missing import in zipfile (gh-110822)
(cherry picked from commit 4110cfe)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@AlexWaygood
Copy link
Member

Thanks @Eclips4 and @corona10! :D

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants