[zeromq-dev] Fixing tempnam() usage

Peter LaDow pladow at gmail.com
Mon Mar 14 16:54:44 CET 2016


Done.  I created pull request #1852

On Sat, Mar 12, 2016 at 6:11 AM, Pieter Hintjens <ph at imatix.com> wrote:
> 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
> _______________________________________________
> 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