FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-13097: fix segfault in ctypes callback invocation by swgillespie · Pull Request #19914 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-13097: fix segfault in ctypes callback invocation - #19914

Merged
vstinner merged 6 commits into
python:masterfrom
swgillespie:swgillespie/bpo-13097
May 27, 2020
ConversationCommits (6)ChecksFiles changed
Merged

bpo-13097: fix segfault in ctypes callback invocation#19914
vstinner merged 6 commits into
python:masterfrom
swgillespie:swgillespie/bpo-13097

Conversation

swgillespie commented May 5, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

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.

https://bugs.python.org/issue13097

bpo-13097: fix segfault in ctypes callback invocation
5e9ef9a 
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.

the-knights-who-say-ni commented May 5, 2020

Copy link
Copy Markdown

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 username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@swgillespie

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 requested changes May 6, 2020
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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().

Comment thread Lib/ctypes/test/test_callbacks.py Outdated
Comment thread Lib/ctypes/test/test_callbacks.py Outdated
Comment thread Lib/ctypes/test/test_callbacks.py Outdated
Comment thread Modules/_ctypes/callproc.c Outdated
Comment thread Modules/_ctypes/callproc.c Outdated

bedevere-bot commented May 6, 2020

Copy link
Copy Markdown

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.

Apply suggestions from code review
062c786 
Co-authored-by: Victor Stinner <vstinner@python.org>

swgillespie commented May 6, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

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().

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 argcount is greater than the limit. I'm happy to do that, but I personally didn't think the complexity was worth it for such a niche use case.

swgillespie commented May 6, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

bedevere-bot commented May 6, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

bedevere-bot requested a review from vstinner May 6, 2020 17:43

swgillespie commented May 22, 2020

Copy link
Copy Markdown
Contributor Author

Is this PR OK to merge? @vstinner

vstinner approved these changes May 27, 2020
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

vstinner merged commit 29a1384 into python:master May 27, 2020

miss-islington commented May 27, 2020

Copy link
Copy Markdown
Contributor

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

bedevere-bot commented May 27, 2020

Copy link
Copy Markdown

GH-20453 is a backport of this pull request to the 3.9 branch.

miss-islington commented May 27, 2020

Copy link
Copy Markdown
Contributor

Thanks @swgillespie for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2020
bpo-13097: ctypes: limit callback to 1024 arguments (pythonGH-19914)
5829125 
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>

bedevere-bot commented May 27, 2020

Copy link
Copy Markdown

GH-20454 is a backport of this pull request to the 3.8 branch.

miss-islington commented May 27, 2020

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2020
bpo-13097: ctypes: limit callback to 1024 arguments (pythonGH-19914)
06a0ed1 
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>

bedevere-bot commented May 27, 2020

Copy link
Copy Markdown

GH-20455 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request May 27, 2020
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
788d7bf 
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>
miss-islington added a commit that referenced this pull request May 27, 2020
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
1c4dcaf 
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>
miss-islington added a commit that referenced this pull request May 27, 2020
bpo-13097: ctypes: limit callback to 1024 arguments (GH-19914)
a285af7 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL