Support abstract UnixAddress by epi · Pull Request #5617 · dlang/phobos

epi · 2017-07-16T19:35:09Z

Lack of support for abstract UNIX domain sockets was discussed in forums [1], but I couldn't find a related issue in bugzilla.

According to [2]: "abstract: an abstract socket address is distinguished (from a pathname socket) by the fact that sun_path[0] is a null byte ('\0'). The socket's address in this namespace is given by the additional bytes in sun_path that are covered by the specified length of the address structure. (Null bytes in the name have no special significance.)"

For this reason, the previous implementation using strlen() was not sufficient, and additional class field containing the length of the address was required.

[1] http://forum.dlang.org/post/mmk50s$1qm$1@digitalmars.com
[2] UNIX(7)

dlang-bot · 2017-07-16T19:35:10Z

Thanks for your pull request, @epi! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

CyberShadow

Good idea, however the implementation seems to be somewhat confused.

It seems like the change seems to go halfway to making UnixAddress just store the address as a char[] with a header, which also seems to align with the actual representation and usage of UnixAddress. This is not how typical C programs deal with UNIX addresses (and other types of addresses), as there the address types are forced to be self-contained value types, which is not the case for std.socket.

So, why not go all the way and make UnixAddress just manage a header of sockaddr_un.sun_path.offsetof bytes plus an arbitrarily long (and containing arbitrary bytes) path? Something very similar was already done for SocketSet, which is not limited to FD_SETSIZE like the C equivalent.

CyberShadow · 2017-07-17T04:42:12Z

std/socket.d

            sun.sun_path.ptr[0 .. path.length] = (cast(byte[]) path)[];
            sun.sun_path.ptr[path.length] = 0;
+            _nameLen = cast(socklen_t) (sockaddr_un.init.sun_path.offsetof +
+                path.length + (sun.sun_path.ptr[0] ? 1 : 0));


Wouldn't the null byte be already included in path, and thus, path.length?

For pathname addresses, the terminating null is of course not included in path, and the manual recommends following these rules:

The pathname in sun_path should be null-terminated.

The length of the pathname, including the terminating null byte, should not exceed the size of sun_path.

The addrlen argument that describes the enclosing sockaddr_un structure should have a value of at least:
offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
or, more simply, addrlen can be specified as sizeof(struct sockaddr_un).

For abstract addresses, the null byte is the initial one and it is included in the path. The length of the address in this case is sockaddr_un.init.sun_path.offsetof + path.length.

Alright, so I understand that unlike regular UNIX sockets, abstract sockets' paths do not need to be null-terminated (and in fact doing so would change the address).

Please add a short comment describing the length calculation done here, as in your message above. A DDoc comment on the constructor mentioning how to create an abstract UNIX socket address would also be nice.

CyberShadow · 2017-07-17T04:45:01Z

std/socket.d

+            return (cast(const(char)*) sun.sun_path.ptr)[0 ..
+                _nameLen - sockaddr_un.init.sun_path.offsetof -
+                (sun.sun_path.ptr[0] ? 1 : 0)].idup;
        }


Why exclude the null byte from the result? It was present in the constructor, so excluding it here would be inconsistent. Also, without it, it seems like it's no longer possible to know whether a particular UnixAddress represents an abstract path or not.

The excluded byte is the null byte terminating a pathname address (added in the constructor to make it a C string).
For abstract address (sun.sun_path.ptr[0] == 0), nothing is excluded.

I see, thanks for the explanation. This should be added as a comment. I think it would be better to split the length calculation in multiple statements, so it's possible to explain each step.

A DDoc comment mentioning that filesystem paths are returned without the implicit null terminator but Linux abstract socket paths are returned as-is would be nice.

CyberShadow · 2017-07-17T04:48:15Z

std/socket.d

+        if (len > this.nameLen)
+            throw new SocketParameterException("Not enough socket address storage");
+    }
+


I don't understand the point of this method. The base Address class does not own the storage for the address data, so where the address itself is stored or how it deals with its storage/length is up to the descendant classes.

Setting the length for an address of fixed length (e.g. IP addresses) is nonsensical, so I don't see why this should be part of the abstract Address interface.

Furthermore, it seems to break the expectation that invoking a property's setter with some value should either throw or cause invoking the getter right afterwards to return the same value.

The reasoning behind adding this setter was:
There are methods in Socket (remoteAddress, localAddress, receiveFrom), which do the following:

call Socket.createAddress, getting an instance of one of Address' descendants, created with argument-less constructor.

call Address.name and Address.nameLen getters on that instance to get the address and capacity of the buffer for the next call

call one of getsockname, getpeername, recvfrom, to set the name in the buffer just obtained.

read Address.nameLen again and compare with result of previous call to see if the buffer was large enough (receiveFrom doesn't do this step)

While the above procedure works well for fixed-length addresses, it doesn't provide a way to feed the actual length of the address back to the concrete Address. I had to add a way that doesn't change the public interface nor the behavior of the above methods for fixed-length addresses, and does not require adding a special case for UnixAddress in all of them.

The nameLen setter is protected and deliberately not documented with DDoc. The default implementation in base Address is intended to be sufficient for the typical case: fixed-length addresses.
Yes, there is one case where it breaks the expectation you mentioned: where the len argument passed is less than the fixed-length. I should add a check for this.

Another solution I had in mind was to remove the argument-less constructors, get the address from getsockname, getpeername and receiveFrom to a temporary buffer on the stack, and refactor createAddress to create concrete Addresses by passing that temporary buffer to the appropriate constructor.

Alright. So, abstract UNIX socket addresses require that the address length is not greater than the length of the actual abstract "path", because they allow embedded null bytes and there is no way to infer the address length like the other address types we support so far. For this, I think a regular method would be more appropriate (for the reasons I mentioned above).

Also, it would be great if you could summarize the above into comments, as all this may not be obvious to the next person looking at this code.

CyberShadow · 2017-07-17T05:04:10Z

CyberShadow/DAutoTest — Pull request SHA mismatch

Gotcha.

If you look at the commit list on this PR, it says that the latest commit is 6f247e6.

However, if you try to fetch the PR branch (refs/pull/5617/head), the resulting commit is 84b727a.

It looks like 6f247e6 has the changelog entry, so it seems GitHub's caching is broken and is serving the wrong commit on the pull/ refs.

CyberShadow · 2017-07-17T05:10:36Z

Sent a message to support. CC @wilzbach :)

epi · 2017-07-17T13:35:58Z

As for the fail in DAutoTest, the reason could be that I added the changelog entry, committed with --amend, and then pushed with --force.

As for storing arbitrarily long arrays of bytes in UnixAddress, I don't see the benefit of that that could justify the added complexity. Implementations are limited to the static size of sockaddr_un anyway, e.g. Linux copies the address to a fixed-length buffer in kernel space, and rejects addresses longer than sizeof(sockaddr_un) [1][2]

[1] http://lxr.linux.no/linux+v4.10.1/include/uapi/linux/socket.h#L7
[2] http://lxr.linux.no/linux+v4.10.1/net/unix/af_unix.c#L218

CyberShadow · 2017-07-17T14:44:34Z

Implementations are limited to the static size of sockaddr_un anyway, e.g. Linux copies the address to a fixed-length buffer in kernel space, and rejects addresses longer than sizeof(sockaddr_un) [1][2]

That is a great argument against that idea.

I'm still somewhat unsure about the inconsistency in behaviour, such as treatment of the terminating null byte, for abstract sockets. Perhaps introducing a new type would be more appropriate, what do you think? Sorry, retracting this for now, I need to have a closer look at this tomorrow.

wilzbach · 2017-07-17T19:53:12Z

As for the fail in DAutoTest, the reason could be that I added the changelog entry, committed with --amend, and then pushed with --force.

That's quite a typical action and we are seeing this error message on other PRs , so I don't think this is related to anything you did (or did not) ;-)

epi · 2017-07-17T19:54:13Z

I'll just note that my first try was with class AbstractUnixAddress : UnixAddress, but it ended up being even more confusing than the current one. I've seen various conventions elsewhere: netstat and Go use '@' prefix (instead of initial '\0') to distinguish an abstract address [1]. DBus expects the key "abstract", e.g. "unix:abstract=/tmp/dbus-OtHLWmCLPR" [2].
My feeling is that since '\0' is explicitly allowed in D strings, the current behavior follows the POLA for someone who has read UNIX(7). I agree that adding an example in DDoc would make this clear.

[1] http://golang-examples.tumblr.com/post/92025745979/pitfall-of-abstract-unix-domain-socket-address-in
[2] https://dbus.freedesktop.org/doc/dbus-specification.html#transports-unix-domain-sockets

CyberShadow

Thanks for the explanations; this looks pretty great, just needs some clarifications in the code itself. An example for creating abstract UNIX socket addresses in the documentation would also be nice.

CyberShadow · 2017-07-18T01:43:36Z

std/socket.d

+        if (len > this.nameLen)
+            throw new SocketParameterException("Not enough socket address storage");
+    }
+


Alright. So, abstract UNIX socket addresses require that the address length is not greater than the length of the actual abstract "path", because they allow embedded null bytes and there is no way to infer the address length like the other address types we support so far. For this, I think a regular method would be more appropriate (for the reasons I mentioned above).

Also, it would be great if you could summarize the above into comments, as all this may not be obvious to the next person looking at this code.

CyberShadow · 2017-07-18T01:48:57Z

std/socket.d

            sun.sun_path.ptr[0 .. path.length] = (cast(byte[]) path)[];
            sun.sun_path.ptr[path.length] = 0;
+            _nameLen = cast(socklen_t) (sockaddr_un.init.sun_path.offsetof +
+                path.length + (sun.sun_path.ptr[0] ? 1 : 0));


Alright, so I understand that unlike regular UNIX sockets, abstract sockets' paths do not need to be null-terminated (and in fact doing so would change the address).

Please add a short comment describing the length calculation done here, as in your message above. A DDoc comment on the constructor mentioning how to create an abstract UNIX socket address would also be nice.

CyberShadow · 2017-07-18T01:55:00Z

std/socket.d

+            return (cast(const(char)*) sun.sun_path.ptr)[0 ..
+                _nameLen - sockaddr_un.init.sun_path.offsetof -
+                (sun.sun_path.ptr[0] ? 1 : 0)].idup;
        }


I see, thanks for the explanation. This should be added as a comment. I think it would be better to split the length calculation in multiple statements, so it's possible to explain each step.

A DDoc comment mentioning that filesystem paths are returned without the implicit null terminator but Linux abstract socket paths are returned as-is would be nice.

epi · 2017-07-18T08:51:15Z

Thanks for the detailed review, I'll follow up with the requested changes shortly.

epi · 2017-07-20T07:42:20Z

What's wrong with std.concurrency tests?
Also, I can't see my changelog entry when I build the preview according to these instructions: https://github.com/dlang/phobos/tree/master/changelog

CyberShadow · 2017-07-20T08:26:08Z

What's wrong with std.concurrency tests?

Looks like a random failure.

Also, I can't see my changelog entry when I build the preview according to these instructions: https://github.com/dlang/phobos/tree/master/changelog

Works for me. You may need to rm ../dlang.org/changelog/2.076.0_pre.dd if you've built dlang.org before adding your changelog entry.

wilzbach · 2017-07-20T11:18:35Z

You may need to rm ../dlang.org/changelog/2.076.0_pre.dd

That shouldn't be necessary - please let me know if the nuke helped.
Btw normally you can also preview the changelog at DAutoTest, but we are currently experiencing some troubles with GitHub (they have been notified and are working on their bugs).

wilzbach · 2017-07-21T02:42:03Z

That shouldn't be necessary - please let me know if the nuke helped.

Okay it was necessary. Sorry. Fix -> dlang/dlang.org#1837

epi · 2017-07-21T20:43:40Z

Thanks, I can now see my changelog entry.

epi · 2017-07-21T20:44:43Z

Is there anything else I should improve in this PR?

CyberShadow · 2017-07-22T15:11:21Z

I don't think so.

Thanks for your great work on this PR. Please send more! :)

epi requested review from CyberShadow and dnadlinger as code owners July 16, 2017 19:35

epi force-pushed the abstract-uds branch from 84b727a to 6f247e6 Compare July 17, 2017 03:57

CyberShadow requested changes Jul 17, 2017

View reviewed changes

CyberShadow mentioned this pull request Jul 17, 2017

Rename nWayUnion => multiwayMerge and NWayUnion => MultiwayMerge #5619

Merged

CyberShadow reviewed Jul 18, 2017

View reviewed changes

CyberShadow mentioned this pull request Jul 19, 2017

reimplement std.traits.ParameterStorageClassTuple() #5626

Merged

epi force-pushed the abstract-uds branch from 6f247e6 to 74c3dc3 Compare July 20, 2017 05:54

wilzbach mentioned this pull request Jul 21, 2017

Regenerate changelog on new changes dlang/dlang.org#1837

Merged

Support abstract UnixAddress

e3a650c

epi force-pushed the abstract-uds branch from 74c3dc3 to e3a650c Compare July 21, 2017 20:12

CyberShadow approved these changes Jul 22, 2017

View reviewed changes

CyberShadow added the Merge:auto-merge label Jul 22, 2017

dlang-bot merged commit c29516a into dlang:master Jul 22, 2017

epi deleted the abstract-uds branch July 23, 2017 18:41

CyberShadow mentioned this pull request Jul 25, 2017

Add spawnProcessDetached #5483

Merged

Uh oh!

Conversation

epi commented Jul 16, 2017

Uh oh!

dlang-bot commented Jul 16, 2017

Bugzilla references

Uh oh!

CyberShadow left a comment

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!

CyberShadow commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

epi commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

epi commented Jul 17, 2017

Uh oh!

CyberShadow left a comment

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!

epi commented Jul 18, 2017

Uh oh!

epi commented Jul 20, 2017

Uh oh!

CyberShadow commented Jul 20, 2017

Uh oh!

wilzbach commented Jul 20, 2017

Uh oh!

wilzbach commented Jul 21, 2017

Uh oh!

epi commented Jul 21, 2017

Uh oh!

epi commented Jul 21, 2017

Uh oh!

CyberShadow commented Jul 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyberShadow commented Jul 17, 2017 •

edited

Loading

CyberShadow commented Jul 17, 2017 •

edited

Loading