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

test test_multibytecodec: Test_IncrementalEncoder.test_subinterp() leaks references #87032

Closed
vstinner opened this issue Jan 8, 2021 · 18 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

vstinner commented Jan 8, 2021

BPO 42866
Nosy @vstinner, @corona10, @miss-islington, @erlend-aasland
PRs
  • bpo-42866: Fix refleak in CJK getcodec() #24165
  • bpo-42866: Add traverse func to _multibytecodec.MultibyteCodec #24166
  • [3.9] bpo-42866: Fix refleak in CJK getcodec() (GH-24165) #24167
  • [3.8] bpo-42866: Fix refleak in CJK getcodec() (GH-24165) #24168
  • 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 2021-01-08.14:49:29.209>
    created_at = <Date 2021-01-08.09:28:39.500>
    labels = ['library', '3.10']
    title = 'test test_multibytecodec: Test_IncrementalEncoder.test_subinterp() leaks references'
    updated_at = <Date 2021-01-08.14:49:29.209>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-01-08.14:49:29.209>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-01-08.14:49:29.209>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2021-01-08.09:28:39.500>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42866
    keywords = ['patch']
    message_count = 18.0
    messages = ['384643', '384645', '384647', '384648', '384650', '384651', '384652', '384653', '384654', '384661', '384662', '384665', '384667', '384669', '384670', '384671', '384673', '384675']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'corona10', 'miss-islington', 'erlendaasland']
    pr_nums = ['24165', '24166', '24167', '24168']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42866'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    $ ./python -m test test_multibytecodec -m test.test_multibytecodec.Test_IncrementalEncoder.test_subinterp -R 3:3
    (...)
    test_multibytecodec leaked [258, 258, 258] references, sum=774

    I simplified the code. The following test leaks references:

        def test_subinterp(self):
            import _testcapi
            code = textwrap.dedent("""
                import _codecs_jp
                codec = _codecs_jp.getcodec('cp932')
                codec = None
            """)
            _testcapi.run_in_subinterp(code)

    _codecs_jp.getcodec() is defined in Modules/cjkcodecs/cjkcodecs.h. Extract:

        cofunc = getmultibytecodec();
        ...
        codecobj = PyCapsule_New((void *)codec, PyMultibyteCodec_CAPSULE_NAME, NULL);
        if (codecobj == NULL)
            return NULL;
        r = PyObject_CallOneArg(cofunc, codecobj);
        Py_DECREF(codecobj);

    getmultibytecodec() is the _multibytecodec.__create_codec() which is defined in Modules/cjkcodecs/multibytecodec.c. Simplified code:

        codec = PyCapsule_GetPointer(arg, PyMultibyteCodec_CAPSULE_NAME);
        _multibytecodec_state *state = _multibytecodec_get_state(module);
        self = PyObject_New(MultibyteCodecObject, state->multibytecodec_type);
        self->codec = codec;
        return (PyObject *)self;

    @vstinner vstinner added 3.10 only security fixes stdlib Python modules in the Lib dir labels Jan 8, 2021
    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    I added the test which leaks in bpo-42846.

    commit 07f2cee
    Author: Victor Stinner <vstinner@python.org>
    Date: Fri Jan 8 00:15:22 2021 +0100

    bpo-42846: Convert CJK codec extensions to multiphase init (GH-24157)
    
    Convert the 6 CJK codec extension modules (_codecs_cn, _codecs_hk,
    _codecs_iso2022, _codecs_jp, _codecs_kr and _codecs_tw) to the
    multiphase initialization API (PEP-489).
    
    Remove getmultibytecodec() local cache: always import
    _multibytecodec. It should be uncommon to get a codec. For example,
    this function is only called once per CJK codec module.
    
    Fix a reference leak in register_maps() error path.
    

    I don't think that the leak is new. It's just that it wasn't seen previously.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    By the way, I wrote an article "Leaks discovered by subinterpreters":
    https://vstinner.github.io/subinterpreter-leaks.html

    This leak may be new kind related to capsule, I'm not sure so far.

    @erlend-aasland
    Copy link
    Contributor

    Thank you so much for taking the time to write these blog posts, Victor, and for explaining your fixes is such great detail. It is very helpful!

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    PR 24165 fix one reference leak in the getcodec() function of CJK codecs. But it doesn't fix all ref leaks of this issue.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    The leak can be simplified to:

        @support.cpython_only
        def test_subinterp(self):
            import _testcapi
            code = textwrap.dedent("""
                import encodings
                import _codecs_jp
                encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')
            """)
            res = _testcapi.run_in_subinterp(code)
            self.assertEqual(res, 0)

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    See also bpo-42671 "Make the Python finalization more deterministic" but it seems like PR 23826 makes the leak worse (+2000 leaked references instead of +200) :-)

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')

    • encodings._cache is kept alive by encodings.search_function.__globals__
    • encodings.search_function function is kept alive by PyInterpreterState.codec_search_path list. The function by _PyCodec_Register() in encodings/init.py: codecs.register(search_function).

    For example, unregistering the search function prevents the leak:

                import encodings
                import _codecs_jp
                encodings._cache['cp932'] = _codecs_jp.getcodec('cp932')
    
                import codecs
                codecs.unregister(encodings.search_function)

    The PyInterpreterState.codec_search_path list is cleared at Python exit by interpreter_clear().

    The weird part is that the _codecs_jp.getcodec('cp932') codec object *is* deleted. I checked and multibytecodec_dealloc() is called with the object stored in the encodings cache.

    A _multibytecodec.MultibyteCodec instance (MultibyteCodecObject* structure in C) is a simple type: it only stores pointer to C functions and C strings. It doesn't contain any Python object. So I don't see how it could be part of a reference cycle by itself. Moreover, again, it is deleted.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    Calling gc.collect() twice works around the issue, which sounds like a missing traverse function somewhere.

    diff --git a/Python/pystate.c b/Python/pystate.c
    index c791b23999..66bbe1bf7d 100644
    --- a/Python/pystate.c
    +++ b/Python/pystate.c
    @@ -321,6 +321,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
     
         /* Last garbage collection on this interpreter */
         _PyGC_CollectNoFail(tstate);
    +    _PyGC_CollectNoFail(tstate);
         _PyGC_Fini(tstate);
     
         /* We don't clear sysdict and builtins until the end of this function.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    New changeset e542d41 by Victor Stinner in branch 'master':
    bpo-42866: Fix refleak in CJK getcodec() (GH-24165)
    e542d41

    @erlend-aasland
    Copy link
    Contributor

    Don't you need to free memory using PyObject_GC_Del when you allocate using PyObject_GC_New?

    @erlend-aasland
    Copy link
    Contributor

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    Don't you need to free memory using PyObject_GC_Del when you allocate using PyObject_GC_New?

    My PR uses the generic tp->tp_free which PyObject_GC_Del().

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    typo: My PR uses the generic tp->tp_free which *is* PyObject_GC_Del().

    @erlend-aasland
    Copy link
    Contributor

    Ah, thanks! I also found that info in the docs: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_free

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    New changeset 11ef53a by Victor Stinner in branch 'master':
    bpo-42866: Add traverse func to _multibytecodec.MultibyteCodec (GH-24166)
    11ef53a

    @vstinner
    Copy link
    Member Author

    vstinner commented Jan 8, 2021

    Ok, it's now fixed.

    To make sure that a heap type can be deleted, it must by a GC type, has a traverse function, and it must track its instances. We should take this in account when we convert static types to heap types. In practice, deleting a type is mostly an issue when we check for reference leak and an instance is kept alive until the last GC collection, at the end of Py_EndInterpreter().

    @vstinner vstinner closed this as completed Jan 8, 2021
    @vstinner vstinner closed this as completed Jan 8, 2021
    @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.10 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants