This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Missing terminated NUL in the length of sockaddr_un
Type: resource usage Stage: resolved
Components: Extension Modules Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: christian.heimes, gregory.p.smith, habnabit, miss-islington, pitrou, serhiy.storchaka, zonyitoo
Priority: normal Keywords: patch

Created on 2021-06-23 03:08 by zonyitoo, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 26866 merged zonyitoo, 2021-06-23 03:09
PR 32140 merged miss-islington, 2022-03-27 20:22
PR 32141 closed miss-islington, 2022-03-27 20:23
PR 32156 merged miss-islington, 2022-03-28 18:26
Messages (18)
msg396379 - (view) Author: ty (zonyitoo) * Date: 2021-06-23 03:08
https://github.com/python/cpython/blob/main/Modules/socketmodule.c#L1700
msg398213 - (view) Author: ty (zonyitoo) * Date: 2021-07-26 05:48
Changes have been made in this PR, waiting for reviewing.
msg400058 - (view) Author: ty (zonyitoo) * Date: 2021-08-22 08:48
Hello and PING.
msg415916 - (view) Author: Aaron Gallagher (habnabit) Date: 2022-03-24 00:14
sigh.. adding myself to nosy here too in the hope that this gets any traction
msg415943 - (view) Author: ty (zonyitoo) * Date: 2022-03-24 14:50
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)
msg415944 - (view) Author: ty (zonyitoo) * Date: 2022-03-24 14:55
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.
msg415945 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-03-24 15:15
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.
msg415947 - (view) Author: ty (zonyitoo) * Date: 2022-03-24 15:32
Sorry Heimes. I just don't know who is responsible for this code, which is very very old.
msg415949 - (view) Author: ty (zonyitoo) * Date: 2022-03-24 15:39
More about the tests:

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

Here are some tests done with C and Rust:

https://github.com/rust-lang/rust/issues/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).
msg415950 - (view) Author: ty (zonyitoo) * Date: 2022-03-24 15:40
Oh, it should also occurs on Linux, I should correct that.
msg415953 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2022-03-24 16:01
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.
msg415956 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2022-03-24 16:29
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.
msg415966 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2022-03-24 18:33
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.
msg415967 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2022-03-24 18:35
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?
msg415988 - (view) Author: ty (zonyitoo) * Date: 2022-03-25 04:27
Actually the OS will always trust the length has already contained the last NUL.

The test I did in this issue: https://github.com/rust-lang/rust/issues/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.
msg416135 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2022-03-27 20:22
New changeset f6b3a07b7df60dc04d0260169ffef6e9796a2124 by ty in branch 'main':
bpo-44493: Add missing terminated NUL in sockaddr_un's length (GH-26866)
https://github.com/python/cpython/commit/f6b3a07b7df60dc04d0260169ffef6e9796a2124
msg416200 - (view) Author: miss-islington (miss-islington) Date: 2022-03-28 18:26
New changeset 5944807b09717d43bb017f700e8c451dd07199ed 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)
https://github.com/python/cpython/commit/5944807b09717d43bb017f700e8c451dd07199ed
msg416207 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2022-03-28 20:03
New changeset dae09c2b819c2683ad870733451c050b59c3eb93 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)
https://github.com/python/cpython/commit/dae09c2b819c2683ad870733451c050b59c3eb93
History
Date User Action Args
2022-04-11 14:59:47adminsetgithub: 88659
2022-03-28 23:52:51gregory.p.smithsetresolution: fixed
2022-03-28 20:03:36gregory.p.smithsetmessages: + msg416207
2022-03-28 18:26:59miss-islingtonsetpull_requests: + pull_request30236
2022-03-28 18:26:58miss-islingtonsetmessages: + msg416200
2022-03-28 01:58:49zonyitoosetstatus: open -> closed
stage: patch review -> resolved
2022-03-27 20:23:04miss-islingtonsetpull_requests: + pull_request30223
2022-03-27 20:22:59miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request30222
2022-03-27 20:22:31gregory.p.smithsetmessages: + msg416135
2022-03-25 04:53:21gregory.p.smithsetassignee: gregory.p.smith
2022-03-25 04:27:18zonyitoosetmessages: + msg415988
2022-03-24 18:35:08gregory.p.smithsetmessages: + msg415967
2022-03-24 18:33:29gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg415966
2022-03-24 16:29:53serhiy.storchakasetmessages: + msg415956
2022-03-24 16:01:38christian.heimessetmessages: + msg415953
2022-03-24 15:56:59serhiy.storchakasetnosy: + christian.heimes
2022-03-24 15:40:47zonyitoosetnosy: - christian.heimes
messages: + msg415950
2022-03-24 15:39:02zonyitoosetmessages: + msg415949
2022-03-24 15:32:54zonyitoosetmessages: + msg415947
2022-03-24 15:15:52christian.heimessetversions: + Python 3.9, Python 3.10, Python 3.11
nosy: + pitrou, serhiy.storchaka

messages: + msg415945

components: + Extension Modules, - Library (Lib)
type: behavior -> resource usage
2022-03-24 14:55:59zonyitoosetnosy: + christian.heimes
messages: + msg415944
2022-03-24 14:50:33zonyitoosetmessages: + msg415943
versions: - Python 3.11
2022-03-24 00:14:06habnabitsetnosy: + habnabit
messages: + msg415916
2021-09-26 21:51:49ned.deilysetresolution: fixed -> (no value)
versions: + Python 3.11, - Python 3.6
2021-08-22 08:48:49zonyitoosetmessages: + msg400058
2021-07-26 05:48:28zonyitoosetresolution: fixed
messages: + msg398213
2021-06-23 03:09:22zonyitoosetkeywords: + patch
stage: patch review
pull_requests: + pull_request25441
2021-06-23 03:08:23zonyitoocreate