[zeromq-dev] Fixing tempnam() usage

Pieter Hintjens ph at imatix.com
Sat Mar 12 15:11:56 CET 2016


Would you make a pull request against libzmq master? Thanks

On Fri, Mar 11, 2016 at 7:07 PM, Peter LaDow <pladow at gmail.com> wrote:
> 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&);
>      };
> _______________________________________________
> zeromq-dev mailing list
> zeromq-dev at lists.zeromq.org
> http://lists.zeromq.org/mailman/listinfo/zeromq-dev



More information about the zeromq-dev mailing list