Skip to content
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

sockmodule.c: sock_connect vs negative errno values... #84808

Closed
rcgordon mannequin opened this issue May 14, 2020 · 5 comments
Closed

sockmodule.c: sock_connect vs negative errno values... #84808

rcgordon mannequin opened this issue May 14, 2020 · 5 comments
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@rcgordon
Copy link
Mannequin

rcgordon mannequin commented May 14, 2020

BPO 40628
Nosy @rcgordon

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-05-14.18:55:16.943>
labels = ['extension-modules', 'type-bug', '3.7']
title = 'sockmodule.c: sock_connect vs negative errno values...'
updated_at = <Date 2020-05-14.18:55:16.943>
user = 'https://github.com/rcgordon'

bugs.python.org fields:

activity = <Date 2020-05-14.18:55:16.943>
actor = 'icculus'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2020-05-14.18:55:16.943>
creator = 'icculus'
dependencies = []
files = []
hgrepos = []
issue_num = 40628
keywords = []
message_count = 1.0
messages = ['368863']
nosy_count = 1.0
nosy_names = ['icculus']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue40628'
versions = ['Python 3.7']

Linked PRs

@rcgordon
Copy link
Mannequin Author

rcgordon mannequin commented May 14, 2020

(Forgive any obvious mistakes in this report, I'm almost illiterate with Python, doubly so with Python internals.)

In trying to get buildbot-worker running on Haiku ( https://haiku-os.org/ ), it runs into a situation where it tries to connect a non-blocking TCP socket, which correctly reports EINPROGRESS, and cpython/Modules/sockmodule.c's internal_connect() returns this error code to sock_connect() and sock_connect_ex().

Both of the sock_connect* functions will return NULL if the error code is negative, but on Haiku, all the errno values are negative (EINPROGRESS, for example, is -2147454940).

I _think_ what sock_connect is intending to do here...

    res = internal_connect(s, SAS2SA(&addrbuf), addrlen, 1);
    if (res < 0)
        return NULL;

...is say "if we had a devastating and unexpected system error, give up immediately." Buildbot-worker seems to confirm this by throwing this exception in response:

builtins.SystemError: <method 'connect_ex' of '_socket.socket' objects> returned NULL without setting an error

internal_connect returns -1 in those devastating-and-unexpected cases--namely when an exception is to be raised--and does not ever use that to otherwise signify a legit socket error. Linux and other systems don't otherwise fall into this "res < 0" condition because errno values are positive on those systems.

So I believe the correct fix here, in sock_connect() and sock_connect_ex(), is to check "if (res == -1)" instead of "res < 0" and let all other negative error codes carry on.

If this seems like the correct approach, I can assemble a pull request, but I don't know the full ramifications of this small change, so I thought I'd report it here first.

--ryan.

@rcgordon rcgordon mannequin added 3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels May 14, 2020
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error 3.7 (EOL) end of life labels May 14, 2023
@iritkatriel
Copy link
Member

Since Haiku is not a supported platform, I'm relabelling this from bug to feature request. I don't know how likely it is that it will be implemented though, there would need to be a core developer who cares enough about Haiku to do something non-standard for it (an assess what the implications of that may be).

@encukou
Copy link
Member

encukou commented May 15, 2023

IMO, Python should not assume that errno codes are positive. The point of PEP-11 is to not maintain speedups/workarounds for niche platforms, not to overfit CPython to rely on quirks of the tiered platforms.
To me, this is a legitimate bug, though one that core devs won't fix alone, and might not prioritize.

Please send the PR. Considering the ramifications of the change is the reviewer's job.

@erlend-aasland
Copy link
Contributor

I agree with Petr; this definitely sounds like a bug, not a feature.

@arhadthedev arhadthedev added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels May 16, 2023
korli added a commit to korli/cpython that referenced this issue Jul 26, 2024
Python shouldn't assume that errno codes are positive.

reported and proposed by Ryan C. Gordon
korli added a commit to korli/cpython that referenced this issue Jul 27, 2024
Python shouldn't assume that errno codes are positive.

reported and proposed by Ryan C. Gordon
korli added a commit to korli/cpython that referenced this issue Jul 27, 2024
internal_connect() errors when returning -1 with exception set.

reported and proposed by Ryan C. Gordon
korli added a commit to korli/cpython that referenced this issue Sep 6, 2024
internal_connect() errors when returning -1 with exception set.

reported and proposed by Ryan C. Gordon
error handling scheme (raise, no raise) as suggested by encukou

Co-authored-by: Petr Viktorin <encukou@gmail.com>
encukou pushed a commit that referenced this issue Sep 9, 2024
POSIX allows errno to be negative.
Even though all currently supported platforms have non-negative errno,
relying on a quirk like that would make Python less portable.
@encukou
Copy link
Member

encukou commented Sep 9, 2024

Thank you rcgordon for the report, and @korli for the fix!

@encukou encukou closed this as completed Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants