-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-143637: Fix re-entrant mutation of ancillary data in socket.sendmsg() #143892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
597c672
7e57097
a6fbd4b
be7371d
4703c2b
2bd7e14
710daaa
a2992a8
75d8ae9
6f0ddf4
ff8af98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fixed a crash in socket.sendmsg() that could occur if ancillary data is mutated re-entrantly during argument parsing. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4977,11 +4977,13 @@ _socket_socket_sendmsg_impl(PySocketSockObject *s, PyObject *data_arg, | |
| if (cmsg_arg == NULL) | ||
| ncmsgs = 0; | ||
| else { | ||
| if ((cmsg_fast = PySequence_Fast(cmsg_arg, | ||
| "sendmsg() argument 2 must be an " | ||
| "iterable")) == NULL) | ||
| cmsg_fast = PySequence_Tuple(cmsg_arg); | ||
| if (cmsg_fast == NULL) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "sendmsg() argument 2 must be an iterable"); | ||
| goto finally; | ||
| ncmsgs = PySequence_Fast_GET_SIZE(cmsg_fast); | ||
| } | ||
| ncmsgs = PyTuple_GET_SIZE(cmsg_fast); | ||
| } | ||
|
|
||
| #ifndef CMSG_SPACE | ||
|
|
@@ -5001,20 +5003,21 @@ _socket_socket_sendmsg_impl(PySocketSockObject *s, PyObject *data_arg, | |
| controllen = controllen_last = 0; | ||
| while (ncmsgbufs < ncmsgs) { | ||
| size_t bufsize, space; | ||
| PyObject *item; | ||
|
|
||
| if (!PyArg_Parse(PySequence_Fast_GET_ITEM(cmsg_fast, ncmsgbufs), | ||
| "(iiy*):[sendmsg() ancillary data items]", | ||
| &cmsgs[ncmsgbufs].level, | ||
| &cmsgs[ncmsgbufs].type, | ||
| &cmsgs[ncmsgbufs].data)) | ||
| item = PyTuple_GET_ITEM(cmsg_fast, ncmsgbufs); | ||
|
|
||
| if (!PyArg_Parse(item, | ||
| "(iiy*):[sendmsg() ancillary data items]", | ||
| &cmsgs[ncmsgbufs].level, | ||
| &cmsgs[ncmsgbufs].type, | ||
| &cmsgs[ncmsgbufs].data)){ | ||
| goto finally; | ||
| } | ||
|
|
||
| bufsize = cmsgs[ncmsgbufs++].data.len; | ||
|
|
||
| #ifdef CMSG_SPACE | ||
| if (!get_CMSG_SPACE(bufsize, &space)) { | ||
| #else | ||
| if (!get_CMSG_LEN(bufsize, &space)) { | ||
| #endif | ||
| if(!get_CMSG_SPACE(bufsize, &space)){ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change??
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This value is only used to decide how much memory to allocate for msg.msg_control.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function is not necessarily present though?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| PyErr_SetString(PyExc_OSError, "ancillary data item too large"); | ||
| goto finally; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a private method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a helper method rather than a standalone test because it depends on the surrounding test class setup and socket lifecycle, and is not meant to be picked up directly by unittest discovery.
The skipUnless(sendmsg) decorator is placed here so the helper only runs on platforms that support sendmsg(),with execution still controlled by the enclosing test logic.
I can add a small public test_* wrapper calling this helper if that’s prefferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But where is it tested then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, you are right, I have missed it- I have added a public test method that invokes
_test_sendmsg_reentrant_ancillary_mutationin my updated PR is it correct?, Thanks for the clarification.