Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

gh-107545: Fix misleading setsockopt error message #107546

wants to merge 5 commits into from

Conversation

naweiss
Copy link

@naweiss naweiss commented Aug 1, 2023 •
edited by bedevere-bot
Loading

According to Python's documentation, setsockopt can get either int, buffer or None as the first arguments.
The way it is implemented currently is:

cpython/Modules/socketmodule.c

Lines 3169 to 3190 in f01e4ce

if (PyArg_ParseTuple(args, "iii:setsockopt",
&level, &optname, &flag)) {
res = setsockopt(s->sock_fd, level, optname,
(char*)&flag, sizeof flag);
goto done;
}
PyErr_Clear();
/* setsockopt(level, opt, None, flag) */
if (PyArg_ParseTuple(args, "iiO!I:setsockopt",
&level, &optname, Py_TYPE(Py_None), &none, &optlen)) {
assert(sizeof(socklen_t) >= sizeof(unsigned int));
res = setsockopt(s->sock_fd, level, optname,
NULL, (socklen_t)optlen);
goto done;
}
PyErr_Clear();
/* setsockopt(level, opt, buffer) */
if (!PyArg_ParseTuple(args, "iiy*:setsockopt",
&level, &optname, &optval))
return NULL;

So, we first try to parse all of the three arguments as ints, if this fails no matter what the reason is we just try the next overload. So since 2**31 causes PyExc_OverflowError exception, and the buffer overload is last you get type error for the third argument instead (e.g. TypeError: a bytes-like object is required, not 'int').

@bedevere-bot bedevere-bot mentioned this pull request Aug 1, 2023
Open
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Aug 1, 2023 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@naweiss
Copy link
Author

naweiss commented Aug 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 that PyErr_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 of setsockopt they will all fail and you get the error message from the buffer overload. when you could report the error immediately after the first overload failure.

Error message for:

import socket

with socket.socket() as s:
    s.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 2 ** 31)

is now OverflowError: signed integer is greater than maximum

@CharlieZhao95
Copy link
Contributor

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

naweiss and others added 2 commits August 3, 2023 21:38
@naweiss @CharlieZhao95
Update gh-issue-107545 NEWS
377701d 
Co-authored-by: Charlie Zhao <zhaoyu_hit@qq.com>
@naweiss
Add tests for setsockopt errors
ddcdb0d
@naweiss
Copy link
Author

naweiss commented Aug 6, 2023

I added the relevant tests. Any new review comments?

@CharlieZhao95
Copy link
Contributor

CharlieZhao95 commented Aug 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-bot bot commented Apr 18, 2025 •
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

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

@CharlieZhao95 CharlieZhao95 CharlieZhao95 left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@naweiss @bedevere-bot @CharlieZhao95