classification
Title: [easy C issue] ctypes: segfault with large number of callback arguments
Type: crash Stage: resolved
Components: ctypes Versions: Python 3.10, Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: meador.inge Nosy List: amaury.forgeotdarc, belopolsky, furkanonder, meador.inge, miss-islington, python-dev, vstinner
Priority: low Keywords: easy (C), newcomer friendly, patch

Created on 2011-10-04 03:57 by meador.inge, last changed 2020-05-27 15:54 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19914 merged python-dev, 2020-05-05 05:06
PR 20453 merged miss-islington, 2020-05-27 15:22
PR 20454 merged miss-islington, 2020-05-27 15:23
PR 20455 merged miss-islington, 2020-05-27 15:23
Messages (13)
msg144852 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-10-04 03:57
Reproducible in 2.7 and tip:

[meadori@motherbrain cpython]$ ./python 
Python 3.3.0a0 (default:61de28fa5537+d05350c14e77+, Oct  3 2011, 21:47:04) 
[GCC 4.6.0 20110603 (Red Hat 4.6.0-10)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *
>>> NARGS = 2 ** 20
>>> proto = CFUNCTYPE(None, *(c_int,) * NARGS)
>>> def func(*args):
...    return (1, "abc", None)
... 
>>> cb = proto(func)
>>> cb(*(1,) * NARGS)
Segmentation fault (core dumped)
msg145259 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-10-09 14:37
As mentioned in issue12881, this issue is a result of an unbounded 'alloca' call that trashes the stack.
msg148648 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-11-30 12:20
Right, alloca() could be replaced by some malloc(), but is it really useful?  After all, when a C function calls back to Python, all arguments needs to be pushed to the stack anyway.
msg148701 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-12-01 04:24
On Wed, Nov 30, 2011 at 6:20 AM, Amaury Forgeot d'Arc
<report@bugs.python.org> wrote:

> Right, alloca() could be replaced by some malloc(), but is it really useful?  After all, when a C function calls back to Python, all arguments needs to be > pushed to the stack anyway.

The case is somewhat pathological.  However, there are *four* 'alloca'
calls in '_ctypes_callproc', three of which are proportional to
'argcount'.  And as you point out there is an additional stack
allocation inside of 'libffi' when the callback is actually called
(also using 'alloca').

I see two reasons switching to 'malloc' might be beneficial: (1) by
shifting some of the allocation to dynamic allocation we may reduce
the chance that we blow the stack at all.  Keep in mind that this gets
more likely if the C ffi function is called from a deep in a call tree
and *not* necessarily with just a huge number of parameters.  (2) if
resources are exhausted, then we can exit more gracefully.  Out of
dynamic memory errors can actually be handled as opposed to an
'alloca' call that ends up allocating more than is left.

That being said, if this does get changed it is low priority.
msg148712 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-12-01 08:11
Is there really an use case where you need 2 ** 20 (1,048,576) arguments? If yes, I'm not against the torture in this case :-) If no, why not raising an error if there are too many arguments? E.g. limit to 1,024 arguments or maybe just 10?
msg149508 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-12-15 05:05
On Thu, Dec 1, 2011 at 2:11 AM, STINNER Victor <report@bugs.python.org> wrote:

> Is there really an use case where you need 2 ** 20 (1,048,576) arguments? If yes, I'm not against the torture in this case :-)

Not very likely :-)  However, the segfault can occur with less
arguments in low stack space situations (e.g. deep callstack).

> If no, why not raising an error if there are too many arguments? E.g. limit to 1,024 arguments or maybe just 10?

That is certainly an option.
msg368001 - (view) Author: Furkan Onder (furkanonder) * Date: 2020-05-03 23:50
The issue continues in python 3.8.2.
msg368064 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-04 16:32
I suggest to raise an exception if it's called with more than 1024 arguments.
msg370093 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-27 15:22
New changeset 29a1384c040d39659e7d01f1fd7b6eb71ef2634e by Sean Gillespie in branch 'master':
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
https://github.com/python/cpython/commit/29a1384c040d39659e7d01f1fd7b6eb71ef2634e
msg370099 - (view) Author: miss-islington (miss-islington) Date: 2020-05-27 15:47
New changeset 788d7bfe189e715eab3855c20ea5d6da0d8bed70 by Miss Islington (bot) in branch '3.9':
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
https://github.com/python/cpython/commit/788d7bfe189e715eab3855c20ea5d6da0d8bed70
msg370102 - (view) Author: miss-islington (miss-islington) Date: 2020-05-27 15:51
New changeset 1c4dcafd0b025e771f4dbd7197d0b5f263c9cb54 by Miss Islington (bot) in branch '3.7':
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
https://github.com/python/cpython/commit/1c4dcafd0b025e771f4dbd7197d0b5f263c9cb54
msg370103 - (view) Author: miss-islington (miss-islington) Date: 2020-05-27 15:53
New changeset a285af7e626d1b81cf09f8b2bf7656f100bc1237 by Miss Islington (bot) in branch '3.8':
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
https://github.com/python/cpython/commit/a285af7e626d1b81cf09f8b2bf7656f100bc1237
msg370105 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-27 15:54
Thanks Meador Inge for the bug report and thanks Sean Gillespie for the fix! It just took 9 years to fix this corner case ;-)

Copy of the comment on the PR:
https://github.com/python/cpython/pull/19914#pullrequestreview-419331432

I tried to rewrite _ctypes_callproc() to use PyMem_Malloc() instead of alloca(), but it's quite complicated. There are 3 arrays with a length of argcount items: args, avalues, atypes. Moreover, resbuf is also allocated with alloca(). When using PyMem_Malloc(), error handling is much more complicated.

I also tried to restrict the overall usage of stack memory to 4096 bytes (size of one page on x86), but users would be surprised by CTYPES_MAX_ARGCOUNT value.

I would say that raising an exception is better than crashing for a lot of arguments. If someone is blocked by this new limitation, in that case we can revisit the PyMem_Malloc() idea.
History
Date User Action Args
2020-05-27 15:54:46vstinnersetstatus: open -> closed
versions: + Python 3.7, Python 3.8, Python 3.9, Python 3.10, - Python 2.7, Python 3.2, Python 3.3
messages: + msg370105

resolution: fixed
stage: patch review -> resolved
2020-05-27 15:53:11miss-islingtonsetmessages: + msg370103
2020-05-27 15:51:31miss-islingtonsetmessages: + msg370102
2020-05-27 15:47:04miss-islingtonsetmessages: + msg370099
2020-05-27 15:23:35miss-islingtonsetpull_requests: + pull_request19710
2020-05-27 15:23:15miss-islingtonsetpull_requests: + pull_request19709
2020-05-27 15:22:34miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request19708
2020-05-27 15:22:11vstinnersetmessages: + msg370093
2020-05-05 05:06:02python-devsetkeywords: + patch
nosy: + python-dev

pull_requests: + pull_request19229
stage: needs patch -> patch review
2020-05-04 16:32:25vstinnersetkeywords: + easy (C), newcomer friendly

messages: + msg368064
title: ctypes: segfault with large number of callback arguments -> [easy C issue] ctypes: segfault with large number of callback arguments
2020-05-03 23:50:55furkanondersetnosy: + furkanonder
messages: + msg368001
2011-12-15 05:05:02meador.ingesetmessages: + msg149508
2011-12-01 08:11:10vstinnersetmessages: + msg148712
2011-12-01 04:24:55meador.ingesetpriority: normal -> low
2011-12-01 04:24:09meador.ingesetmessages: + msg148701
2011-11-30 12:20:03amaury.forgeotdarcsetmessages: + msg148648
2011-11-30 12:17:44vstinnersetnosy: + vstinner
2011-11-29 02:34:19meador.ingesetassignee: meador.inge
2011-10-09 14:37:05meador.ingesetmessages: + msg145259
2011-10-04 03:57:17meador.ingecreate