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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:47 | admin | set | github: 88659 |
2022-03-28 23:52:51 | gregory.p.smith | set | resolution: fixed |
2022-03-28 20:03:36 | gregory.p.smith | set | messages:
+ msg416207 |
2022-03-28 18:26:59 | miss-islington | set | pull_requests:
+ pull_request30236 |
2022-03-28 18:26:58 | miss-islington | set | messages:
+ msg416200 |
2022-03-28 01:58:49 | zonyitoo | set | status: open -> closed stage: patch review -> resolved |
2022-03-27 20:23:04 | miss-islington | set | pull_requests:
+ pull_request30223 |
2022-03-27 20:22:59 | miss-islington | set | nosy:
+ miss-islington pull_requests:
+ pull_request30222
|
2022-03-27 20:22:31 | gregory.p.smith | set | messages:
+ msg416135 |
2022-03-25 04:53:21 | gregory.p.smith | set | assignee: gregory.p.smith |
2022-03-25 04:27:18 | zonyitoo | set | messages:
+ msg415988 |
2022-03-24 18:35:08 | gregory.p.smith | set | messages:
+ msg415967 |
2022-03-24 18:33:29 | gregory.p.smith | set | nosy:
+ gregory.p.smith messages:
+ msg415966
|
2022-03-24 16:29:53 | serhiy.storchaka | set | messages:
+ msg415956 |
2022-03-24 16:01:38 | christian.heimes | set | messages:
+ msg415953 |
2022-03-24 15:56:59 | serhiy.storchaka | set | nosy:
+ christian.heimes
|
2022-03-24 15:40:47 | zonyitoo | set | nosy:
- christian.heimes messages:
+ msg415950
|
2022-03-24 15:39:02 | zonyitoo | set | messages:
+ msg415949 |
2022-03-24 15:32:54 | zonyitoo | set | messages:
+ msg415947 |
2022-03-24 15:15:52 | christian.heimes | set | versions:
+ 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:59 | zonyitoo | set | nosy:
+ christian.heimes messages:
+ msg415944
|
2022-03-24 14:50:33 | zonyitoo | set | messages:
+ msg415943 versions:
- Python 3.11 |
2022-03-24 00:14:06 | habnabit | set | nosy:
+ habnabit messages:
+ msg415916
|
2021-09-26 21:51:49 | ned.deily | set | resolution: fixed -> (no value) versions:
+ Python 3.11, - Python 3.6 |
2021-08-22 08:48:49 | zonyitoo | set | messages:
+ msg400058 |
2021-07-26 05:48:28 | zonyitoo | set | resolution: fixed messages:
+ msg398213 |
2021-06-23 03:09:22 | zonyitoo | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request25441 |
2021-06-23 03:08:23 | zonyitoo | create | |