Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-107545: Fix misleading setsockopt error message#107546

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
naweiss wants to merge8 commits intopython:main
base:main
Choose a base branch
Loading
fromnaweiss:fix-issue-107545

Conversation

naweiss
Copy link

@naweissnaweiss commentedAug 1, 2023•
edited
Loading

According to Python's documentation,setsockopt can get eitherint,buffer orNone as the third argument.
The way it is currently implemented is by trying to parse all of the three arguments as ints, if this failsno matter what the reason is we just try the next overload.
Since2**31 causesPyExc_OverflowError we try the next overloads. Because thebuffer overload is last we get type error for the third argument instead ofOverflowError (e.g.TypeError: a bytes-like object is required, not 'int').

CharlieZhao95 reacted with thumbs up emoji
@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@ghost
Copy link

ghost commentedAug 1, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@naweiss
Copy link
Author

naweiss commentedAug 1, 2023•
edited
Loading

I choose to solve the issue using a white-list approach rather then a black-list approach since there might be more errors thatPyErr_Clear will silently suppress. For example, currently, if the type of the first argument is incorrect you still need to keep checking all of the overloads ofsetsockopt they will all fail and you get the error message from thebuffer overload. when you could report the error immediately after the first overload failure.

Error message for:

importsocketwithsocket.socket()ass:s.setsockopt(socket.SOL_SOCKET,socket.SO_RCVBUF,2**31)

is nowOverflowError: signed integer is greater than maximum

@CharlieZhao95
Copy link
Contributor

Looks good, how about adding some tests for new error messages.

naweiss reacted with thumbs up emoji

@naweiss
Copy link
Author

I added the relevant tests. Any new review comments?

@CharlieZhao95
Copy link
Contributor

CharlieZhao95 commentedAug 7, 2023•
edited
Loading

I added the relevant tests. Any new review comments?

Considering this is your first PR in cpython, please be patient! The next step is waiting for the review by core devs.

Generally, core developers don't have enough time to review every PR, so you can see there are many PRs that can't be merged into the main. Before they come to review, we just need to keep our PR compliant and wait :)

@python-cla-bot
Copy link

python-cla-botbot commentedApr 18, 2025•
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

@naweiss
Copy link
Author

Since this PR is old, I validated that the issue is still reproducible.
Than rebased the branch onto main, resolved any conflicts, and confirmed that all tests pass.
I also did some minor changes to make the PR files a little cleaner and follow conventions better.

serhiy-storchaka reacted with thumbs up emoji

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, but I think that we can get rid of_testcapi.

The more user friendly approach is to parse arguments as"iiOO|O" and then use== Py_None,PyIndex_Check() andPyBuffer_Check() for the third argument. This will allow to generate better error messages like "... should be integer, bytes-like object or None".

@unittest.skipIf(_testcapi is None, "requires _testcapi")
def test_setsockopt_errors(self):
# See issue #107546.
from _testcapi import INT_MAX, INT_MIN

Choose a reason for hiding this comment

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

We could just use some large values like2**1000 and-2**1000.

Copy link
Member

Choose a reason for hiding this comment

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

I'm scared by big numbers,2**100 and-2**100 should be enough to trigger OverflowError on all platforms :-)

Choose a reason for hiding this comment

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

But not on platforms with 128-bitint. I often use2**1000 as more future proof option.

Copy link
Member

Choose a reason for hiding this comment

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

Does Python support any platform with 128-bit integer? Does such platform exist?

@serhiy-storchakaserhiy-storchaka added the needs backport to 3.14bugs and security fixes labelJul 7, 2025
@naweiss
Copy link
Author

Thanks for the feedback.
I would be happy for guidance regarding theAF_VSOCK family.
The old code supported only an integer option, is this the expected behavior?

@@ -3338,26 +3338,32 @@ sock_setsockopt(PyObject *self, PyObject *args)
Py_buffer optval;
int flag;
unsigned int optlen;
PyObject *none;
PyObject *type;

Choose a reason for hiding this comment

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

Semantically, this is not type, but value.

Copy link
Author

@naweissnaweissJul 8, 2025•
edited
Loading

Choose a reason for hiding this comment

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

I choose the nameoptval for all cases which access the value of the option.

with self.assertRaises(OverflowError):
sock.setsockopt(2 ** 100, socket.SO_REUSEADDR, 1)

with self.assertRaises(TypeError):

Choose a reason for hiding this comment

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

You can test the error message as well.

Copy link
Author

Choose a reason for hiding this comment

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

Good Idea, I also created some more type checks and added their exact message to the tests code.

@serhiy-storchaka
Copy link
Member

Thank you for your update,@naweiss. Here is the next portion of comments.

@serhiy-storchaka
Copy link
Member

The old code supported only an integer option, is this the expected behavior?

I noticed this too. We need to look at the history of the code. Anyway, this is a different issue.

if (!PyArg_ParseTuple(args, "iiO:setsockopt",
&level, &optname, &optval)) {
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move this test for None and 4 arguments here?

if (optval == Py_None) { PyErr_Format(...) }

naweiss reacted with thumbs up emoji
}
break;
case 4:
if (!PyArg_ParseTuple(args, "iiO!I:setsockopt",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!PyArg_ParseTuple(args,"iiO!I:setsockopt",
if (!PyArg_ParseTuple(args,"iiOI:setsockopt",

Copy link
Author

Choose a reason for hiding this comment

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

This ensures that the 3rd argument would be of the specified type.
In my casePy_TYPE(Py_None) that way I ensure that if we have 4 arguments the 3rd must beNone.

@@ -3374,36 +3397,45 @@ sock_setsockopt(PyObject *self, PyObject *args)
goto done;
}

PyErr_Clear();
/* setsockopt(level, opt, None, flag) */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* setsockopt(level, opt, None,flag) */
/* setsockopt(level, opt, None,optlen) */

naweiss reacted with thumbs up emoji
Comment on lines 3427 to 3428
res = setsockopt(get_sock_fd(s), level, optname,
buffer.buf, (int)buffer.len);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
res=setsockopt(get_sock_fd(s),level,optname,
buffer.buf, (int)buffer.len);
res=setsockopt(get_sock_fd(s),level,optname,
buffer.buf, (int)buffer.len);

naweiss reacted with thumbs up emoji
&level, &optname, Py_TYPE(Py_None), &optval, &optlen)) {
return NULL;
}
break;

Choose a reason for hiding this comment

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

We needif (optval != Py_None) {error...} here.

Copy link
Author

@naweissnaweissJul 11, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Look athttps://github.com/python/cpython/pull/107546/files#r2192102447.
The "O!" format along withPy_TYPE(Py_None) ensures that the type of the 3rd argument would beNone.

sock.setsockopt(2 ** 100, socket.SO_REUSEADDR, 1)

msg = "socket option should be should be integer, bytes-like object or None"
with self.assertRaises(TypeError, msg=msg):

Choose a reason for hiding this comment

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

It should beassertRaisesRegex. And the expected error message is a regular expression, so you need to escape parentheses.

naweiss reacted with thumbs up emoji
arglen = PyTuple_Size(args);
if (arglen == 3 && optval == Py_None) {
PyErr_Format(PyExc_TypeError,
"setsockopt() take 4 arguments when socket option is None (%zd given)",

Choose a reason for hiding this comment

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

Would not the following be better: "setsockopt() requires 4 arguments when the third argument is None"?

naweiss reacted with thumbs up emoji
}
if (arglen == 4 && optval != Py_None) {
PyErr_Format(PyExc_TypeError,
"setsockopt() argument 3 must be NoneType, not %T",

Choose a reason for hiding this comment

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

"setsockopt() takes 3 arguments when the third argument is not None" or "setsockopt() only takes 4 arguments when the third argument is None"?

naweiss reacted with thumbs up emoji
PyObject *none;
PyObject *optval;

if (!PyArg_ParseTuple(args, "iiO|I:setsockopt",

Choose a reason for hiding this comment

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

All this should be after the code for AF_VSOCK. Otherwise we will get wrong error messages for AF_VSOCK.

Copy link
Author

@naweissnaweissJul 15, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Thedocs for setsockopt state there are 3 different overloads and does not mention anything different forAF_VSOCK.
So, I think that the old error messages where misleading:

>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, 1, 2, 3)TypeError: setsockopt() takes exactly 3 arguments (5 given)>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, None, 1)TypeError: setsockopt() takes exactly 3 arguments (4 given)>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, b"aaa")TypeError: setsockopt() argument 3 must be int, not bytes

It weird that suddenly the error message tell you that setsockopt only supports 3 arguments although there is overload with 4. I personally would expect thatAF_VSOCK would still allowbuffer/None argument (like in my implementation) but would raise an error telling the user that this are unsupported types specifically forAF_VSOCK. Something like this:

>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, 1, 2, 3)TypeError: setsockopt() takes at most 4 arguments (5 given)>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, None, 1)TypeError: setsockopt() argument 3 for AF_VSOCK must only be an int (got None)>>> s.setsockopt(socket.AF_VSOCK, socket.SO_REUSEADDR, b"aaa")TypeError: setsockopt() argument 3 for AF_VSOCK must only be an int (got bytes)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@CharlieZhao95CharlieZhao95CharlieZhao95 left review comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@naweiss@bedevere-bot@CharlieZhao95@serhiy-storchaka@vstinner
[8]ページ先頭
©2009-2025 Movatter.jp