[zeromq-dev] Fixing tempnam() usage

Peter LaDow pladow at gmail.com
Fri Mar 11 19:07:42 CET 2016


I have run into the warning regarding the usage of tempnam() in
ipc_listener_t::set_address (our code was using 4.0.4).  I see looking
through the archives there was some discussion on resolving the
"danger" of using this function.  But I think the proposed solution
(which I see have been integrated into the 4.1 and development
branches) could be improved.

First, the race condition that is associated with tempnam() (i.e.
creation of the pathname and the opening of the file) is not removed
by the current implementation.  From the development branch:

146:  // Allow wildcard file
147:  if (addr [0] == '*') {
148:    char buffer [12] = "2134XXXXXX";
149:    int fd = mkstemp (buffer);
150:    if (fd == -1)
151:      return -1;
152:    addr.assign (buffer);
153:    ::close (fd);
154:  }
155:
156:  // Get rid of the file associated with the UNIX domain socket that
157:  // may have been left behind by the previous run of the application.
158:  ::unlink (addr.c_str());
159:  filename.clear ();
160:
161:  // Initialise the address structure.
162:  ipc_address_t address;
163:  int rc = address.resolve (addr.c_str());
164:  if (rc != 0)
165:    return -1;
166:
167:  // Create a listening socket.
168:  s = open_socket (AF_UNIX, SOCK_STREAM, 0);
169:  if (s == -1)
170:    return -1;
171:
172:  address.to_string (endpoint);
173:
174:  //  Bind the socket to the file path.
175:  rc = bind (s, address.addr (), address.addrlen ());
176:  if (rc != 0)
177:    goto error;

>From the time the file is closed on line 153 to the time it is bound
on line 175, another process could have created/bound the same file.

The portable means to avoid this race condition is to create a
temporary directory with the appropriate permissions and create the
socket in that directory.  Under linux, one can alternatively use
abstract sockets.

Second, I note that the current usage is to create the socket in the
current directory.  Would it not perhaps be better to create it in
TMPDIR, TEMPDIR, or TMP (if any of the variables exist)?

To that end, I've created the following patch that:

  1) Eliminates the race condition between file creation and socket binding
  2) Makes use of TMPDIR, TEMPDIR, or TMP if available.  If not, the
CWD is used.

This creates a socket of the template: $TMPDIR/tmpXXXXXX/socket.  The
code also performs a cleanup of the directory in
ipc_listener_t::close() if the file is removed.

I used POSIX functions only (getenv(), stat(), mkdtemp()) and basic
C++ containers.  I tried to avoid C++11 constructs (though it appears
to work fine when building under Linux even with
std::string::crbegin).

I did some initial testing and it seems to work fine for me.  The
temporary directory is created, the socket is created inside it, and
both are cleaned up on destruction of the context associated with the
socket (via zmq_ctx_term).

Thanks,
Pete

diff --git a/src/ipc_listener.cpp b/src/ipc_listener.cpp
index 6dc915e..93fb8bd 100644
--- a/src/ipc_listener.cpp
+++ b/src/ipc_listener.cpp
@@ -49,6 +49,7 @@
 #include <sys/socket.h>
 #include <fcntl.h>
 #include <sys/un.h>
+#include <sys/stat.h>

 #ifdef ZMQ_HAVE_LOCAL_PEERCRED
 #   include <sys/types.h>
@@ -63,6 +64,54 @@
 #   endif
 #endif

+const char *zmq::ipc_listener_t::tmp_env_vars[] = {
+  "TMPDIR",
+  "TEMPDIR",
+  "TMP",
+  0  // Sentinel
+};
+
+int zmq::ipc_listener_t::create_wildcard_address(std::string& path_)
+{
+  std::string tmp_path;
+
+  // If TMPDIR, TEMPDIR, or TMP are available and are directories, create
+  // the socket there.
+  const char **tmp_env = tmp_env_vars;
+  while ( tmp_path.empty() && *tmp_env != 0 )
+  {
+    char *tmpdir = getenv(*tmp_env);
+    struct stat statbuf;
+    if ( tmpdir != 0 && ::stat(tmpdir, &statbuf) == 0 &&
S_ISDIR(statbuf.st_mode) )
+    {
+      tmp_path.assign(tmpdir);
+      if ( *(tmp_path.rbegin()) != '/' )
+      {
+        tmp_path.push_back('/');
+      }
+    }
+
+    ++tmp_env;
+  }
+
+  // Append a directory name
+  tmp_path.append("tmpXXXXXX");
+
+  // We need room for tmp_path + trailing NUL
+  std::vector<char> buffer(tmp_path.length()+1);
+  strcpy(buffer.data(), tmp_path.c_str());
+
+  // Create the directory
+  if ( mkdtemp(buffer.data()) == 0 )
+  {
+    return -1;
+  }
+
+  path_.assign(buffer.data());
+
+  return 0;
+}
+
 zmq::ipc_listener_t::ipc_listener_t (io_thread_t *io_thread_,
       socket_base_t *socket_, const options_t &options_) :
     own_t (io_thread_, options_),
@@ -148,12 +197,15 @@ int zmq::ipc_listener_t::set_address (const char *addr_)

     //  Allow wildcard file
     if (addr [0] == '*') {
-        char buffer [12] = "2134XXXXXX";
-        int fd = mkstemp (buffer);
-        if (fd == -1)
-            return -1;
-        addr.assign (buffer);
-        ::close (fd);
+      std::string tmp_path;
+
+      if ( create_wildcard_address(tmp_path) < 0 ) {
+        return -1;
+      }
+
+      tmp_socket_dirname.assign(tmp_path);
+
+      addr.assign (tmp_path + "/socket");
     }

     //  Get rid of the file associated with the UNIX domain socket that
@@ -170,7 +222,7 @@ int zmq::ipc_listener_t::set_address (const char *addr_)
     ipc_address_t address;
     int rc = address.resolve (addr.c_str());
     if (rc != 0)
-        return -1;
+        goto error;

     address.to_string (endpoint);

@@ -180,7 +232,7 @@ int zmq::ipc_listener_t::set_address (const char *addr_)
         //  Create a listening socket.
         s = open_socket (AF_UNIX, SOCK_STREAM, 0);
         if (s == -1)
-            return -1;
+            goto error;

         //  Bind the socket to the file path.
         rc = bind (s, address.addr (), address.addrlen ());
@@ -208,9 +260,13 @@ error:

 int zmq::ipc_listener_t::close ()
 {
-    zmq_assert (s != retired_fd);
-    int rc = ::close (s);
-    errno_assert (rc == 0);
+    int rc;
+
+    if ( s != -1 ) {
+      zmq_assert (s != retired_fd);
+      rc = ::close (s);
+      errno_assert (rc == 0);
+    }

     s = retired_fd;

@@ -219,8 +275,17 @@ int zmq::ipc_listener_t::close ()
     //  MUST NOT unlink if the FD is managed by the user, or it will stop
     //  working after the first client connects. The user will take care of
     //  cleaning up the file after the service is stopped.
-    if (has_file && !filename.empty () && options.use_fd == -1) {
-        rc = ::unlink(filename.c_str ());
+    if (has_file && options.use_fd == -1) {
+        rc = 0;
+
+        if ( !filename.empty () ) {
+          rc = ::unlink(filename.c_str ());
+        }
+
+        if ( rc == 0 && !tmp_socket_dirname.empty() ) {
+          rc = ::rmdir(tmp_socket_dirname.c_str ());
+        }
+
         if (rc != 0) {
             socket->event_close_failed (endpoint, zmq_errno());
             return -1;
diff --git a/src/ipc_listener.hpp b/src/ipc_listener.hpp
index 56f931c..82c1ba8 100644
--- a/src/ipc_listener.hpp
+++ b/src/ipc_listener.hpp
@@ -73,6 +73,9 @@ namespace zmq
         //  Close the listening socket.
         int close ();

+        // Create wildcard path address
+        static int create_wildcard_address(std::string& path_);
+
         //  Filter new connections if the OS provides a mechanism to get
         //  the credentials of the peer process.  Called from accept().
 #       if defined ZMQ_HAVE_SO_PEERCRED || defined ZMQ_HAVE_LOCAL_PEERCRED
@@ -87,6 +90,10 @@ namespace zmq
         //  True, if the underlying file for UNIX domain socket exists.
         bool has_file;

+        //  Name of the temporary directory (if any) that has the
+        //  the UNIX domain socket
+        std::string tmp_socket_dirname;
+
         //  Name of the file associated with the UNIX domain address.
         std::string filename;

@@ -99,9 +106,12 @@ namespace zmq
         //  Socket the listener belongs to.
         zmq::socket_base_t *socket;

-       // String representation of endpoint to bind to
+        // String representation of endpoint to bind to
         std::string endpoint;

+        // Acceptable temporary directory environment variables
+        static const char *tmp_env_vars[];
+
         ipc_listener_t (const ipc_listener_t&);
         const ipc_listener_t &operator = (const ipc_listener_t&);
     };



More information about the zeromq-dev mailing list