Would you make a pull request against libzmq master? Thanks
On Fri, Mar 11, 2016 at 7:07 PM, Peter LaDow <[email protected]> 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 > [email protected] > http://lists.zeromq.org/mailman/listinfo/zeromq-dev _______________________________________________ zeromq-dev mailing list [email protected] http://lists.zeromq.org/mailman/listinfo/zeromq-dev
