bpo-32331: Fix socket.type on Linux; expose SOCK_TYPE_MASK. #4877

1st1 · 2017-12-15T03:45:17Z

https://bugs.python.org/issue32331

asvetlov · 2017-12-15T05:55:42Z

Doc/library/socket.rst

Please describe what SOCK_TYPE_MASK is and hint on proper its usage.

I'm not even sure we should expose it, since socket.type takes care of it with this PR.

To clarify: socket.type now applies it, so there's no real use case for SOCK_TYPE_MASK anymore.

Yes, I see.
Ok with not exposing the mask, but if it is exposed -- doc should be updated.
Honestly I don't see how the mask can be useful -- at Python level socket type is always masked already.

asvetlov

Looks good but documentation for SOCK_TYPE_MASK need a little polishing.

methane · 2017-12-15T10:00:17Z

+0.5 to not exposing SOCK_TYPE_MASK.
LGTM for code.

And I think we can backport it to Python 3.6.

asvetlov · 2017-12-15T10:03:16Z

Yes, backport makes sense.

vstinner

This PR removes SOCK_CLOEXEC. How I am supposed to get this info? From socket.get_inheritable(sock)?

Same question for SOCK_NONBLOCK: use sock.get_blocking()?

This change is backward incompatible. It cannot go into 3.6.

If sock.sock_type is modified, SOCK_TYPE_MASK is useless.

I am not sure that we want to break the backward compatibility here.

This PR doesn't change the checks made on sock.type. it would be nice to include them to have an idea of how much code is impacted.

I am for example surprised that test_socket aren't updated.

@pitrou: I would like to see know your opinion here.

bedevere-bot · 2017-12-15T10:16:50Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pitrou · 2017-12-15T10:23:11Z

I agree with Victor:

it can't go into 3.6
making the change in 3.7 is contentious

Can there be an other way to solve the issue? Can we for example keep socket.type as it is and add a utility function or a property reporting the "masked" type?

vstinner · 2017-12-15T14:22:45Z

Can we for example keep socket.type as it is and add a utility function or a property reporting the "masked" type?

Would it make sense to add a new read-only attribute, maybe sock.masked_type?

I would prefer to get it directly in socket objects, to not have to import an utility.

1st1 · 2017-12-15T16:37:22Z

Please let's bring this discussion to the issue on b.p.o.

1st1 · 2017-12-16T00:05:55Z

I've rewritten the PR from scratch. Please read the issue for the summary of updates.

1st1 · 2017-12-16T00:39:11Z

There's no docs update; let's focus on the implementation first.

1st1 · 2017-12-16T05:07:50Z

Another full update.

njsmith · 2017-12-16T05:37:30Z

I think the fiddling with SOCK_NONBLOCK can also be removed from the accept function in Lib/socket.py (note: not socketmodule.c).

njsmith · 2017-12-16T05:38:27Z

Also FYI you have test failures.

1st1 · 2017-12-16T05:47:02Z

I think the fiddling with SOCK_NONBLOCK can also be removed from the accept function in Lib/socket.py (note: not socketmodule.c).

Yes. That will auto-close https://bugs.python.org/issue32318

Also FYI you have test failures.

Fixed.

…EXEC on Linux

vadmium · 2017-12-17T21:46:05Z

Doc/library/socket.rst

+   .. versionchanged:: 3.7
+      On Linux: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC` bit
+      flags are applied to *type* they are cleared, and :attr:`socket.type`
+      will not reflect them.


This belongs with the socket.type attribute, not the constructors. It sounds like Python will prevent the caller from passing these flags to Linux, and makes you wonder if socketpair and fromfd are affected.

Actually they will be passed to Linux socket(), but won't be applied to Python sock.type attribute. I'll rephrase this.

vstinner · 2017-12-18T22:09:04Z

Modules/socketmodule.c

+       0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux
+       is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC.
+    */
+    s->sock_type = type & 0xF;


It's not possible to use SOCK_TYPE_MASK?

No, Linux doesn't export it. It's a private macro in 'include/linux/net.h' that doesn't get exported in 'socket.h'.

If it's not exported, maybe it's not a good idea to use it. Can we mask SOCK_NONBLOCK and SOCK_CLOEXEC instead? type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

Or maybe use getsockopt(SOL_SOCKET, SO_TYPE)? But this option would add a syscall.

So SOCK_TYPE_MASK isn't exported. I described why I think it's safe to just use 0xF in the code in the issue, quoting:

Since Linux doesn't export SOCK_TYPE_MASK I decided to hardcode it in socketmodule.c. My reasoning:

a/ It's highly unlikely that Linux will implement 5 more new socket types anytime soon, so 0xF should last for a very long time.

b/ It's more likely than "a/" that they add a new flag like SOCK_CLOEXEC, in which case the "type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)" approach will fail.

So given a/ and b/ I think it's safe to just use 0xF mask on Linux.

If it's not exported, maybe it's not a good idea to use it. Can we mask SOCK_NONBLOCK and SOCK_CLOEXEC instead? type & ~(SOCK_NONBLOCK | SOCK_CLOEXEC)

Alright, let's play it safe. All in all, if Linux adds another 'SOCK_SOMETHING' bit flag, we'll add it to our socket module, and then we can add it to the list of excluded flags.

At least, please don't hardcode a constant. Use something like "#define SOCK_TYPE_MASK 0xf". Maybe add a reference to bpo-32331.

In the module init, we may add the following assertion coming from the Linux kernel to make sure that SOCK_TYPE_MASK removes SOCK_CLOEXEC and SOCK_NONBLOCK flags:

assert(!(SOCK_CLOEXEC & SOCK_TYPE_MASK)); assert(!(SOCK_NONBLOCK & SOCK_TYPE_MASK));

vstinner

The change should be documented at https://docs.python.org/dev/whatsnew/3.7.html#changes-in-the-python-api

1st1 · 2017-12-18T22:25:49Z

@vstinner I've pushed an update addressing your comments.

vstinner · 2017-12-18T22:41:21Z

Modules/socketmodule.c

    s->sock_fd = fd;
    s->sock_family = family;
+
+#ifdef __linux__


See the bpo. The bug is not specific to Linux, FreeBSD is also affected. Maybe also other OSes?

vstinner · 2017-12-18T22:46:59Z

I don't know if the following change is the best option...

+#ifdef SOCK_NONBLOCK
+    s->sock_type = s->sock_type & ~SOCK_NONBLOCK;
+#endif
+#ifdef SOCK_CLOEXEC
+    s->sock_type = s->sock_type & ~SOCK_CLOEXEC;
+#endif

... but at least it works on FreeBSD to clear SOCK_NONBLOCK and SOCK_CLOEXEC in socket.socket constructor:

[haypo@freebsd ~/prog/python/master]$ ./python
Python 3.7.0a3+ (heads/master-dirty:6efcb6d3d5, Dec 18 2017, 23:44:31) 
[Clang 3.8.0 (tags/RELEASE_380/final 262564)] on freebsd11
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK | socket.SOCK_CLOEXEC).type
<SocketKind.SOCK_STREAM: 1>

1st1 · 2017-12-18T22:51:36Z

I don't know if the following change is the best option...

It's better than hardcoding 0xF for sure. We now clear the flags explicitly and only flags that we know we can clear.

vstinner · 2017-12-18T23:54:18Z

Doc/library/socket.rst

       The CAN_ISOTP protocol was added.

+   .. versionchanged:: 3.7
+      Linux-specific: when :const:`SOCK_NONBLOCK` or :const:`SOCK_CLOEXEC`


Linux and FreeBSD and maybe other OSes specific? :-)

vstinner · 2017-12-18T23:54:41Z

Doc/library/socket.rst


+   .. versionchanged:: 3.7
+      The method no longer applies :const:`SOCK_NONBLOCK` flag on
+      :attr:`socket.type` on Linux.


you may drop "on Linux".

vstinner · 2017-12-18T23:54:58Z

Doc/library/socket.rst


+   .. versionchanged:: 3.7
+      The method no longer toggles :const:`SOCK_NONBLOCK` flag on
+      :attr:`socket.type` on Linux.


ditto (on linux)

vstinner · 2017-12-18T23:55:35Z

Doc/whatsnew/3.7.rst

+* On Linux, the :attr:`socket.type <socket.socket.type>` attribute no
+  longer has :const:`socket.SOCK_NONBLOCK` or :const:`socket.SOCK_CLOEXEC`
+  flags applied, so checks like ``if sock.type == socket.SOCK_STREAM``
+  work as expected on all platforms.


You may suggest to use getblocking() to check if the socket is non-blocking.

"On Linux" doesn't include FreeBSD. Maybe just remove "On Linux".

vstinner · 2017-12-18T23:57:12Z

Misc/NEWS.d/next/Library/2017-12-15-23-48-43.bpo-32331.fIg1Uc.rst

@@ -0,0 +1,3 @@
+Fix socket.settimeout() and socket.setblocking() to not change socket.type
+on Linux.  Fix socket.socket() constructor to reset any bit flags applied to
+socket's type.  This change only affects Linux.


and FreeBSD? (or remove the last sentence)

vstinner · 2017-12-18T23:57:47Z

Modules/socketmodule.c

+       not portable.
+
+       0xF is SOCK_TYPE_MASK in include/linux/net.h which on Linux
+       is used to mask off SOCK_NONBLOCK and SOCK_CLOEXEC.


Outdated comment?

1st1 · 2017-12-19T00:06:26Z

@vstinner I've fixed docs/comments.

vstinner · 2017-12-19T00:14:04Z

Lib/socket.py

+                   C.startswith('SOCK_') and
+                   # SOCK_NONBLOCK and SOCK_CLOEXEC are not socket types,
+                   # they are Linux-specific bit flags.
+                   C not in {'SOCK_NONBLOCK', 'SOCK_CLOEXEC'}))


With this change, we loose the nice repr:

socket.SOCK_CLOEXEC
<SocketKind.SOCK_CLOEXEC: 524288>
socket.SOCK_NONBLOCK
<SocketKind.SOCK_NONBLOCK: 2048>

Which problem are you solving with this change?

They are not types of sockets. They are OS specific special flags.

If someone prints members of the SocketKind to lear what kinds of sockets exist, they will see SOCK_NONBLOCK on Linux and will likely be surprised.

Maybe put them in a different enum in this case (SocketFlags?), but I would prefer to keep the nice repr, it eases debugging.

SocketFlags makes sense. I'll remove this change from this PR, we'll need a new one for it.

vstinner

LGTM.

1st1 requested review from asvetlov, methane, pitrou and vstinner December 15, 2017 03:45

the-knights-who-say-ni added the CLA signed label Dec 15, 2017

bedevere-bot added the awaiting merge label Dec 15, 2017

1st1 force-pushed the fix_sock_type branch from 267a57d to 72e2786 Compare December 15, 2017 03:47

asvetlov reviewed Dec 15, 2017

View reviewed changes

asvetlov approved these changes Dec 15, 2017

View reviewed changes

vstinner requested changes Dec 15, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 15, 2017

1st1 force-pushed the fix_sock_type branch from 72e2786 to 90bbae1 Compare December 16, 2017 00:04

1st1 force-pushed the fix_sock_type branch from 90bbae1 to 95bd98d Compare December 16, 2017 05:07

1st1 force-pushed the fix_sock_type branch 2 times, most recently from 4b11ac4 to c1806d6 Compare December 16, 2017 05:46

bpo-32331: Don't obstruct socket.type with SOCK_NONBLOCK and SOCK_CLO…

3d9ca1f

…EXEC on Linux

1st1 force-pushed the fix_sock_type branch from c1806d6 to 3d9ca1f Compare December 16, 2017 05:51

vadmium reviewed Dec 17, 2017

View reviewed changes

Add a comment; fix docs

dae4784

vstinner reviewed Dec 18, 2017

View reviewed changes

Address Victor's feedback

ed40d77

1st1 added 2 commits December 18, 2017 17:27

Another small update of what's new

ee6a024

Don't use ifdef __linux__; reset SOCK_* flags explicitly

8e5bd2b

vstinner reviewed Dec 18, 2017

View reviewed changes

More fixes per Victor's review.

7e9eb53

vstinner reviewed Dec 19, 2017

View reviewed changes

Don't cleanup SocketKind; we'll do it in a separate PR

7b62803

1st1 force-pushed the fix_sock_type branch from cdbc60a to 7b62803 Compare December 19, 2017 00:22

vstinner approved these changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 19, 2017

Fix test_uknown_socket_family_repr test

ad998e4

1st1 merged commit 9818142 into python:master Dec 19, 2017

bedevere-bot removed the awaiting merge label Dec 19, 2017

1st1 deleted the fix_sock_type branch December 19, 2017 01:02

snemes mentioned this pull request Aug 5, 2019

Workaround for a bug in Python 3.5 and 3.6 affecting UDP proxying Anorov/PySocks#128

Open

Uh oh!

bpo-32331: Fix socket.type on Linux; expose SOCK_TYPE_MASK. #4877

bpo-32331: Fix socket.type on Linux; expose SOCK_TYPE_MASK. #4877

Uh oh!

Conversation

1st1 commented Dec 15, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

methane commented Dec 15, 2017

Uh oh!

asvetlov commented Dec 15, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 15, 2017

Uh oh!

pitrou commented Dec 15, 2017

Uh oh!

vstinner commented Dec 15, 2017

Uh oh!

1st1 commented Dec 15, 2017

Uh oh!

1st1 commented Dec 16, 2017

Uh oh!

1st1 commented Dec 16, 2017

Uh oh!

1st1 commented Dec 16, 2017

Uh oh!

njsmith commented Dec 16, 2017

Uh oh!

njsmith commented Dec 16, 2017

Uh oh!

1st1 commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Dec 18, 2017

Uh oh!

1st1 commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

1st1 commented Dec 15, 2017 •

edited by bedevere-bot

Loading

asvetlov Dec 15, 2017 •

edited

Loading

1st1 commented Dec 19, 2017 •

edited

Loading