bpo-35993: Fix an incorrect use of released memory #11852

matrixise · 2019-02-14T08:16:00Z

https://bugs.python.org/issue35993

vstinner · 2019-02-14T11:29:48Z

Python/pystate.c

@@ -293,7 +294,8 @@ _PyInterpreterState_DeleteExceptMain()
        if (interp->id_mutex != NULL) {
            PyThread_free_lock(interp->id_mutex);
        }
-        PyMem_RawFree(interp);
+        tmp_interp = interp;
+        PyMem_RawFree(tmp_interp);


I don't understand how the current code can work since PyMem_RawFree() fills freed memory with a byte pattern 0xDB and so interp = interp->next should just crash.

I don't see how this change fix anything: interp still points to freed bytes.

You should read interp->next before PyMem_RawFree(), and so change how the loop is written.

cc @ericsnowcurrently

When PyMem_RawFree(tmp_interp) the tmp_interp point to interp, so you are freed interp too, right?

What about this?

tmp_interp = interp->next; for (; tmp_interp !=NULL; interp = tmp_interp){ ... tmp_interp = interp->next; PyMem_RawFree(interp); }

sure? the last time I have coded in C, that was in 2007 :/ ok

or just

PyInterpreterState *tmp_interp = NULL; for (; interp != NULL;) { ... tmp_interp = interp; interp = interp->next; PyMem_RawFree(tmp_interp); }

@vstinner and @eamanu

@eamanu are you on Zulip or IRC?

@matrixise I am connecting to zulip

With a while (interp) should work the same, and the jungling of tmp/interp looks good to me.

@JulienPalard I have just updated my PR, thanks for the review.
Now, I am going to wait for the review of @vstinner, he is more expert than me with the C language.

JulienPalard

LGTM

eamanu

lgtm

vstinner · 2019-02-14T15:38:17Z

Python/pystate.c

-    for (; interp != NULL; interp = interp->next) {
+    fprintf(stdout, "1. interp: %p\n", interp);
+    fprintf(stdout, "2. interp->next: %p\n", interp->next);
+    fprintf(stdout, "2.1. _PyRuntime.interpreters.main: %p\n", _PyRuntime.interpreters.main);


Remove debug printf.

(Those have been pushed recently to diagnose the "take infinite time to build when this code is added" / "make CI timeout" issue)

@vstinner yep, just for the debug and because I wanted to show you

Python/pystate.c

bedevere-bot · 2019-02-14T15:38:20Z

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.

vstinner

I don't understand how the current code can code: it must crash if I understand correctly.

Is the function called? You may modify the code to add abort(), run the full Python test suite and check if a test does crash.

If there is no crash, would it be possible to add a test?

cc @ericsnowcurrently

vstinner · 2019-02-14T15:40:40Z

for (; tmp_interp !=NULL; interp = tmp_interp){
...
tmp_interp = interp->next;
PyMem_RawFree(interp);
}

I prefer this version, especially if you rename tmp_interp to next_interp.

JulienPalard · 2019-02-14T15:41:00Z

Current version does not compile, runs indefinitely, making CI timeout.

Changed.

matrixise · 2019-02-14T16:15:50Z

@vstinner it's my fault if my PR is broken after the approval of @JulienPalard. But I am working on it and try to fix it.

matrixise · 2019-02-14T16:24:13Z

@vstinner @ericsnowcurrently the current code is used when you compile Python, but there is no crash (un/fortunately)

JulienPalard

LGTM

matrixise · 2019-02-14T16:32:27Z

@vstinner the code works and the tests pass. no lock on travis.

vstinner · 2019-02-14T16:54:34Z

@vstinner @ericsnowcurrently the current code is used when you compile Python

Would you mind to elaborate? I would like to make sure that the function is properly tested.

but there is no crash (un/fortunately)

Strange. So maybe the current code is just fine? Or we miss a test.

matrixise · 2019-02-14T17:10:51Z

@vstinner example when you compile python

gcc -pthread -shared build/temp.linux-x86_64-3.8-pydebug/home/stephane/src/github.com/python/cpython/Modules/_contextvarsmodule.o -L/home/stephane/src/github.com/python/cpython-add_smtpshandler/lib -L/usr/local/lib -o build/lib.linux-x86_64-3.8-pydebug/_contextvars.cpython-38dm-x86_64-linux-gnu.so
1 _PyInterpreterState_DeleteExceptMain
2 _PyInterpreterState_DeleteExceptMain
#### _PyInterpreterState_DeleteExceptMain
building 'cmath' extension
gcc -pthread -fPIC -Wno-unused-result -Wsign-compare -g -Og -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -I./Include/internal -I./Include -I/home/stephane/src/github.com/python/cpython-add_smtpshandler/include -I. -I/usr/local/include -I/home/stephane/src/github.com/python/cpython/Include -I/home/stephane/src/github.com/python/cpython -c /home/stephane/src/github.com/python/cpython/Modules/cmathmodule.c -o build/temp.linux-x86_64-3.8-pydebug/home/stephane/src/github.com/python/cpython/Modules/cmathmodule.o
1 _PyInterpreterState_DeleteExceptMain
2 _PyInterpreterState_DeleteExceptMain
#### _PyInterpreterState_DeleteExceptMain

vstinner · 2019-02-14T17:22:33Z

Ok, but I guess that the code path which trigger the bug is not taken, since according to the bug tracker, it must crash. I guess that no test call this function with more than 1 interpreter. That's why I added @ericsnowcurrently in copy of this change :-)

eamanu · 2019-02-14T19:16:50Z

I am not sure when _PyInterpreterState_DeleteExceptMain() is called. This way we could test it. @vstinner @matrixise

Python/pystate.c

matrixise · 2019-02-15T08:01:30Z

@eamanu as you can see my paste in the previous comment, the function is called when we try to compile Python, certainly in the Makefile.

methane · 2019-02-15T10:15:20Z

I don't understand how the current code can code: it must crash if I understand correctly.

@vstinner this for loop iterate over (sub)interpreters.
When there is only main interpreter, PyMem_RawFree(interp); is never called.

cpython/Python/pystate.c

Lines 285 to 289 in 3e028b2

    
           if (interp == _PyRuntime.interpreters.main) { 
        
               _PyRuntime.interpreters.main->next = NULL; 
        
               _PyRuntime.interpreters.head = interp; 
        
               continue; 
        
           }

matrixise · 2019-02-18T12:23:11Z

@ericsnowcurrently Any advice for a test for this part?
Thank you,

vstinner · 2019-02-18T23:51:57Z

Misc/NEWS.d/next/Core and Builtins/2019-02-14-09-17-54.bpo-35993.Bvm3fP.rst

@@ -0,0 +1 @@
+Fix an incorrect use of released memory. Contributed by Stéphane Wirtel


Only from this NEWS entry, it's hard to know which code is impacted. I found PyOS_AfterFork_Child() which calls _PyInterpreterState_DeleteExceptMain(). I understand that the bug triggers if an application using multiple interpreters forks... it sounds like a very unusual use case. I propose:

"Fix a crash on fork when using subinterpreters. Contributed by Stéphane Wirtel."

matrixise · 2019-02-20T13:50:47Z

@vstinner I have just updated the blurb entry, sorry I didn't see your notification

miss-islington · 2019-02-20T14:27:25Z

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

miss-islington · 2019-02-20T14:27:33Z

Sorry, @matrixise and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b5409dacc4885146a27d06482b346e55fa12d2ec 3.7

vstinner · 2019-02-20T14:29:44Z

Ah, it's a new function in Python 3.8. I didn't know. No need to backport in that case.

matrixise · 2019-02-20T14:35:33Z

@vstinner thank you for the merge ;-) a good news for today ;-)

Fix a crash on fork when using subinterpreters.

the-knights-who-say-ni added the CLA signed label Feb 14, 2019

bedevere-bot added the awaiting review label Feb 14, 2019

bpo-35993: Fix an incorrect use of released memory

de8886b

matrixise force-pushed the bpo-35993 branch from 9077764 to de8886b Compare Feb 14, 2019

vstinner reviewed Feb 14, 2019

View changes

matrixise added 2 commits Feb 14, 2019

interp is updated with the next element

4100ec4

use a while loop instead of the for loop

f8ee81c

JulienPalard previously approved these changes Feb 14, 2019

View changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 14, 2019

eamanu approved these changes Feb 14, 2019

View changes

matrixise changed the title ~~bpo-35993: Fix an incorrect use of released memory~~ WIP: bpo-35993: Fix an incorrect use of released memory Feb 14, 2019

debug the code and execute it on Travis

3957c72

vstinner requested changes Feb 14, 2019

View changes

bedevere-bot removed the awaiting merge label Feb 14, 2019

bedevere-bot added the awaiting changes label Feb 14, 2019

vstinner reviewed Feb 14, 2019

View changes

JulienPalard added the DO-NOT-MERGE label Feb 14, 2019

Remove the fprintf functions and use replace tmp_interp by prev_interp

3ab72b5

JulienPalard approved these changes Feb 14, 2019

View changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 14, 2019

PEP7

a765146

JulienPalard removed the DO-NOT-MERGE label Feb 14, 2019

matrixise changed the title ~~WIP: bpo-35993: Fix an incorrect use of released memory~~ bpo-35993: Fix an incorrect use of released memory Feb 14, 2019

methane reviewed Feb 15, 2019

View changes

Python/pystate.c Outdated Show resolved Hide resolved

Update pystate.c

ad0cf0c

methane approved these changes Feb 15, 2019

View changes

vstinner reviewed Feb 18, 2019

View changes

Update the blurb entry

c805277

vstinner added the needs backport to 3.7 label Feb 20, 2019

vstinner merged commit b5409da into python:master Feb 20, 2019
5 checks passed

bedevere-bot removed the awaiting merge label Feb 20, 2019

miss-islington assigned vstinner Feb 20, 2019

vstinner removed the needs backport to 3.7 label Feb 20, 2019

ma8ma pushed a commit to ma8ma/cpython that referenced this issue Apr 7, 2019

bpo-35993: Fix _PyInterpreterState_DeleteExceptMain() (pythonGH-11852)

927438e

Fix a crash on fork when using subinterpreters.

arnolddumas pushed a commit to arnolddumas/cpython that referenced this issue May 3, 2019

bpo-35993: Fix _PyInterpreterState_DeleteExceptMain() (pythonGH-11852)

00b4e36

Fix a crash on fork when using subinterpreters.

python / cpython Public

bpo-35993: Fix an incorrect use of released memory #11852

bpo-35993: Fix an incorrect use of released memory #11852

matrixise commented Feb 14, 2019 •

edited by bedevere-bot

vstinner Feb 14, 2019

eamanu Feb 14, 2019

matrixise Feb 14, 2019

matrixise Feb 14, 2019 •

edited

matrixise Feb 14, 2019

eamanu Feb 14, 2019

JulienPalard Feb 14, 2019

matrixise Feb 14, 2019

JulienPalard left a comment

eamanu left a comment

vstinner Feb 14, 2019

JulienPalard Feb 14, 2019

matrixise Feb 14, 2019

bedevere-bot commented Feb 14, 2019

vstinner left a comment

vstinner commented Feb 14, 2019

JulienPalard commented Feb 14, 2019

matrixise commented Feb 14, 2019

matrixise commented Feb 14, 2019

JulienPalard left a comment

matrixise commented Feb 14, 2019

vstinner commented Feb 14, 2019

matrixise commented Feb 14, 2019

vstinner commented Feb 14, 2019

eamanu commented Feb 14, 2019

matrixise commented Feb 15, 2019

methane commented Feb 15, 2019

matrixise commented Feb 18, 2019

vstinner Feb 18, 2019

matrixise commented Feb 20, 2019

miss-islington commented Feb 20, 2019

miss-islington commented Feb 20, 2019

vstinner commented Feb 20, 2019

matrixise commented Feb 20, 2019

		@@ -0,0 +1 @@
		Fix an incorrect use of released memory. Contributed by Stéphane Wirtel

python / cpython Public

bpo-35993: Fix an incorrect use of released memory #11852

bpo-35993: Fix an incorrect use of released memory #11852

Conversation

matrixise commented Feb 14, 2019 • edited by bedevere-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matrixise Feb 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulienPalard left a comment

eamanu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 14, 2019

vstinner left a comment

vstinner commented Feb 14, 2019

JulienPalard commented Feb 14, 2019

matrixise commented Feb 14, 2019

matrixise commented Feb 14, 2019

JulienPalard left a comment

matrixise commented Feb 14, 2019

vstinner commented Feb 14, 2019

matrixise commented Feb 14, 2019

vstinner commented Feb 14, 2019

eamanu commented Feb 14, 2019

matrixise commented Feb 15, 2019

methane commented Feb 15, 2019

matrixise commented Feb 18, 2019

Choose a reason for hiding this comment

matrixise commented Feb 20, 2019

miss-islington commented Feb 20, 2019

miss-islington commented Feb 20, 2019

vstinner commented Feb 20, 2019

matrixise commented Feb 20, 2019

matrixise commented Feb 14, 2019 •

edited by bedevere-bot

matrixise Feb 14, 2019 •

edited