Done. I created pull request #1852
On Sat, Mar 12, 2016 at 6:11 AM, Pieter Hintjens <[email protected]> wrote: > 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 _______________________________________________ zeromq-dev mailing list [email protected] http://lists.zeromq.org/mailman/listinfo/zeromq-dev
