classification
Title: Python/import.c uses a lot of stack space due to MAXPATHLEN
Type: Stage:
Components: Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: brett.cannon, eric.smith, eric.snow, gregory.p.smith, jcea, ncoghlan, python-dev, twouters
Priority: normal Keywords: patch

Created on 2012-03-16 00:42 by gregory.p.smith, last changed 2012-09-29 19:07 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
malloc-import-pathbufs-py27.001.diff gregory.p.smith, 2012-03-16 06:08 review
malloc-import-pathbufs-py27.002.diff gregory.p.smith, 2012-03-16 22:28 review
malloc-import-pathbufs-py27.003.diff gregory.p.smith, 2012-03-16 23:29 review
malloc-import-pathbufs-py32.003.diff gregory.p.smith, 2012-03-17 01:17 review
malloc-import-pathbufs-py32.004.diff gregory.p.smith, 2012-03-18 22:42 review
malloc-import-pathbufs-py27.004.diff gregory.p.smith, 2012-03-18 22:56 review
Messages (20)
msg155981 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-16 00:42
Python/import.c in 2.7 and 3.2 consume a lot of stack space when importing modules.  In stack constrained environments (think: 64k stack) this can cause a crash when you have a large chain of imports.

The bulk of this likely comes from places where a char buf[MAXPATHLEN+1] or similar is declared on the stack in the call chain.  MAXPATHLEN is commonly defined to be in the 256..1024 range depending on the OS.

Changing the code to not put the large buffer on the stack but to malloc and free it instead is a pretty straightforward re-factor.

import is being significantly reworked in 3.3 so I won't consider this issue there until after that settles as it may no longer apply.
msg155993 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-16 06:08
Here's a patch for python 2.7.  test cases pass but it could use review to see if I missed any free()s.
msg156073 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-16 19:38
It looks like MAXPATHLEN is 4096 on our systems.  The offending code that caused a stack overflow segfault shows over 100 Python/import.c function calls in its backtrace.
msg156082 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-03-16 20:46
Cursory glance has the patch LGTM.
msg156091 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-16 22:28
Updated per review (style changes).
msg156100 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2012-03-16 23:13
For the record, as I said in the review of 002.diff: LGTM.
msg156106 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-16 23:29
Updated to use PyErr_NoMemory().  Thanks Antoine!

I'm now working on this for 3.2 as well before I commit.
msg156119 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-17 01:17
attaching the equivalent patch against python 3.2.  that could also use a pair of eyeballs for review.  it should show up as 'patch 4' in the rietveld reviews.
msg156284 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-18 22:42
side by side code review of the 3.2 version revealed some missing PyMem_FREE calls.  patch updated.
msg156286 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-18 22:55
minor corresponding updated to the 2.7 patch as well - Patch 6 in reitveld/review.

The 3.2 patch from the previous comment is Patch 5 in reitveld/review.
msg156289 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-03-18 23:14
New changeset daed636a3536 by Gregory P. Smith in branch '3.2':
Fixes Issue #14331: Use significantly less stack space when importing modules by
http://hg.python.org/cpython/rev/daed636a3536

New changeset ad030571e6c0 by Gregory P. Smith in branch '2.7':
Fixes Issue #14331: Use significantly less stack space when importing modules by
http://hg.python.org/cpython/rev/ad030571e6c0

New changeset 5ad5625715b5 by Gregory P. Smith in branch 'default':
Empty merge; imports rewritten in 3.3. issue #14331 may no longer apply.
http://hg.python.org/cpython/rev/5ad5625715b5
msg156347 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-03-19 15:55
2.7 and 3.2 have been fixed.  I'm keeping this open as a reminder to investigate how 3.3 behaves.  I'll fix it or close it after verifying that.
msg158683 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-04-18 23:28
Patch ad030571e6c0 introduces a compilation warning in Python 2.7. See
<http://www.python.org/dev/buildbot/all/builders/x86%20OpenIndiana%202.7/builds/1034/steps/compile/logs/warnings%20%281%29>
msg158684 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-04-18 23:31
In fact, the compilation warning seems to expose a far more serious issue.
msg158685 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-04-18 23:33
That warning is correct, there's a bug in the code.  but given this is only a bug when PyMem_MALLOC returns NULL I do not expect this to be an issue for anyone who does not already have issues.

Regardless, I'm fixing it.
msg158686 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-04-18 23:42
New changeset b82a471d2c5e by Gregory P. Smith in branch '2.7':
Fix compiler warning related to issue #14331.  harmless.
http://hg.python.org/cpython/rev/b82a471d2c5e
msg158687 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-04-19 00:03
"PyErr_NoMemory()" returns always NULL, but it is declared as returning "PyObject *".

Could we change "return PyErr_NoMemory();" to "PyErr_NoMemory(); return NULL;"?. Maybe with some comment... A cast would silence the compiler but could be unclear. What do you think?

This is not a problem in 3.2 because the function patched returns a "PyObject *".
msg158688 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-04-19 00:06
Gregory patching was faster than my writing :-). Thanks for taking the effort :-).
msg158689 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-04-19 00:07
already fixed, I just manually returned NULL. :)

I suppose we could change PyErr_NoMemory's definition in 3.3 to return a "void *" instead of "PyObject *" but I'd rather not.  In this case the warning caused me to examine the code and determine if it was in fact intended to do the right Python exception raising thing when NULL was returned from this non Python C API function.  In this case it was, but not all code can assume that.
msg171590 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2012-09-29 19:07
This was fixed in April.
History
Date User Action Args
2012-09-29 19:07:02gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg171590
2012-04-19 00:07:58gregory.p.smithsetmessages: + msg158689
2012-04-19 00:06:43jceasetmessages: + msg158688
2012-04-19 00:03:04jceasetmessages: + msg158687
2012-04-18 23:42:19python-devsetmessages: + msg158686
2012-04-18 23:33:48gregory.p.smithsetmessages: + msg158685
2012-04-18 23:31:24jceasetmessages: + msg158684
2012-04-18 23:28:37jceasetnosy: + jcea

messages: + msg158683
versions: + Python 2.7, Python 3.2
2012-03-19 15:55:42gregory.p.smithsetmessages: + msg156347
versions: + Python 3.3, - Python 2.7, Python 3.2
2012-03-18 23:14:30python-devsetnosy: + python-dev
messages: + msg156289
2012-03-18 22:56:06gregory.p.smithsetfiles: + malloc-import-pathbufs-py27.004.diff
2012-03-18 22:55:53gregory.p.smithsetmessages: + msg156286
2012-03-18 22:42:38gregory.p.smithsetfiles: + malloc-import-pathbufs-py32.004.diff

messages: + msg156284
2012-03-17 01:17:43gregory.p.smithsetfiles: + malloc-import-pathbufs-py32.003.diff

messages: + msg156119
2012-03-16 23:29:41gregory.p.smithsetfiles: + malloc-import-pathbufs-py27.003.diff

messages: + msg156106
2012-03-16 23:13:28twouterssetmessages: + msg156100
2012-03-16 22:28:40gregory.p.smithsetfiles: + malloc-import-pathbufs-py27.002.diff

messages: + msg156091
2012-03-16 20:46:22brett.cannonsetmessages: + msg156082
2012-03-16 19:38:19gregory.p.smithsetmessages: + msg156073
2012-03-16 06:08:50gregory.p.smithsetfiles: + malloc-import-pathbufs-py27.001.diff
keywords: + patch
messages: + msg155993
2012-03-16 03:03:54eric.smithsetnosy: + eric.smith
2012-03-16 01:07:50eric.snowsetnosy: + eric.snow
2012-03-16 00:42:28gregory.p.smithcreate