bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866

zonyitoo · 2021-06-23T03:09:21Z

Linux: https://man7.org/linux/man-pages/man7/unix.7.html
*BSD: SUN_LEN

https://bugs.python.org/issue44493

Automerge-Triggered-By: GH:gpshead

the-knights-who-say-ni · 2021-06-23T03:09:24Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@zonyitoo

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

github-actions · 2021-07-26T00:04:14Z

This PR is stale because it has been open for 30 days with no activity.

zonyitoo · 2021-07-26T03:41:22Z

Can anyone review this PR?

zonyitoo · 2021-10-16T06:24:50Z

Can anyone review this PR?

habnabit · 2022-03-23T19:21:52Z

I too am now facing this same issue. How can it be moved along?

habnabit · 2022-03-24T00:11:31Z

I can't believe this missed the 3.10 release. :(

zonyitoo · 2022-03-24T01:27:08Z

Where are the maintainers?

Hello @zooba , @asvetlov , @tiran

tiran · 2022-03-24T08:29:16Z

I'm temporarily locking this PR to prevent further abuse.

The issue on BPO hasn't been triaged because it lacks any sort of description of the issue. You should provide an explanation why you consider the code to be wrong. A reproducer is greatly appreciated. The change set also needs a test case.

PS: In case you wonder where the maintainers are, two are currently in the middle of a war zone and the others are worried for their lives.

serhiy-storchaka

LGTM.

I think it is difficult to write any tests for this, right?

Modules/socketmodule.c

gpshead

A regression test seems doable and potentially important if I understand this bug correctly:

It looks like the length would be short by one in Python before this change, meaning binding to a AF_UNIX socket potentially loses the last character of the path name intended to be bound?

That should be an observable behavior change.

It also suggests that fixing this will 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?

bedevere-bot · 2022-03-24T18:28:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

serhiy-storchaka · 2022-03-24T18:42:45Z

I do not sure that it can be easily testes. Otherwise the tests would fail long time ago.

zonyitoo · 2022-03-25T02:55:31Z

I do not sure that it can be easily testes. Otherwise the tests would fail long time ago.

Yes. That's why I didn't add an unit test for this change.

Because Python itself didn't care about the length of sockaddr_un and just use sun_path as a C-string (assume NUL terminated) when converting to PyObject.

cpython/Modules/socketmodule.c

Lines 1320 to 1336 in b68431f

    
           #if defined(AF_UNIX) 
        
               case AF_UNIX: 
        
               { 
        
                   struct sockaddr_un *a = (struct sockaddr_un *) addr; 
        
           #ifdef __linux__ 
        
                   size_t linuxaddrlen = addrlen - offsetof(struct sockaddr_un, sun_path); 
        
                   if (linuxaddrlen > 0 && a->sun_path[0] == 0) {  /* Linux abstract namespace */ 
        
                       return PyBytes_FromStringAndSize(a->sun_path, linuxaddrlen); 
        
                   } 
        
                   else 
        
           #endif /* linux */ 
        
                   { 
        
                       /* regular NULL-terminated string */ 
        
                       return PyUnicode_DecodeFSDefault(a->sun_path); 
        
                   } 
        
               } 
        
           #endif /* AF_UNIX */

But if one of the server or client was written in other language, for example in this case, I started a server in Rust and connected it with a Python client binding to /tmp/shadowsocks-manager-c.sock. The server shows that the client's peer address is /tmp/shadowsocks-manager-c.soc (missing the last character) when accepted.

It can also easily reproduce with C clients or servers.

I think the key of making tests is to verify the socklen returned from APIs are correctly set, which have to be done in C. Where can I put these tests? Expecting for suggestions.

zonyitoo · 2022-03-25T04:36:13Z

One more thing that I have already mentioned in https://bugs.python.org/issue44493 :

The OS doesn't care the correctness of sun_len (in *BSD) and the socklen of struct sockaddr_un. It just copies them directly to the APIs' output. So even Python have set the wrong length, it did copy the path correctly to sun_path. If the other end ignored the length or sun_len and just use sun_path as a C-string, then everything will work as expected.

And this bug will become very obvious when using SOCK_DGRAM with UDS. Because we have to provide receivers' address in sendto.

Modules/socketmodule.c

gpshead · 2022-03-25T04:52:18Z

pretty esoteric and hard to reproduce issue, at least from Python due to the mix and match of sometimes C string and sometimes strict length use around these old hand wavy APIs. agreed that constructing a test is hard. I suggested an assertion that would catch it. not what I'd call pretty though. So if you don't want that, I'm still good with this PR.

Misc/NEWS.d/next/Library/2021-07-26-10-46-49.bpo-44493.xp3CRH.rst

- Linux: https://man7.org/linux/man-pages/man7/unix.7.html - *BSD: SUN_LEN

zonyitoo · 2022-03-25T05:17:45Z

All done.

zonyitoo · 2022-03-25T05:23:02Z

BTW, the sun_len in *BSD's struct sockaddr_un is not set in the getsockaddrarg. Don't know if there are any program out there rely on this field on *BSD.

Should we set this field if it exists?

#ifdef SUN_LEN
	addr->sun_len = SUN_LEN(addr);
#endif

or

#ifdef SUN_LEN
	addr->sun_len = *len_ret;
#endif

zonyitoo · 2022-03-25T05:38:55Z

Oh, @gpshead . It seems that the force push I just made have removed a commit from you. What change did you made? Move a comment?

erlend-aasland · 2022-03-25T08:41:32Z

Oh, @gpshead . It seems that the force push I just made have removed a commit from you. What change did you made? Move a comment?

Please use git merge --no-ff main instead of force pushing; it makes the reviewers job a lot easier. Contrary to GitLab, GitHub does not play well with force push.

See also Eric's excellent #31616 (comment) about force-pushing.

zonyitoo · 2022-03-25T08:49:22Z

Yes, but I have already force pushed. I will use merge next time.

gpshead · 2022-03-26T00:05:50Z

yeah all i did was address the comment move request.

zonyitoo · 2022-03-26T14:52:37Z

yeah all i did was address the comment move request.

Done.

miss-islington · 2022-03-26T18:50:45Z

@zonyitoo: Status check is done, and it's a failure ❌ .

arhadthedev · 2022-03-26T19:34:17Z

The test fluked on make[1]: execvp: ./_bootstrap_python: Permission denied, I had the same problem today. Retriggering the build by closing and reopening a PR allows the check to pass.

the-knights-who-say-ni added the CLA not signed label Jun 23, 2021

bedevere-bot added the awaiting review label Jun 23, 2021

zonyitoo mentioned this pull request Jun 23, 2021

SocketAddr from UnixDatagram recv_from missing the last character (on OS X) rust-lang/rust#69061

Closed

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 25, 2021

github-actions bot added the stale label Jul 26, 2021

zonyitoo force-pushed the main branch 2 times, most recently from 9d52974 to f6c4adc Compare Jul 26, 2021

zonyitoo force-pushed the main branch 3 times, most recently from d96470f to 75a4ce9 Compare Jul 26, 2021

github-actions bot removed the stale label Jul 27, 2021

This comment has been minimized.

Sign in to view

python locked as too heated and limited conversation to collaborators Mar 24, 2022

serhiy-storchaka approved these changes Mar 24, 2022

View changes

bedevere-bot added awaiting merge and removed awaiting review labels Mar 24, 2022

serhiy-storchaka reviewed Mar 24, 2022

View changes

Modules/socketmodule.c Outdated Show resolved Hide resolved

python unlocked this conversation Mar 24, 2022

gpshead requested changes Mar 24, 2022

View changes

bedevere-bot removed the awaiting merge label Mar 24, 2022

bedevere-bot added the awaiting changes label Mar 24, 2022

gpshead reviewed Mar 25, 2022

View changes

Modules/socketmodule.c Show resolved Hide resolved

gpshead reviewed Mar 25, 2022

View changes

Misc/NEWS.d/next/Library/2021-07-26-10-46-49.bpo-44493.xp3CRH.rst Show resolved Hide resolved

bpo-44493: Add missing terminated NUL in sockaddr_un's length

0fac700

- Linux: https://man7.org/linux/man-pages/man7/unix.7.html - *BSD: SUN_LEN

zonyitoo force-pushed the main branch from 4479c14 to 0fac700 Compare Mar 25, 2022

Moved comment to the separated line

a20a659

gpshead approved these changes Mar 26, 2022

View changes

bedevere-bot added awaiting merge and removed awaiting changes labels Mar 26, 2022

gpshead added needs backport to 3.9 needs backport to 3.10 type-bug labels Mar 26, 2022

gpshead self-assigned this Mar 26, 2022

gpshead added the 🤖 automerge label Mar 26, 2022

python / cpython Public

bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866

bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866

zonyitoo commented Jun 23, 2021 •

edited by miss-islington

the-knights-who-say-ni commented Jun 23, 2021

github-actions bot commented Jul 26, 2021

zonyitoo commented Jul 26, 2021

zonyitoo commented Oct 16, 2021

habnabit commented Mar 23, 2022

habnabit commented Mar 24, 2022

zonyitoo commented Mar 24, 2022 •

edited

This comment has been minimized.

tiran commented Mar 24, 2022 •

edited

serhiy-storchaka left a comment

gpshead left a comment

bedevere-bot commented Mar 24, 2022

serhiy-storchaka commented Mar 24, 2022

zonyitoo commented Mar 25, 2022 •

edited

zonyitoo commented Mar 25, 2022 •

edited

gpshead commented Mar 25, 2022

zonyitoo commented Mar 25, 2022

zonyitoo commented Mar 25, 2022 •

edited

zonyitoo commented Mar 25, 2022

erlend-aasland commented Mar 25, 2022 •

edited

zonyitoo commented Mar 25, 2022

gpshead commented Mar 26, 2022

zonyitoo commented Mar 26, 2022

miss-islington commented Mar 26, 2022

arhadthedev commented Mar 26, 2022

python / cpython Public

bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866

Are you sure you want to change the base?

bpo-44493: Add missing terminated NUL in sockaddr_un's length #26866

Conversation

zonyitoo commented Jun 23, 2021 • edited by miss-islington

the-knights-who-say-ni commented Jun 23, 2021

CLA Missing

github-actions bot commented Jul 26, 2021

zonyitoo commented Jul 26, 2021

zonyitoo commented Oct 16, 2021

habnabit commented Mar 23, 2022

habnabit commented Mar 24, 2022

zonyitoo commented Mar 24, 2022 • edited

This comment has been minimized.

tiran commented Mar 24, 2022 • edited

serhiy-storchaka left a comment

gpshead left a comment

bedevere-bot commented Mar 24, 2022

serhiy-storchaka commented Mar 24, 2022

zonyitoo commented Mar 25, 2022 • edited

zonyitoo commented Mar 25, 2022 • edited

gpshead commented Mar 25, 2022

zonyitoo commented Mar 25, 2022

zonyitoo commented Mar 25, 2022 • edited

zonyitoo commented Mar 25, 2022

erlend-aasland commented Mar 25, 2022 • edited

zonyitoo commented Mar 25, 2022

gpshead commented Mar 26, 2022

zonyitoo commented Mar 26, 2022

miss-islington commented Mar 26, 2022

arhadthedev commented Mar 26, 2022

zonyitoo commented Jun 23, 2021 •

edited by miss-islington

zonyitoo commented Mar 24, 2022 •

edited

tiran commented Mar 24, 2022 •

edited

zonyitoo commented Mar 25, 2022 •

edited

zonyitoo commented Mar 25, 2022 •

edited

zonyitoo commented Mar 25, 2022 •

edited

erlend-aasland commented Mar 25, 2022 •

edited