[GitHub] [thrift] ulidtko commented on a change in pull request #2234: Improve setting the domain socket path into the sockaddr_un struct in TSockets

[GitBox <gi...@apache.org>]

ulidtko commented on a change in pull request #2234:
URL: https://github.com/apache/thrift/pull/2234#discussion_r485650407



##########
File path: lib/cpp/src/thrift/transport/TServerSocket.cpp
##########
@@ -408,24 +407,21 @@ void TServerSocket::listen() {
     }
 
     struct sockaddr_un address;
+    // Store the zero-terminated path in address.sun_path
+    memset(&address, '\0', sizeof(address));
     address.sun_family = AF_UNIX;
-    memcpy(address.sun_path, path_.c_str(), len);
-
-    auto structlen = static_cast<socklen_t>(sizeof(address));
+    memcpy(address.sun_path, path_.c_str(), path_.size());
 
     if (!address.sun_path[0]) { // abstract namespace socket
-#ifdef __linux__
-      // sun_path is not null-terminated in this case and structlen determines its length
-      structlen -= sizeof(address.sun_path) - len;
-#else
+#ifndef __linux__
       GlobalOutput.perror("TSocket::open() Abstract Namespace Domain sockets only supported on linux: ", -99);
       throw TTransportException(TTransportException::NOT_OPEN,
                                 " Abstract Namespace Domain socket path not supported");
 #endif
     }
 
     do {
-      if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, structlen)) {
+      if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(address)))) {

Review comment:
       I'm a bit wary about this `sizeof(address)` — because of how `struct sockaddr` can have more data following it. What type is `address` again?..
   
   `man 7 unix` says this:
   >    **Pathname sockets**
   >       When binding a socket to a pathname, a few rules should be observed for maximum portability and ease of coding:
   >
   >   *  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)`.
   
   Ah, so I see, line 409, it's exactly `sockaddr_un`. So all's good.
   
   How about maybe:
   ```suggestion
         if (0 == ::bind(serverSocket_, (struct sockaddr*)&address, static_cast<socklen_t>(sizeof(struct sockaddr_un)))) {
   ```
   
   ? And similarly below




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org