#425 Remove UNIX socket from FS before binding.#441

socketpair · 2016-10-06T09:31:29Z

No description provided.

ZhukovAlexander · 2016-10-06T09:54:21Z

asyncio/unix_events.py

+            if path[0] not in (0, '\x00'):
+                try:
+                    if stat.S_ISSOCK(os.stat(path).st_mode):
+                        os.remove(path)


What if the file exists and is not a socket?

if stat.S_ISSOCK(os.stat(path).st_mode)

As much as we can. Let's kernel to decide what to do during bind()

I don't understand: can stat.S_ISSOCK return True if it's not a socket?

No, it can not. If this path refers to plain file, bind syscall will fail. We must not erase non-sockets in any case.

Ah, OK, I now understand what was @ZhukovAlexander's question about.

Do you still have a questions ?

No, no further questions about this line :)

1st1 · 2016-10-06T16:26:43Z

I think we should also remove the socket after the server is closed.

1st1 · 2016-10-06T16:27:51Z

asyncio/unix_events.py

+                        os.remove(path)
+                except OSError:
+                    # Directory may have permissions only to create socket.
+                    pass


I think we need to log this.

Sure, but we should introduce something like log = logging.getLogger(__name__). Please tell where to do that.

Look at how base_events.py logs stuff.

1st1 · 2016-10-06T16:30:18Z

tests/test_unix_events.py

-                with self.assertRaisesRegex(OSError,
-                                            'Address.*is already in use'):
-                    self.loop.run_until_complete(coro)
+                self.loop.run_until_complete(coro)


How does this work? We have a live socket, bound to path. Then we create a server. Does this mean that the first socket is in an invalid state? Or they are both bound to path now?

Seems, potential client will connect to second socket. First one will be hidden.

I did not check. Maybe it is implementation specific - i.e. Different on bsd and linux.

Please try to figure out what exactly is going on in this unittest.

I have checked. If socket-inode on FS is deleted, no one can connect. If it overrided with another UNIX-socket, all clients will be connected to that new one, as I expect eariler.

import socket import os if os.path.exists('/tmp/qwe'): os.unlink('/tmp/qwe') sock1 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock1.bind('/tmp/qwe') sock1.listen() os.unlink('/tmp/qwe') sock2 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock2.bind('/tmp/qwe') sock2.listen() #os.unlink('/tmp/qwe') sock3 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock3.connect('/tmp/qwe') sock3.send(b'asd') (sk, addr) = sock2.accept() print(sk.recv(6))

I can not find POSIX standard, describinf this...reading man 7 unix say that many things are implementation specific.

I think, that unittests are fine. Unittests must check that all assertions are not failed on target OS.

What assertions? Clearly, there is some system-depending behaviour here: some systems allow Unix sockets to bind to existing in-use Unix sockets paths. Why should we have a test for that?

Well. I will remove that test.

I transform that test: It now checks that binding to same path as CLOSED socket works. This will improve code-coverage. And also I have found a bug with logging :).

1st1 · 2016-10-07T16:28:16Z

@socketpair FWIW we're just a couple days from a complete feature freeze of asyncio. Will you have time to finish this, or should I submit a new PR?

1st1 · 2016-10-07T16:42:50Z

asyncio/unix_events.py

                    'path was not specified, and no sock specified')

-            if sock.family != socket.AF_UNIX:
+            if sock.family != socket.AF_UNIX or sock.type != socket.SOCK_STREAM:


BTW, I've just committed this check to asyncio, please rebase your PR. Regardless of this PR, this is something that needed to be fixed (I wanted to fix this before 3.5 came out, but completely forgot about it). /cc @gvanrossum

socketpair · 2016-10-07T20:27:26Z

I think we should also remove the socket after the server is closed.
@1st1 It is not so easy, to do that, I have to intorduce something new in base_events.Server

Actually (as I think), removing before binding and removing after closing should be done at Python's socket level, not at asyncio level. But this is another sad story.

1st1 · 2016-10-07T20:29:18Z

I think we should also remove the socket after the server is closed.
@1st1 It is not so easy, to do that, I have to intorduce something new in base_events.Server

Actually (as I think), removing before binding and removing after closing should be done at Python's socket level, not at asyncio level. But this is another sad story.

Maybe. But let's fix this in asyncio first.

socketpair · 2016-10-07T20:39:16Z

Maybe. But let's fix this in asyncio first.

@1st1 Can you advice best way to add unlink() after closing ?

1st1 · 2016-10-07T20:50:26Z

@1st1 Can you advice best way to add removing after closing ?

After giving this some thought - let's not add this. I now remember an issue with uvloop: originally I was removing the path, but then someone's program broke because it was cleaning up the path manually and assumed it's always there.

Trying to remove the path before connecting to it makes total sense, removing it after is nice, but not strictly necessary.

1st1 · 2016-10-09T16:12:26Z

Thank you, Mark!

ZhukovAlexander reviewed Oct 6, 2016

View reviewed changes

1st1 reviewed Oct 6, 2016

View reviewed changes

1st1 reviewed Oct 7, 2016

View reviewed changes

Fixes #425 Remove UNIX socket from FS before binding.

806cc68

1st1 merged commit 806cc68 into python:master Oct 9, 2016

socketpair deleted the unix_rm branch October 9, 2016 18:06

tdihp mentioned this pull request Jul 2, 2024

create_unix_server() removes listening unix socket, potentially from other process python/cpython#121252

Open

Uh oh!

Conversation

socketpair commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socketpair Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socketpair Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Oct 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socketpair commented Oct 7, 2016

Uh oh!

1st1 commented Oct 7, 2016

Uh oh!

socketpair commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Oct 7, 2016

Uh oh!

1st1 commented Oct 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

socketpair Oct 6, 2016 •

edited

Loading

1st1 Oct 7, 2016 •

edited

Loading

socketpair Oct 8, 2016 •

edited

Loading

socketpair commented Oct 7, 2016 •

edited

Loading