bpo-35993: Fix an incorrect use of released memory by matrixise · Pull Request #11852 · python/cpython

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

https://bugs.python.org/issue35993

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

        }
-        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:37:22Z

-    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

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

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:54Z

@@ -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 ;-)

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 February 14, 2019 08:18

vstinner reviewed Feb 14, 2019

View reviewed changes

matrixise added 2 commits February 14, 2019 15:21

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 reviewed changes

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

eamanu approved these changes Feb 14, 2019

View reviewed 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 reviewed 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 reviewed 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 reviewed 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 reviewed changes

Comment thread Python/pystate.c Outdated

Update pystate.c

ad0cf0c

methane approved these changes Feb 15, 2019

View reviewed changes

vstinner reviewed Feb 18, 2019

View reviewed 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

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

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

Uh oh!

Uh oh!

Conversation

matrixise commented Feb 14, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matrixise Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulienPalard left a comment

Choose a reason for hiding this comment

Uh oh!

eamanu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Feb 14, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 14, 2019

Uh oh!

JulienPalard commented Feb 14, 2019

Uh oh!

matrixise commented Feb 14, 2019

Uh oh!

matrixise commented Feb 14, 2019

Uh oh!

JulienPalard left a comment

Choose a reason for hiding this comment

Uh oh!

matrixise commented Feb 14, 2019

Uh oh!

vstinner commented Feb 14, 2019

Uh oh!

matrixise commented Feb 14, 2019

Uh oh!

vstinner commented Feb 14, 2019

Uh oh!

eamanu commented Feb 14, 2019

Uh oh!

Uh oh!

matrixise commented Feb 15, 2019

Uh oh!

methane commented Feb 15, 2019

Uh oh!

matrixise commented Feb 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matrixise commented Feb 20, 2019

Uh oh!

miss-islington commented Feb 20, 2019

Uh oh!

miss-islington commented Feb 20, 2019

Uh oh!

vstinner commented Feb 20, 2019

matrixise commented Feb 14, 2019 •

edited by bedevere-bot

Loading

matrixise Feb 14, 2019 •

edited

Loading