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.

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

Merged
merged 11 commits into from
Aug 5, 2025

Conversation

naweiss
Copy link
Contributor

@naweiss naweiss commented Aug 1, 2023

According to Python's documentation, setsockopt can get either int, buffer or None as the third argument.
The way it is currently implemented is by trying to parse all of the three arguments as ints, if this fails no matter what the reason is we just try the next overload.
Since 2**31 causes PyExc_OverflowError we try the next overloads. Because the buffer overload is last we get type error for the third argument instead of OverflowError (e.g. TypeError: a bytes-like object is required, not 'int').

@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

All commit authors signed the Contributor License Agreement.
CLA signed

@naweiss
Copy link
Contributor Author

naweiss commented Aug 1, 2023

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
Copy link
Contributor Author

naweiss commented Aug 6, 2023

I added the relevant tests. Any new review comments?

@CharlieZhao95
Copy link
Contributor

CharlieZhao95 commented Aug 7, 2023

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

All commit authors signed the Contributor License Agreement.

CLA signed

@naweiss naweiss force-pushed the fix-issue-107545 branch from f712b65 to 6439014 Compare June 23, 2025 21:12
@naweiss naweiss force-pushed the fix-issue-107545 branch from 6439014 to 167d212 Compare June 23, 2025 21:22
@naweiss
Copy link
Contributor Author

naweiss commented Jun 25, 2025

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.

Copy link
Member

@serhiy-storchaka serhiy-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() and PyBuffer_Check() for the third argument. This will allow to generate better error messages like "... should be integer, bytes-like object or None".

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

naweiss commented Jul 7, 2025

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

@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.

@naweiss naweiss requested a review from serhiy-storchaka July 20, 2025 12:08
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka: Would you mind to review the latest version of this change?

Copy link
Member

@serhiy-storchaka serhiy-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. 👍

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) August 5, 2025 09:52
@serhiy-storchaka
Copy link
Member

Thank you for your PR, @naweiss. After merging it, would you mind to create an issue and a PR for unification for
AF_VSOCK?

@serhiy-storchaka serhiy-storchaka merged commit a50822f into python:main Aug 5, 2025
41 checks passed
@miss-islington-app
Copy link

Thanks @naweiss for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 5, 2025
…-107546)

(cherry picked from commit a50822f)

Co-authored-by: naweiss <naweiss@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 5, 2025

GH-137411 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Aug 5, 2025
@naweiss
Copy link
Contributor Author

naweiss commented Aug 5, 2025

Thank you for your PR, @naweiss. After merging it, would you mind to create an issue and a PR for unification for
AF_VSOCK?

No problem. Does it really make sense to have all overloads for AF_VSOCK?

@serhiy-storchaka
Copy link
Member

In general, buffer and None, length would be enough for all cases. But most option values have type int, and it is more convenient to pass just optval instead of optval.to_bytes(4, sys.byteorder, signed=True). It seems that for AF_VSOCK options have 64-bit values, so a new special case was added. But you should always be able to pass a buffer of arbitrary length.

And I think that it will make the code simpler.

@vstinner
Copy link
Member

vstinner commented Aug 5, 2025

Congrats @naweiss for your change!

Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants