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

bpo-28134: Auto-detect socket values from file descriptor #1349

Merged
merged 1 commit into from
Jan 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions Doc/library/socket.rst
Original file line number Diff line number Diff line change
Expand Up @@ -461,10 +461,15 @@ The following functions all create :ref:`socket objects <socket-objects>`.
:const:`SOCK_DGRAM`, :const:`SOCK_RAW` or perhaps one of the other ``SOCK_``
constants. The protocol number is usually zero and may be omitted or in the
case where the address family is :const:`AF_CAN` the protocol should be one
of :const:`CAN_RAW`, :const:`CAN_BCM` or :const:`CAN_ISOTP`. If *fileno* is specified, the other
arguments are ignored, causing the socket with the specified file descriptor
to return. Unlike :func:`socket.fromfd`, *fileno* will return the same
socket and not a duplicate. This may help close a detached socket using
of :const:`CAN_RAW`, :const:`CAN_BCM` or :const:`CAN_ISOTP`

If *fileno* is specified, the values for *family*, *type*, and *proto* are
auto-detected from the specified file descriptor. Auto-detection can be
overruled by calling the function with explicit *family*, *type*, or *proto*
arguments. This only affects how Python represents e.g. the return value
of :meth:`socket.getpeername` but not the actual OS resource. Unlike
:func:`socket.fromfd`, *fileno* will return the same socket and not a
duplicate. This may help close a detached socket using
:meth:`socket.close()`.

The newly created socket is :ref:`non-inheritable <fd_inheritance>`.
Expand Down
9 changes: 8 additions & 1 deletion Lib/socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,18 @@ class socket(_socket.socket):

__slots__ = ["__weakref__", "_io_refs", "_closed"]

def __init__(self, family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None):
def __init__(self, family=-1, type=-1, proto=-1, fileno=None):
# For user code address family and type values are IntEnum members, but
# for the underlying _socket.socket they're just integers. The
# constructor of _socket.socket converts the given argument to an
# integer automatically.
if fileno is None:
Copy link
Member Author

Choose a reason for hiding this comment

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

This block is not necessary but makes the code easier to understand for non-C coders

Copy link
Member

Choose a reason for hiding this comment

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

Do you propose to remove it? If not, I suggest None for default values rather than −1.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, while -1 falling through to socketmodule.c would trigger its default behavior, writing this here in Python would be nicer using the =None idiom.

if family == -1:
family = AF_INET
if type == -1:
type = SOCK_STREAM
if proto == -1:
proto = 0
_socket.socket.__init__(self, family, type, proto, fileno)
self._io_refs = 0
self._closed = False
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/_test_multiprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -4204,7 +4204,7 @@ def get_high_socket_fd(self):

def close(self, fd):
if WIN32:
socket.socket(fileno=fd).close()
socket.socket(socket.AF_INET, socket.SOCK_STREAM, fileno=fd).close()
Copy link
Member

Choose a reason for hiding this comment

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

Use keyword arguments for all of these.

else:
os.close(fd)

Expand Down
45 changes: 44 additions & 1 deletion Lib/test/test_socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import pickle
import struct
import random
import shutil
import string
import _thread as thread
import threading
Expand Down Expand Up @@ -1278,6 +1279,7 @@ def testSendAfterClose(self):

def testCloseException(self):
sock = socket.socket()
sock.bind((socket._LOCALHOST, 0))
socket.socket(fileno=sock.fileno()).close()
try:
sock.close()
Expand Down Expand Up @@ -1614,9 +1616,11 @@ def test_uknown_socket_family_repr(self):
) + 1

with socket.socket(
family=unknown_family, type=unknown_type, fileno=fd) as s:
family=unknown_family, type=unknown_type, proto=23,
fileno=fd) as s:
self.assertEqual(s.family, unknown_family)
self.assertEqual(s.type, unknown_type)
self.assertEqual(s.proto, 23)

@unittest.skipUnless(hasattr(os, 'sendfile'), 'test needs os.sendfile()')
def test__sendfile_use_sendfile(self):
Expand All @@ -1636,6 +1640,45 @@ def fileno(self):
with self.assertRaises(TypeError):
sock._sendfile_use_sendfile(File(None))

def _test_socket_fileno(self, s, family, stype):
self.assertEqual(s.family, family)
self.assertEqual(s.type, stype)

fd = s.fileno()
s2 = socket.socket(fileno=fd)
self.addCleanup(s2.close)
# detach old fd to avoid double close
s.detach()
self.assertEqual(s2.family, family)
self.assertEqual(s2.type, stype)
self.assertEqual(s2.fileno(), fd)

def test_socket_fileno(self):
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self.addCleanup(s.close)
s.bind((support.HOST, 0))
self._test_socket_fileno(s, socket.AF_INET, socket.SOCK_STREAM)

if hasattr(socket, "SOCK_DGRAM"):
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
self.addCleanup(s.close)
s.bind((support.HOST, 0))
self._test_socket_fileno(s, socket.AF_INET, socket.SOCK_DGRAM)

if support.IPV6_ENABLED:
s = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
self.addCleanup(s.close)
s.bind((support.HOSTv6, 0, 0, 0))
self._test_socket_fileno(s, socket.AF_INET6, socket.SOCK_STREAM)

if hasattr(socket, "AF_UNIX"):
tmpdir = tempfile.mkdtemp()
self.addCleanup(shutil.rmtree, tmpdir)
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
self.addCleanup(s.close)
s.bind(os.path.join(tmpdir, 'socket'))
self._test_socket_fileno(s, socket.AF_UNIX, socket.SOCK_STREAM)


@unittest.skipUnless(HAVE_SOCKET_CAN, 'SocketCan required for this test.')
class BasicCANTest(unittest.TestCase):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Sockets now auto-detect family, type and protocol from file descriptor by
default.
71 changes: 69 additions & 2 deletions Modules/socketmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ Local naming conventions:

/* Socket object documentation */
PyDoc_STRVAR(sock_doc,
"socket(family=AF_INET, type=SOCK_STREAM, proto=0, fileno=None) -> socket object\n\
"socket(family=AF_INET, type=SOCK_STREAM, proto=0) -> socket object\n\
socket(family=-1, type=-1, proto=-1, fileno=None) -> socket object\n\
\n\
Open a socket of the given type. The family argument specifies the\n\
address family; it defaults to AF_INET. The type argument specifies\n\
Expand All @@ -111,6 +112,9 @@ or datagram (SOCK_DGRAM) socket. The protocol argument defaults to 0,\n\
specifying the default protocol. Keyword arguments are accepted.\n\
The socket is created as non-inheritable.\n\
\n\
When a fileno is passed in, family, type and proto are auto-detected,\n\
unless they are explicitly set.\n\
\n\
A socket object represents one endpoint of a network connection.\n\
\n\
Methods of socket objects (keyword arguments not allowed):\n\
Expand Down Expand Up @@ -4745,7 +4749,7 @@ sock_initobj(PyObject *self, PyObject *args, PyObject *kwds)
PySocketSockObject *s = (PySocketSockObject *)self;
PyObject *fdobj = NULL;
SOCKET_T fd = INVALID_SOCKET;
int family = AF_INET, type = SOCK_STREAM, proto = 0;
int family = -1, type = -1, proto = -1;
static char *keywords[] = {"family", "type", "proto", "fileno", 0};
#ifndef MS_WINDOWS
#ifdef SOCK_CLOEXEC
Expand Down Expand Up @@ -4795,9 +4799,72 @@ sock_initobj(PyObject *self, PyObject *args, PyObject *kwds)
"can't use invalid socket value");
return -1;
}

if (family == -1) {
sock_addr_t addrbuf;
socklen_t addrlen = sizeof(sock_addr_t);

memset(&addrbuf, 0, addrlen);
if (getsockname(fd, SAS2SA(&addrbuf), &addrlen) == 0) {
family = SAS2SA(&addrbuf)->sa_family;
} else {
#ifdef MS_WINDOWS
PyErr_SetFromWindowsErrWithFilename(0, "family");
#else
PyErr_SetFromErrnoWithFilename(PyExc_OSError, "family");
#endif
return -1;
}
}
#ifdef SO_TYPE
if (type == -1) {
int tmp;
socklen_t slen = sizeof(tmp);
if (getsockopt(fd, SOL_SOCKET, SO_TYPE, &tmp, &slen) == 0) {
type = tmp;
} else {
#ifdef MS_WINDOWS
PyErr_SetFromWindowsErrWithFilename(0, "type");
#else
PyErr_SetFromErrnoWithFilename(PyExc_OSError, "type");
#endif
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

any idea if SO_TYPE or SO_PROTOCOL are ever actually undefined on any platforms? If so, it'd be nice if we could make the exception raised as a result of this contain error message text indicating that the type and protocol must be supplied on the given platform.

}
}
#else
type = SOCK_STREAM;
#endif
#ifdef SO_PROTOCOL
if (proto == -1) {
int tmp;
socklen_t slen = sizeof(tmp);
if (getsockopt(fd, SOL_SOCKET, SO_PROTOCOL, &tmp, &slen) == 0) {
proto = tmp;
} else {
#ifdef MS_WINDOWS
PyErr_SetFromWindowsErrWithFilename(0, "protocol");
#else
PyErr_SetFromErrnoWithFilename(PyExc_OSError, "protocol");
#endif
return -1;
}
}
#else
proto = 0;
#endif
}
}
else {
/* No fd, default to AF_INET and SOCK_STREAM */
if (family == -1) {
family = AF_INET;
}
if (type == -1) {
type = SOCK_STREAM;
}
if (proto == -1) {
proto = 0;
}
#ifdef MS_WINDOWS
/* Windows implementation */
#ifndef WSA_FLAG_NO_HANDLE_INHERIT
Expand Down