Many functions in socket module are not thread safe

[sobomax]

BPO	1467080
Nosy	@loewis
Files	socketmodule.diff: Proposed fix. socketmodule.diff.v2: Version of the patch that don't use heap.

^{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 = <Date 2006-08-21.23:43:20.000>
created_at = <Date 2006-04-09.01:09:11.000>
labels = []
title = 'Many functions in socket module are not thread safe'
updated_at = <Date 2006-08-21.23:43:20.000>
user = 'https://bugs.python.org/sobomax'

bugs.python.org fields:

activity = <Date 2006-08-21.23:43:20.000>
actor = 'loewis'
assignee = 'none'
closed = True
closed_date = None
closer = None
components = ['None']
creation = <Date 2006-04-09.01:09:11.000>
creator = 'sobomax'
dependencies = []
files = ['1959', '1960']
hgrepos = []
issue_num = 1467080
keywords = []
message_count = 11.0
messages = ['28153', '28154', '28155', '28156', '28157', '28158', '28159', '28160', '28161', '28162', '28163']
nosy_count = 2.0
nosy_names = ['loewis', 'sobomax']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1467080'
versions = ['Python 2.4']

[sobomax]

The socket module make a great effort to be thread-safe, but misses one
big point - it uses single per-instance buffer to hold resolved
sockaddr_xxx structures. Therefore, on SMP system it is possible that
several threads calling functions that perform address resolution in
parallel will stomp on each other resulting in incorrect or invalid address
to be used in each case.

For example, two threads calling sendto() in parallel can result in packets
to be sent to incorrect addresses - packets from thread one from time to
time will be sent to address requested by thread two and vice versa.

Another, smaller issue is that the call to getnameinfo() is not protected
with netdb mutex on systems that don't have thread-safe resolver.

[loewis]

Logged In: YES
user_id=21627

I wonder why the sock_addr member is there in the socket
object in the first place. AFAICT, there is no need for it;
it was introduced in r4509.

Would you like to work on a patch?

[sobomax]

Logged In: YES
user_id=24670

Sorry, for some reason the patch did not go through first time. See attached.

[loewis]

Logged In: YES
user_id=21627

Why is it necessary to allocate the memory dynamically?
Couldn't the caller provide the memory on the stack (i.e.
through a local variable)?

[sobomax]

Logged In: YES
user_id=24670

Why is it necessary to allocate the memory dynamically?
Couldn't the caller provide the memory on the stack (i.e.
through a local variable)?

Local variable is not very good IMHO since in threading
environment with hundreds of threads running at the same
time stack can be a scarce resource. Another issue is that
the caller doesn't know the actual size it has to allocate
until the resolution has been done, therefore it would need
to overallocate in most cases.

-Maxim

[loewis]

Logged In: YES
user_id=21627

The argument of "hundreds of threads" is a red herring. The
address space available to each thread typically doesn't
depend on the number of threads. Instead, the stack size is
pre-determined, so it's vice versa: the number of threads
supported depends on that stack-size, which (currently)
isn't tunable. Also, stack space is typically a scarce
resource only for recursive functions. For leave functions,
it doesn't matter, unless a single function consumes the
majority of the stack (which certainly wouldn't be the case
here).

Allocation on the stack is cheap, and over-allocation
doesn't hurt. Please change the patch to use automatic
variables. It will become simpler and more maintainable that
way (in addition, it should also become minimally faster).

[sobomax]

Logged In: YES
user_id=24670

The address space available to each thread typically
doesn't
depend on the number of threads. Instead, the stack size
is
pre-determined, so it's vice versa: the number of threads
supported depends on that stack-size, which (currently)
isn't tunable.

Yes, but my point is that on low-end and/or embedded system
the system can be configured with as low stack per thread
as possible (since with for example 100 threads, every
extra 10K of stack per thread means 1M of extra memory,
which in the absence of paging needs to be wired down) and
if only one thread needs this stack 990K of it will be
effectively wasted. And since getaddrinfo()-family already
uses heap for its results I don't see any big win in
avoiding extra malloc()/free() call. Expecially considering
that we are dealing with i/o here, so that system call
overhead will be much more than that anyway, even if the
program calls those functions heavily.

-Maxim

[loewis]

Logged In: YES
user_id=21627

The big win is in simplification of the code. Also, we are
not talking about 10k here. On Linux, sock_addr_t is 128
bytes. Take a look at set_error: it allocates 100 bytes for
an error buffer. Or sock_repr: 512 bytes for a buffer. Or
socket_gethostname: 1024 bytes. Or socket_gethostbyname_ex:
16384 bytes. socket_getaddrinfo: 30 bytes. os_init: 100 bytes

Or, looking at other modules: symtable.c:check_unoptimized:
300 bytes. Py_GetVersion: 250 bytes. PySys_SetArgv:
2*MAXPATHLEN+1 (on Linux, this is 8193 bytes). I could go on.

Don't worry about stack consumption. Or, if you do, analyse
the Python source code, and fix the big offenders first.

Premature optimization is the root of all evil.

[sobomax]

Logged In: YES
user_id=24670

The big win is in simplification of the code.

What do you call "big simplification"? Several malloc/free
calls and appropriate NULL checks?

Aside from stack usage issues the big loss here is
portability - we either need to use some fixed length
buffer with the size sufficient to hold any type of address
(ugly!) or use sockaddr_storage, which may not exist on all
platforms supported by the python.

-Maxim

[sobomax]

Logged In: YES
user_id=24670

OK, it looks like the only way to get this bug fixed is to reimplement patch to
use stack instead of heap, so that here we go. Attached please find new version
of the patch which allocates address buffer on stack.

-Maxim

[loewis]

Logged In: YES
user_id=21627

The latest patch got resubmitted as bpo-1544279.