Skip to content

bz2/lzma: Compressor/Decompressor objects are only initialized in __init__ #67413

Closed

Description

@vadmium
BPO 23224
Nosy @vstinner, @vadmium, @serhiy-storchaka, @animalize, @orenmn, @izbyshev, @corona10, @ZackerySpytz
PRs
  • gh-67413: Fix segfaults and multiple leaks in the lzma and bz2 modules #7822
  • Files
  • fix-lzma-bz2-segfaults.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 = None
    created_at = <Date 2015-01-12.00:58:28.305>
    labels = ['extension-modules', '3.10', '3.9', 'type-crash', '3.11']
    title = 'bz2/lzma: Compressor/Decompressor objects are only initialized in __init__'
    updated_at = <Date 2021-12-20.03:23:36.888>
    user = 'https://github.com/vadmium'

    bugs.python.org fields:

    activity = <Date 2021-12-20.03:23:36.888>
    actor = 'malin'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules']
    creation = <Date 2015-01-12.00:58:28.305>
    creator = 'martin.panter'
    dependencies = []
    files = ['45730']
    hgrepos = []
    issue_num = 23224
    keywords = ['patch']
    message_count = 9.0
    messages = ['233866', '233900', '236355', '282216', '282254', '320146', '325698', '365920', '408942']
    nosy_count = 10.0
    nosy_names = ['vstinner', 'nadeem.vawda', 'martin.panter', 'serhiy.storchaka', 'malin', 'Aaron1011', 'Oren Milman', 'izbyshev', 'corona10', 'ZackerySpytz']
    pr_nums = ['7822']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue23224'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    Activity

    1. vadmium commented on Jan 12, 2015

      @vadmium
      MemberAuthor

      Noticed in a patch review around LZMModules/lzmamodule.c:1055 that the C-level LZMADecompressor object is being initialized in an __init_() method. It crashes if you create the object with __new__() and never call __init__():

      >>> from lzma import LZMADecompressor
      >>> LZMADecompressor.__new__(LZMADecompressor).decompress(bytes())
      Segmentation fault (core dumped)
      [Exit 139]
    2. added
      type-crashA hard crash of the interpreter, possibly with a core dump
      on Jan 12, 2015
    3. vadmium commented on Jan 13, 2015

      @vadmium
      MemberAuthor

      A patch for this might conflict with the LZMA patch for bpo-15955, so it would be simplest to wait for that issue to be resolved first

    4. vadmium commented on Feb 21, 2015

      @vadmium
      MemberAuthor

      Similar situation in the bzip module:

      >>> BZ2Decompressor.__new__(BZ2Decompressor).decompress(bytes())
      Segmentation fault (core dumped)
    5. Aaron1011 commented on Dec 2, 2016

      Aaron1011mannequin
      Mannequin

      I've upload a patch which should address the issue in both the lzma and bz2 modules.

    6. serhiy-storchaka commented on Dec 2, 2016

      @serhiy-storchaka
      Member

      This is not the only issue. Calling __init__ multiple times causes leaking locks, bz2/lzma internal buffers, etc.

      It looks to me that the most straightforward way is using __new__ instead of __init__.

    7. vstinner commented on Jun 21, 2018

      @vstinner
      Member

      Calling directly __new__() is very unusual, so I'm not surprised that you found bugs. Calling __init__() twice is also an unusal practice. I concur with Serhiy: implementing new instead of init should prevent such bug. I recall that some objects decided instead to raise an exception if they are not fully initialized (if init has not been called), but using new instead of init seems to be safer approach if we can implement it.

      In the meanwhile, I blocked by the memory leak (handle leak) in bz2 and lzma: bpo-33916. I proposed a very simple fix: PR 7843. I propose to apply this one since it's easy to backport it to all branches, whereas replacing init with new would be more difficult and risky to backport: I would suggest to only make this change in the master.

      What do you think?

    8. izbyshev commented on Sep 18, 2018

      izbyshevmannequin
      Mannequin

      bz2 in 2.7 is also affected.

      Victor, do we want to fix the crash at all in stable branches? If yes, IMHO taking the slight risk of __init__ -> __new__ change is preferable to taking the trouble to implement the alternative backwards-compatible fix (init checks in all methods) for stable branches.

    9. changed the title [-]LZMADecompressor object is only initialized in __init__[/-] [+]bz2/lzma: Compressor/Decompressor objects are only initialized in __init__[/+] on Sep 18, 2018
    10. corona10 commented on Apr 7, 2020

      @corona10
      Member

      The issue is not solved yet.
      @ZackerySpytz would you like to finalize this issue?

    11. added
      3.11only security fixes
      and removed on Dec 19, 2021
    12. animalize commented on Dec 20, 2021

      animalizemannequin
      Mannequin

      These can be done in .__new__() method:

      • create thread lock
      • create (de)?compression context
      • initialize (de)?compressor states

      In .__init__() method, only set (de)?compression parameters. And prevent .__init__() method from being called multiple times.

      This mode works fine in my pyzstd module (A Python bindings to zstd library).
      But I think very few people will encounter this problem, we can leave it.

    13. transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.10 (EOL)end of life3.11only security fixes3.9 (EOL)end of lifeextension-modulesC modules in the Modules dirtype-crashA hard crash of the interpreter, possibly with a core dump

      Projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions