Skip to content
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.

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

Make zipimport work with zipfile containing comments #50200

Closed
dsamersoff mannequin opened this issue May 6, 2009 · 25 comments
Closed

Make zipimport work with zipfile containing comments #50200

dsamersoff mannequin opened this issue May 6, 2009 · 25 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@dsamersoff
Copy link
Mannequin

dsamersoff mannequin commented May 6, 2009

BPO 5950
Nosy @Yhg1s, @warsaw, @brettcannon, @theller, @birkenfeld, @terryjreedy, @gpshead, @ncoghlan, @ericsnowcurrently, @dimaqq, @serhiy-storchaka, @ZackerySpytz
PRs
  • bpo-5950: Support reading zips with comments in zipimport #9548
  • Files
  • testcase.zip: Testcase zipfile with comment inside
  • zipimport_with_comments.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-09-25.19:16:49.640>
    created_at = <Date 2009-05-06.16:49:46.012>
    labels = ['3.8', 'type-feature', 'library']
    title = 'Make zipimport work with zipfile containing comments'
    updated_at = <Date 2018-09-25.19:16:49.638>
    user = 'https://bugs.python.org/dsamersoff'

    bugs.python.org fields:

    activity = <Date 2018-09-25.19:16:49.638>
    actor = 'barry'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-25.19:16:49.640>
    closer = 'barry'
    components = ['Library (Lib)']
    creation = <Date 2009-05-06.16:49:46.012>
    creator = 'dsamersoff'
    dependencies = []
    files = ['13905', '17975']
    hgrepos = []
    issue_num = 5950
    keywords = ['patch']
    message_count = 25.0
    messages = ['87340', '97316', '109678', '109686', '110014', '110016', '110059', '110151', '110157', '113452', '113467', '113484', '113486', '139187', '139192', '175127', '175169', '219263', '219266', '226234', '325717', '325721', '325733', '326284', '326398']
    nosy_count = 17.0
    nosy_names = ['twouters', 'barry', 'brett.cannon', 'theller', 'georg.brandl', 'terry.reedy', 'gregory.p.smith', 'ncoghlan', 'peter.otten', 'grubert', 'rfk', 'dsamersoff', 'eric.snow', 'Dima.Tisnek', 'serhiy.storchaka', 'superluser', 'ZackerySpytz']
    pr_nums = ['9548']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5950'
    versions = ['Python 3.8']

    @dsamersoff
    Copy link
    Mannequin Author

    dsamersoff mannequin commented May 6, 2009

    Synopsys:
    zimport not able to import a module from zipfile if zipfile contains
    comment.
    Versions:
    This is Zip 2.32 (June 19th 2006), by Info-ZIP
    Python 2.5.2 or 2.6.2

    Steps to reproduce:
    create a module, create an app that imports this module.
    zip the module, make sure it works.
    Run: echo "Some comments" | zip -z module.zip
    the app stop working.

    @dsamersoff dsamersoff mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 6, 2009
    @birkenfeld
    Copy link
    Member

    Yes, comments are not supported right now. Documented this in r77333, and turning this issue into a feature request for adding that support.

    @birkenfeld birkenfeld added the type-feature A feature request or enhancement label Jan 6, 2010
    @terryjreedy
    Copy link
    Member

    Does 'zimport' refer to the 3.1 zipfile or zipimport modules or an internal function called zimport?

    @terryjreedy terryjreedy changed the title zimport doesn't work with zipfile containing comments Make zimport work with zipfile containing comments Jul 9, 2010
    @dsamersoff
    Copy link
    Mannequin Author

    dsamersoff mannequin commented Jul 9, 2010

    I'm talking about internal zimport function (see attached testcase):

    i.e.

    import sys;
    sys.path.insert(0,'test.zip');
    
    import test
    test.testme()

    doesn't work if test.zip contains comment.

    @rfk
    Copy link
    Mannequin

    rfk mannequin commented Jul 11, 2010

    Attached is my attempt at a patch for this functionality, along with some simple tests. This basically mirrors what's done in zipfile.py, searching backwards through the file until it finds the end-of-central-directory marker. It tries to be memory conscious by reading in small chunks.

    Patch is against trunk; I've also tested it against 2.7 and it seems to work. Any chance of it being backported into 2.7?

    Also wanted to mention a real-world usecase for this functionality. I want to digitally sign a frozen python program with appended zipfile, which involves appending the signature to the EXE. Simple to do if only zipimport would support appended comments.

    @rfk
    Copy link
    Mannequin

    rfk mannequin commented Jul 11, 2010

    Whoops, forgot to remove the line from the docs about comments not being supported. Updated the patch accordingly.

    @birkenfeld birkenfeld changed the title Make zimport work with zipfile containing comments Make zipimport work with zipfile containing comments Jul 11, 2010
    @terryjreedy
    Copy link
    Member

    "the line from the docs about comments not being supported." answers your question. Current behavior is documented behavior and therefor changing it is not a bugfix.

    @rfk
    Copy link
    Mannequin

    rfk mannequin commented Jul 13, 2010

    I can't imagine anyone depending on this lack-of-feature, but there's no arguing with the technicality of it. One more small incentive to make the jump to Python 3 then.

    Anyway, I've revisited the patch to clean up the logic and control flow, and added another test to ensure that it works even when the EOCD record crosses a chunk boundary. Also checked that it works on win32, which fortunately it did without modification.

    @terryjreedy
    Copy link
    Member

    I can't imagine anyone depending on this lack-of-feature,

    Neither can I, but I can imaging someone writing code that depended on the feature. If added to 3.1.3, then code that depended on it would not run on 3.1, 3.1.1, and 3.1.2. The same is true, of course, for bug fixes, but the doc does not need changing for bug fixes, and the impact is otherwise less.

    One more small incentive to make the jump to Python 3 then.

    Or to 3.2 from any 3.1 version, which is the normal progession absent the dual track anomaly.

    @terryjreedy
    Copy link
    Member

    I believe this is covered by the PEP-3003 3.2 change moratorium.

    @birkenfeld
    Copy link
    Member

    Why?

    @terryjreedy
    Copy link
    Member

    Because it adds a feature to the import statement, rather than to a module, or so Dmitry said. But fine with me if it goes in now.

    @terryjreedy
    Copy link
    Member

    On bpo-1644818, Brett said import statement is exempt from moratorium.

    @grubert
    Copy link
    Mannequin

    grubert mannequin commented Jun 26, 2011

    +1*4

    @terryjreedy
    Copy link
    Member

    Brett, Nick, this could be considered a patch to the import machinery as the bugs shows with a import statement. In any case, no one is signed up for zipimport specifically.

    @dimaqq
    Copy link
    Mannequin

    dimaqq mannequin commented Nov 7, 2012

    also applies to 2.7 series

    over a year passed since last comment, any progress on this?

    I just ran into this issue myself.
    zipfile module handles commented zip's fine, but zipimport doesn't.
    I didn't expect a gotcha like this from Python!

    @brettcannon
    Copy link
    Member

    This is a feature request so it won't change in Python 2.7.

    As for progress, the answer is no as the hope is to eventually replace zipimport with something in pure Python thanks to importlib.

    @theller
    Copy link

    theller commented May 28, 2014

    As for progress, the answer is no as the hope is to eventually replace
    zipimport with something in pure Python thanks to importlib.

    Does this mean there is no chance to put this into 3.4 and 3.3, do we really have to wait until 3.5 with it's pure-python zipimport?

    I (together with the py2exe-users) have the same usecase as Ryan Kelly in msg110014:

    I want to digitally sign a frozen python program with appended zipfile,
    which involves appending the signature to the EXE. Simple to do if
    only zipimport would support appended comments.

    @brettcannon
    Copy link
    Member

    I actually stopped development of a pure Python zip importer so it won't be in Python 3.5 either (http://bugs.python.org/issue17630). The motivation simply wasn't there if zipimport is going to be sticking around for bootstrapping reasons. This can still get fixed in 3.5 (3.4 is Larry's call).

    I have added Thomas Wouters and Greg Smith who have done the most work with zipimport to the nosy list to see if they have interest in taking a look at the patch.

    @serhiy-storchaka
    Copy link
    Member

    See bpo-22322 for yet one use case ("git archive" creates ZIP file with archive comment).

    @serhiy-storchaka
    Copy link
    Member

    zipimport have been rewritten in pure Python (bpo-25711).

    @dimaqq
    Copy link
    Mannequin

    dimaqq mannequin commented Sep 19, 2018

    Very glad to hear!
    Let's document what Python version(s) are "fixed".
    Perhaps this issue deserves a test case in bpo-25711.

    @serhiy-storchaka
    Copy link
    Member

    It is not fixed yet. But it is now easier to fix.

    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented Sep 24, 2018

    I've created a PR.

    @ZackerySpytz ZackerySpytz mannequin added the 3.8 only security fixes label Sep 24, 2018
    @warsaw
    Copy link
    Member

    warsaw commented Sep 25, 2018

    New changeset 5a5ce06 by Barry Warsaw (Zackery Spytz) in branch 'master':
    bpo-5950: Support reading zips with comments in zipimport (bpo-9548)
    5a5ce06

    @warsaw warsaw added stdlib Python modules in the Lib dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 25, 2018
    @warsaw warsaw closed this as completed Sep 25, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants