GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — #425 Remove UNIX socket from FS before binding. by socketpair · Pull Request #441 · python/asyncio

This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Conversation

socketpair commented Oct 6, 2016

Copy link

Copy Markdown

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZhukovAlexander reviewed

Oct 6, 2016

View reviewed changes

Comment thread

asyncio/unix_events.py

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

ZhukovAlexander Oct 6, 2016

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the file exists and is not a socket?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 6, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 6, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still have a questions ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 7, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no further questions about this line :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 commented Oct 6, 2016

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 reviewed

Oct 6, 2016

View reviewed changes

Comment thread

asyncio/unix_events.py

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

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to log this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 6, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at how base_events.py logs stuff.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 reviewed

Oct 6, 2016

View reviewed changes

Comment thread

tests/test_unix_events.py Outdated

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

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 6, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 6, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 6, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 Oct 7, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 8, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well. I will remove that test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 8, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

1st1 commented Oct 7, 2016

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 reviewed

Oct 7, 2016

View reviewed changes

Comment thread

asyncio/unix_events.py Outdated

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

1st1 Oct 7, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair Oct 7, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

socketpair commented Oct 7, 2016

Copy link

Copy Markdown

Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 commented Oct 7, 2016

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

socketpair commented Oct 7, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Author

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 commented Oct 7, 2016

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fixes #425 Remove UNIX socket from FS before binding.

806cc68

1st1 merged commit 806cc68 into python:master

Oct 9, 2016

1st1 commented Oct 9, 2016

Copy link

Copy Markdown

Member

Thank you, Mark!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

2 more reviewers

1st1

1st1 left review comments

ZhukovAlexander

ZhukovAlexander left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

socketpair Oct 6, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

1st1 Oct 7, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

socketpair Oct 8, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

socketpair commented Oct 7, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.