Missing terminated NUL in the length of sockaddr_un

[zonyitoo]

BPO	44493
Nosy	@gpshead, @pitrou, @tiran, @serhiy-storchaka, @miss-islington, @zonyitoo
PRs	bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866 [3.10] bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866) #32140 [3.9] bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866) #32141 [3.9] [3.10] bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866) (GH-32140) #32156

^{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 = 'https://github.com/gpshead'
closed_at = <Date 2022-03-28.01:58:49.361>
created_at = <Date 2021-06-23.03:08:23.197>
labels = ['extension-modules', '3.11', '3.9', '3.10', 'performance']
title = 'Missing terminated NUL in the length of sockaddr_un'
updated_at = <Date 2022-03-28.23:52:51.525>
user = 'https://github.com/zonyitoo'

bugs.python.org fields:

activity = <Date 2022-03-28.23:52:51.525>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2022-03-28.01:58:49.361>
closer = 'zonyitoo'
components = ['Extension Modules']
creation = <Date 2021-06-23.03:08:23.197>
creator = 'zonyitoo'
dependencies = []
files = []
hgrepos = []
issue_num = 44493
keywords = ['patch']
message_count = 18.0
messages = ['396379', '398213', '400058', '415916', '415943', '415944', '415945', '415947', '415949', '415950', '415953', '415956', '415966', '415967', '415988', '416135', '416200', '416207']
nosy_count = 7.0
nosy_names = ['gregory.p.smith', 'pitrou', 'christian.heimes', 'habnabit', 'serhiy.storchaka', 'miss-islington', 'zonyitoo']
pr_nums = ['26866', '32140', '32141', '32156']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue44493'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[zonyitoo]

https://github.com/python/cpython/blob/main/Modules/socketmodule.c#L1700

[zonyitoo]

Changes have been made in this PR, waiting for reviewing.

[zonyitoo]

Hello and PING.

[habnabit]

sigh.. adding myself to nosy here too in the hope that this gets any traction

[zonyitoo]

I think I have already provided enough information about this bug.

The len_ret was the length of sockaddr_un, which should include the terminated NUL in the sun_path. But the original implementation only use path.len + offsetof(struct sockaddr_un, sun_path).

References:

1. https://man7.org/linux/man-pages/man7/unix.7.html The Address format section.

2. The SUN_LEN macro in *BSD systems, https://man.cx/unix(4)

[zonyitoo]

Ping Heimes.

This is a huge bug that exists for a very long time, please consider merge it and fix it in the next relesae.

[tiran]

Antoine, Serhiy, please take a look. You are the last developers that touched the AF_UNIX socket path code.

Ty, why are you pinging me on this issue or on the PR? I'm neither familiar with that code nor responsible for it.

[zonyitoo]

Sorry Heimes. I just don't know who is responsible for this code, which is very very old.

[zonyitoo]

More about the tests:

The error will only shows on BSD systems, for example macOS.

Here are some tests done with C and Rust:

rust-lang/rust#69061

If both the servers and clients are written in Python, you won't notice this bug because the original code ignored the length of sockaddr_un and use the sun_path as a C string (NUL terminated string).

[zonyitoo]

Oh, it should also occurs on Linux, I should correct that.

[tiran]

We have a dedicated team of volunteers that triage incoming tickets. They assign tickets and ping experts. It looks like nobody triaged your submissions because nobody understood it. Your ticket did not contain an explanation and the link now goes to an unrelated line in the code.

That is the reason I asked you to provide detailed information on the ticket. We have over 7,000 open tickets and over 1,600 open PRs. Your issue got lost in the noise.

[serhiy-storchaka]

I am not the socket programming expert. It just happened that I fixed some bugs here. But according to the manpage https://man7.org/linux/man-pages/man7/unix.7.html the address length should include the terminating NUL: offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) + 1. So the fix is correct.

[gpshead]

Bug filing tip for ty/zonyitoo: Describe the problem in the text when filing the bug. Include its consequences and how it is observed in practice.

A bug filed just saying "look at this code, this is wrong" does not communicate a need for anyone to spend time on an issue or help others find an existing issue describing misbehavior they may be seeing.

[gpshead]

It looks like the length would be short by one in Python before this change, meaning binding or connecting to a non-anonymous named AF_UNIX socket potentially loses the last character of the path name intended to be bound? Depending on if the OS uses the length or trusts a null termination?

That should be an observable behavior change.

It also suggests that fixing this could break code that has been working around this bug forever by adding an extra character when binding or connecting to a non-anonymous AF_UNIX socket? Is that true? In what circumstances is this practically observed?

[zonyitoo]

Actually the OS will always trust the length has already contained the last NUL.

The test I did in this issue: rust-lang/rust#69061 showed that:

1. OS trusted the input length was set correctly, both sun_len and the length of struct sockaddr_un.
2. The struct sockaddr_un and length will be copied without changed to the other programs.
3. It could be reproduced by the Rust dev team. So it shouldn't be a problem in the testcase itself.

Maybe we could discuss this on the Github's PR page? I am very sure that it could be reproduced.

[gpshead]

New changeset f6b3a07 by ty in branch 'main':
bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866)
f6b3a07

[miss-islington]

New changeset 5944807 by Miss Islington (bot) in branch '3.10':
[3.10] bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866) (GH-32140)
5944807

[gpshead]

New changeset dae09c2 by Miss Islington (bot) in branch '3.9':
[3.9] bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866) (GH-32140) (GH-32156)
dae09c2

[nirs]

Hey, this change broke autobind of abstract unix socket on Linux:

When you specify empty string as socket address, the socket is bound to a random
address in the abstract namespace.

from unix(7):

   Autobind feature
       If  a  bind(2)  call  specifies  addrlen as sizeof(sa_family_t), or the
       SO_PASSCRED socket option was specified for a socket that was  not  ex‐
       plicitly  bound  to  an address, then the socket is autobound to an ab‐
       stract address.  The address consists of a  null  byte  followed  by  5
       bytes  in  the  character set [0-9a-f].  Thus, there is a limit of 2^20
       autobind addresses.  (From Linux 2.1.15, when the autobind feature  was
       added,  8  bytes  were  used,  and the limit was thus 2^32 autobind ad‐
       dresses.  The change to 5 bytes came in Linux 2.3.15.)

When you specify empty socket address (""), bind() is called with
sizeof(sa_familty_t) since path.len is 0. After this change, bind is called
with sizeof(sa_familty_t) + 1, so the socket is bound to the "\0"
instead of a random address.

In python 3.8 (not including this change):

$ python3.8
Python 3.8.13 (default, Jun 10 2022, 00:00:00) 
[GCC 12.1.1 20220507 (Red Hat 12.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>>> s.bind("")
>>> s.getsockname()
b'\x0075499'

With python 3.9 (including this change):

$ python3.9
Python 3.9.13 (main, Jun  9 2022, 00:00:00) 
[GCC 12.1.1 20220507 (Red Hat 12.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>>> s.bind("")
>>> s.getsockname()
b'\x00'

Since this bug did not show any evidence of a bug, and did not include
a failing test case, I think it is invalid, and the change should be
reverted.

In general socket address is not null terminated, so we don't need to
include the non existing terminating null in the address.

[tiran]

@nirs could you please open a new bug for the regression? Thanks!

[nirs]

Sure, I also have a trivial fix.

[nirs]

Opened #94821 to track this issue.