bpo-13097: fix segfault in ctypes callback invocation#19914
Conversation
The ctypes module allocates arguments on the stack in `ctypes_callproc` using alloca, which is problematic when large numbers of parameters are passed. Instead of crashing, this commit raises an ArgumentError if more than 1024 parameters are passed.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Hum. This approach makes sense.
But. There is another way. If sizeof(struct argument) * argcount is larger than 4 KB, we can allocate memory on the heap, instead of using alloca().
|
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 |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Thanks for the review!
It's true, but I also don't think there's ever a legitimate reason for a callback to have this many arguments. There's some additional complexity involved when freeing the argument buffer in this case, since we'd need to only free it when |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
|
Is this PR OK to merge? @vstinner |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
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.
|
Thanks @swgillespie for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
|
GH-20453 is a backport of this pull request to the 3.9 branch. |
|
Thanks @swgillespie for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
ctypes now raises an ArgumentError when a callback is invoked with more than 1024 arguments. The ctypes module allocates arguments on the stack in ctypes_callproc() using alloca(), which is problematic when large numbers of arguments are passed. Instead of a stack overflow, this commit raises an ArgumentError if more than 1024 parameters are passed. (cherry picked from commit 29a1384) Co-authored-by: Sean Gillespie <sean@swgillespie.me>
|
GH-20454 is a backport of this pull request to the 3.8 branch. |
|
Thanks @swgillespie for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
ctypes now raises an ArgumentError when a callback is invoked with more than 1024 arguments. The ctypes module allocates arguments on the stack in ctypes_callproc() using alloca(), which is problematic when large numbers of arguments are passed. Instead of a stack overflow, this commit raises an ArgumentError if more than 1024 parameters are passed. (cherry picked from commit 29a1384) Co-authored-by: Sean Gillespie <sean@swgillespie.me>
|
GH-20455 is a backport of this pull request to the 3.7 branch. |
ctypes now raises an ArgumentError when a callback is invoked with more than 1024 arguments. The ctypes module allocates arguments on the stack in ctypes_callproc() using alloca(), which is problematic when large numbers of arguments are passed. Instead of a stack overflow, this commit raises an ArgumentError if more than 1024 parameters are passed. (cherry picked from commit 29a1384) Co-authored-by: Sean Gillespie <sean@swgillespie.me>
ctypes now raises an ArgumentError when a callback is invoked with more than 1024 arguments. The ctypes module allocates arguments on the stack in ctypes_callproc() using alloca(), which is problematic when large numbers of arguments are passed. Instead of a stack overflow, this commit raises an ArgumentError if more than 1024 parameters are passed. (cherry picked from commit 29a1384) Co-authored-by: Sean Gillespie <sean@swgillespie.me>
ctypes now raises an ArgumentError when a callback is invoked with more than 1024 arguments. The ctypes module allocates arguments on the stack in ctypes_callproc() using alloca(), which is problematic when large numbers of arguments are passed. Instead of a stack overflow, this commit raises an ArgumentError if more than 1024 parameters are passed. (cherry picked from commit 29a1384) Co-authored-by: Sean Gillespie <sean@swgillespie.me>
5 participants
The ctypes module allocates arguments on the stack in
ctypes_callprocusing alloca, which is problematicwhen large numbers of parameters are passed. Instead
of crashing, this commit raises an ArgumentError if
more than 1024 parameters are passed.
https://bugs.python.org/issue13097