[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