#425 Remove UNIX socket from FS before binding. by socketpair · Pull Request #441 · python/asyncio · GitHub

No description provided.

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 :)

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

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.

fixed

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 :).

@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?

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

done

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.

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.

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

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

@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.

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

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

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

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

                                   '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:

#425 Remove UNIX socket from FS before binding. by socketpair · Pull Request #441 · python/asyncio · GitHub

Repository navigation

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

#425 Remove UNIX socket from FS before binding.#4411st1 merged 1 commit intopython:masterpython/asyncio:masterfrom socketpair:unix_rmCopy head branch name to clipboard

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

#425 Remove UNIX socket from FS before binding.#441
1st1 merged 1 commit into
python:masterfrom
socketpair:unix_rm

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