Improve setting the domain socket path into the sockaddr_un struct in TSockets

[emmenlau]

The code that sets a domain socket path in TServerSocket.cpp and TSocket.cpp is currently slightly more complex than required (at least that is my understanding). Also it seems to contain a potential out-of-bounds-access.

I simplified the code in the following way:

• Ensure that the sockaddr_un struct is fully set to zero before using it

  • This removes the necessity to deal with zero-termination of the path. It also removes the need for the len variable

• Do not use path_.size() + 1 length when memcpy the path from path_ to the sockaddr_un struct

  • This may be an out-of-bounds-access in the current code, depending on whether path_ actually contains an additional zero-terminating character (which is unlikely)

• Did you create an Apache Jira ticket? (not required for trivial changes)

• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?

• Did you squash your changes to a single commit? (not required, but preferred)

• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?

• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[ulidtko]

I think I get it.

This code would "truncate" the unused bytes from address.sun_path (which'd include the null terminator BTW).

This may have worked — but unix(7) clearly says to pass sizeof(struct sockaddr_un), the full thing, without any truncation business.

[ulidtko]

This addresses the null-termination concern nicely (via memset), better conforms to unix(7) and also obviates funny manipulations with lengths.

LGTM.

[deiv]

I think, this will break the abstract unix socket type name. You need the length of the string without the null termination for abstract unix sockets. For pathname unix sockets, the null termination is needed.

[emmenlau]

Can you elaborate further if you observed problems with this? I had the same problems as https://stackoverflow.com/a/11640827/7200132 and problems went away when switching to above solution. This was tested on Ubuntu 18.04 and CentOS 7.

[deiv]

As the manual page says:

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 abstract socket you need to pass the length of the string (without the null termination), plus sockaddr_un.sun_family size.

static_cast<socklen_t>(sizeof(struct sockaddr_un))) I think that will give the entire struct size.

In my case, Debian testing, for the null termination I'm getting an @. So if I want to connect to the socket from Java, I need to put extras '\0' at the final of the asbtract socket id to make it work.

[ulidtko]

Null termination, whether needed or not, is covered by the memset. Zero out the whole structure before use — that's the best way to go. So, null termination is obviously correct here.

Now, I think @deiv has a point too. The addrlen argument to bind may be incorrect. @emmenlau can you check if with this patch socket names in /proc/net/unix contain unexpected zero-bytes? Tests would still work this way I'm sure, but it's better to double-check this.

[deiv]

Yes, as I say, the problem is the size not the null terminating character. But if you prefer to fill it with zeros, without needing it, go ahead !!

[ulidtko]

As #2235 shows, I've LGTM'd this too soon. Thanks, @deiv.

The 3rd argument to bind(), the addrlen, should be computed differently. Depending on the requested socket type, addrlen should have a + 1 and null terminator for "pathname" sockets, and no +1 neither terminator for "abstract" sockets.

Further, the sun_path space check also needs + 1 or +0, for pathname and abstract sockets respectively.

[emmenlau]

This commit is now superseded by #2235