bpo-40412: Nullify inittab_copy during finalization

[indygreg]

Otherwise we leave a dangling pointer to free'd memory. If we
then initialize a new interpreter in the same process and call
PyImport_ExtendInittab, we will (likely) crash when calling
PyMem_RawRealloc(inittab_copy, ...) since the pointer address
is bogus.

https://bugs.python.org/issue40412

Automerge-Triggered-By: @brettcannon

[ericsnowcurrently]

LGTM

Good catch! How did you notice this?

[indygreg]

I discovered it while writing tests for PyOxidizer's custom meta path importer. The tests initialize several Python interpreters per process.

Also, is it too late to get this patch in 3.8.3?

[brettcannon]

@indygreg mind doing a quick news entry? This close to beta means people are starting to care about every change more and more. You can use https://blurb-it.herokuapp.com/ as linked from "Details" for the bedevere/news line if you want to use a web form to fill it out.

[brettcannon]

Simplification of the news entry and then this is ready to go!

[bedevere-bot]

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.

[miss-islington]

@indygreg: Status check is done, and it's a failure ❌ .

[brettcannon]

OK, thanks to the flag being set for submitting my own suggestions, this PR looks good to go! I have set it to automerge and backport to 3.8 and 3.7.

[brettcannon]

I also tossed in a 3.6 backport in case Ned wants to take it due to it being a memory thing.

[miss-islington]

@indygreg: Status check is done, and it's a failure ❌ .

[miss-islington]

@indygreg: Status check is done, and it's a success ✅ .

[miss-islington]

Thanks @indygreg for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7, 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-19841 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

GH-19842 is a backport of this pull request to the 3.7 branch.

[miss-islington]

Sorry, @indygreg, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 64224a4727321a8dd33e6f769edda401193ebef0 3.6

[indygreg]

While NULLifying the invalid pointer is certainly worthy of backporting everywhere, I'm not sure this issue is reproducible on Python <3.8. The reason is that the code that clears this pointer appears to be called after Py_FinalizeEx(). I'm unsure if this code existed before initialization API refactors in 3.8. @vstinner would know.

I've already thrown away PyOxidizer's code to support Python 3.7 and can't easily test. This will require a source inspection to validate against older versions.

[vstinner]

Hum, inittab_copy is freed by pymain_free(). I added this function to clear objects and memory which cannot be freed by Py_Finalize() for backward compatibility.

The PEP 587 says:

PyImport_FrozenModules, PyImport_AppendInittab() and PyImport_ExtendInittab() functions are still relevant and continue to work as previously. They should be set or called after Python preinitialization and before the Python initialization.

I understood that PyImport_AppendInittab() can be called only once, but Py_Initialize() and Py_Finalized() can be called multiple times: the PyImport_AppendInittab() call is expected to be saved even after Py_Finalized().

I understand that this change only impacts C code which calls Py_RunMain() or Py_Main() multiple times. I'm not sure if it was supported previously, but it's a good thing if slowly Python supports it :-)

[vstinner]

I'm not sure this issue is reproducible on Python <3.8

I introduced the issue in Python 3.7 with commit 92a3c6f:

commit 92a3c6f493ad411e4cf0acdf305ef4876aa90669
Author: Victor Stinner <[email protected]>
Date:   Wed Dec 6 18:12:59 2017 +0100

    [bpo-32030](https://bugs.python.org/issue32030): Add _PyImport_Fini2() (#4737)
    
    PyImport_ExtendInittab() now uses PyMem_RawRealloc() rather than
    PyMem_Realloc(). PyImport_ExtendInittab() can be called before
    Py_Initialize() whereas only the PyMem_Raw allocator is supposed to
    be used before Py_Initialize().
    
    Add _PyImport_Fini2() to release the memory allocated by
    PyImport_ExtendInittab() at exit. PyImport_ExtendInittab() now forces
    the usage of the default raw allocator, to be able to release memory
    in _PyImport_Fini2().

A backport of the fix sounds safe, but I don't think that it is safe to call Py_Main() multiple times in Python 3.7. Maybe it works, I don't know :-)