Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set correct sockaddr length for abstract addresses #374

Closed
wants to merge 1 commit into from

Conversation

rblaze
Copy link
Contributor

@rblaze rblaze commented Jan 30, 2019

There are three types of unix sockets [in Linux]: pathname, unnamed and abstract, see unix(7) for details. It is okay to use long sockaddr len with the pathname, which is null-terminated, and unnamed, which has no name.

But sun_path in the abstract sockaddr is effectively a blob, and sockaddr length must be set correctly. Otherwise, the connection fails.

Good strace:

9963  connect(4, {sa_family=AF_UNIX, sun_path=@"/tmp/dbus-fGvREc7jlQ"}, 23 <unfinished ...>

Bad strace:

connect(4, {sa_family=AF_UNIX, sun_path=@"/tmp/dbus-fLBigq6VT1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"}, 110 <unfinished ...>

@kazu-yamamoto
Copy link
Collaborator

Relating to #306

@kazu-yamamoto
Copy link
Collaborator

@vdukhovni @JonCoens Could you review this PR?

sizeOfSockAddr SockAddrUnix{} = #const sizeof(struct sockaddr_un)
sizeOfSockAddr (SockAddrUnix path) =
case path of
'\0':_ -> (#const sizeof(sa_family_t)) + length path

Choose a reason for hiding this comment

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

The abstract sockets are a Linux-specific feature. Should support for this also be Linux-specific? Is there any evidence of support for such unix-domain sockets on any of the BSDs, or other unix-like system?

[ By the way, it is rather unfortunate that the path associated with a unix-domain socket SockAddrUnix is a String and not a ByteString. Calling length here is only correct, because when binding the socket we later truncate all the Chars to CChars. Using ByteString here would have been a better interface. ]

Choose a reason for hiding this comment

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

Also at around line 1019, I don't see any effort to decode abstract socket addresses correctly. Is there a portable way to distinguish empty paths from abstract socket addresses? Presently it looks like they'll return an empty path from accept() , getsockname() and getpeername(). If these are to be supported, surely not just connect(), sendto() and bind() should work, but also accept(), getsockname(), getpeername and recvfrom() for datagram sockets.

@rblaze
Copy link
Contributor Author

rblaze commented Feb 5, 2019

Gentle ping.
Abstract sockets are used, for instance, by dbus-daemon when configured so, and this issue breaks support for said configuration in https://github.com/rblaze/haskell-dbus.

@vdukhovni
Copy link

Will you be addressing the review comments?

@rblaze
Copy link
Contributor Author

rblaze commented Feb 5, 2019

I'm just reverting parts of the #306 which broke things. This change would unbreak abstract sockets support and enable dbus library builds with the latest network versions.

If there was no support for getting proper addresses (I checked), I feel adding new features is out of scope for this diff.

@vdukhovni
Copy link

If abstract sockets are supported, they need to work correctly. Making them work not only for the system calls that take addresses, but also for the system-calls that return addresses is not a new feature. Also if the abstract sockets are Linux-specific, then the logic to support them needs to be specific to Linux, lest it break other platforms.

Once you open the can of worms you should deal with all the issues that present themselves, leaving the code in a known correct state.

@vdukhovni
Copy link

Some tests of support for abstract sockets would also reduce opportunities for future regressions.

@eborden
Copy link
Collaborator

eborden commented Feb 5, 2019

I fully support adding tests. I'd love to land #375 before that, since it is causing a lot of churn in the test modules.

@kazu-yamamoto
Copy link
Collaborator

OK. @rblaze wants to fix this regression and other stuffs are not his responsibility.
I opened #377 to brush up the support of abstract addresses.

I will merge this PR with #ifdef.

@kazu-yamamoto kazu-yamamoto self-requested a review February 6, 2019 05:53
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

I will add #ifdef

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Feb 6, 2019
@kazu-yamamoto
Copy link
Collaborator

Merged.
@rblaze Would you check and close this if OK.

@kazu-yamamoto kazu-yamamoto mentioned this pull request Feb 6, 2019
2 tasks
@rblaze
Copy link
Contributor Author

rblaze commented Feb 6, 2019

@kazu-yamamoto Thank you! This works just fine.

@rblaze rblaze closed this Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants