Hi Ian! Thanks for the patch!
You asked me to review it, so here goes... This is based on https://github.com/zeromq/libzmq/pull/238.diff as of today; there doesn't seem to be a better way to uniquely identify a Github pull request's *content* since it can change all the time :-( > diff --git a/src/ipc_listener.cpp b/src/ipc_listener.cpp > index 07a7dff..6797344 100644 > --- a/src/ipc_listener.cpp > +++ b/src/ipc_listener.cpp > @@ -95,8 +95,31 @@ void zmq::ipc_listener_t::in_event () > send_attach (session, engine, false); > } > > +int zmq::ipc_listener_t::get_address (unsigned char *addr, size_t *len) Coding style: Please use addr_ and len_ for function parameter names. > +{ > + struct sockaddr_un sun; > + int rc; > + > + // Get the details of the IPC socket > + socklen_t sl = sizeof(sockaddr_un); > + rc = getsockname (s, (sockaddr *)&sun, &sl); > + if (rc != 0) { > + return rc; > + } > + // Store the address for retrieval by users using wildcards > + *len = sprintf((char *)addr, "ipc://%s", sun.sun_path); > + > + return 0; > +} > + The use of sprintf() is a security hole if the user allocated not enough space at *addr. Please use snprintf() to ensure a maximum of len bytes (including the string terminator) are written to *addr. > int zmq::ipc_listener_t::set_address (const char *addr_) > { > + > + // Allow wildcard file > + if(*addr_ == '*') { > + addr_ = tempnam(NULL, NULL); > + } > + > // Get rid of the file associated with the UNIX domain socket that > // may have been left behind by the previous run of the application. > ::unlink (addr_); > @@ -124,7 +147,7 @@ int zmq::ipc_listener_t::set_address (const char *addr_) > rc = listen (s, options.backlog); > if (rc != 0) > return -1; > - > + > return 0; > } The use of tempnam() is a potential security hole, although we don't make any guarantees about untrusted local users. I'm not sure if there is an equivalent of mkstemp() for UNIX domain sockets, can't remember off the top of my head. > diff --git a/src/ipc_listener.hpp b/src/ipc_listener.hpp > index e1f4817..57e04ef 100644 > --- a/src/ipc_listener.hpp > +++ b/src/ipc_listener.hpp > @@ -48,6 +48,9 @@ > > // Set address to listen on. > int set_address (const char *addr_); > + > + // Get the bound address for use with wildcards > + int get_address(unsigned char *addr, size_t *len); Coding style: Use addr_ and len_. > > private: > > diff --git a/src/options.cpp b/src/options.cpp > index 4db1a6c..c8790a8 100644 > --- a/src/options.cpp > +++ b/src/options.cpp > @@ -30,6 +30,7 @@ > rcvhwm (1000), > affinity (0), > identity_size (0), > + last_endpoint_size(0), > rate (100), > recovery_ivl (10000), > multicast_hops (1), > @@ -213,7 +214,6 @@ int zmq::options_t::setsockopt (int option_, const void > *optval_, > ipv4only = val; > return 0; > } > - > } > > errno = EINVAL; > @@ -385,7 +385,15 @@ int zmq::options_t::getsockopt (int option_, void > *optval_, size_t *optvallen_) > *((int*) optval_) = ipv4only; > *optvallen_ = sizeof (int); > return 0; > - > + > + case ZMQ_LAST_ENDPOINT: > + if (*optvallen_ < last_endpoint_size) { > + errno = EINVAL; > + return -1; > + } > + memcpy (optval_, last_endpoint, last_endpoint_size); > + *optvallen_ = last_endpoint_size; > + return 0; > } Security hole: You want to copy last_endpoint_size bytes, or optvallen_ bytes, whichever is lesser, and return the actual # of bytes copied in *optvallen_. > > errno = EINVAL; > diff --git a/src/options.hpp b/src/options.hpp > index bfc9dc7..7feea95 100644 > --- a/src/options.hpp > +++ b/src/options.hpp > @@ -46,6 +46,10 @@ > // Socket identity > unsigned char identity_size; > unsigned char identity [256]; > + > + // Last socket endpoint URI > + unsigned char last_endpoint [256]; > + size_t last_endpoint_size; I guess you wanted to use the ZMQ_ENDPOINT_MAX define here rather than hardcoding 256? > > // Maximum tranfer rate [kb/s]. Default 100kb/s. > int rate; > diff --git a/src/socket_base.cpp b/src/socket_base.cpp > index 3761b46..74c807f 100644 > --- a/src/socket_base.cpp > +++ b/src/socket_base.cpp > @@ -161,6 +161,7 @@ int zmq::socket_base_t::parse_uri (const char *uri_, > } > protocol_ = uri.substr (0, pos); > address_ = uri.substr (pos + 3); > + > if (protocol_.empty () || address_.empty ()) { > errno = EINVAL; > return -1; > @@ -340,6 +341,8 @@ int zmq::socket_base_t::bind (const char *addr_) > delete listener; > return -1; > } > + > + rc = listener->get_address (options.last_endpoint, > &(options.last_endpoint_size)); > launch_child (listener); > return 0; > } > @@ -354,6 +357,7 @@ int zmq::socket_base_t::bind (const char *addr_) > delete listener; > return -1; > } > + rc = listener->get_address (options.last_endpoint, > &(options.last_endpoint_size)); > launch_child (listener); > return 0; > } > diff --git a/src/tcp_address.cpp b/src/tcp_address.cpp > index de6e0ad..9fe6083 100644 > --- a/src/tcp_address.cpp > +++ b/src/tcp_address.cpp > @@ -387,11 +387,18 @@ int zmq::tcp_address_t::resolve (const char *name_, > bool local_, bool ipv4only_) > addr_str [addr_str.size () - 1] == ']') > addr_str = addr_str.substr (1, addr_str.size () - 2); > > - // Parse the port number (0 is not a valid port). > - uint16_t port = (uint16_t) atoi (port_str.c_str()); > - if (port == 0) { > - errno = EINVAL; > - return -1; > + uint16_t port; > + // Allow 0 specifically, to detect invalid port error in atoi if not > + if (port_str[0] == '*' || port_str[0] == '0') { > + // Resolve wildcard to 0 to allow autoselection of port > + port = 0; > + } else { > + // Parse the port number (0 is not a valid port). > + port = (uint16_t) atoi (port_str.c_str()); > + if (port == 0) { > + errno = EINVAL; > + return -1; > + } > } I'm not sure I understand what the business with "Allow 0 specifically" is about? > > // Resolve the IP address. > diff --git a/src/tcp_listener.cpp b/src/tcp_listener.cpp > index 0b7a90d..191e05c 100644 > --- a/src/tcp_listener.cpp > +++ b/src/tcp_listener.cpp > @@ -119,6 +119,37 @@ void zmq::tcp_listener_t::close () > s = retired_fd; > } > > +int zmq::tcp_listener_t::get_address (unsigned char *addr, size_t *len) Coding style: Please use addr_ and len_. > +{ > + struct sockaddr sa; > + char host[INET6_ADDRSTRLEN]; > + int port, rc; > + > + // Get the details of the TCP socket > + socklen_t sl = sizeof(sockaddr); > + rc = getsockname (s, &sa, &sl); > + if (rc != 0) { > + return rc; > + } > + > + // Split the retrieval between IPv4 and v6 addresses > + if ( sa.sa_family == AF_INET ) { > + inet_ntop(AF_INET, &(((struct sockaddr_in *)&sa)->sin_addr), host, > INET6_ADDRSTRLEN); > + port = ntohs( ((struct sockaddr_in *)&sa)->sin_port); > + > + // Store the address for retrieval by users using wildcards > + *len = sprintf((char *)addr, "tcp://%s:%d", host, port); > + } else { > + inet_ntop(AF_INET6, &(((struct sockaddr_in6 *)&sa)->sin6_addr), > host, INET6_ADDRSTRLEN); > + port = ntohs( ((struct sockaddr_in6 *)&sa)->sin6_port); > + > + // Store the address for retrieval by users using wildcards > + *len = sprintf((char *)*addr, "tcp://[%s]:%d", host, port); > + } > + > + return 0; > +} Security hole: Please use snprintf(). (See above) > + > int zmq::tcp_listener_t::set_address (const char *addr_) > { > // Convert the textual address into address structure. > @@ -168,6 +199,7 @@ int zmq::tcp_listener_t::set_address (const char *addr_) > errno_assert (rc == 0); > #endif > > + > // Bind the socket to the network interface and port. > rc = bind (s, address.addr (), address.addrlen ()); > #ifdef ZMQ_HAVE_WINDOWS > diff --git a/src/tcp_listener.hpp b/src/tcp_listener.hpp > index c2116b3..fc6ebd1 100644 > --- a/src/tcp_listener.hpp > +++ b/src/tcp_listener.hpp > @@ -44,6 +44,9 @@ > > // Set address to listen on. > int set_address (const char *addr_); > + > + // Get the bound address for use with wildcard > + int get_address(unsigned char *addr, size_t *len); Coding style: Please use addr_ and len_. One more quick comment about this API: It actually won't work for the most common TCP case (based on what people are asking for), which is: bind("tcp://*:*") The problem is that getsockname() will return INADDR_ANY or IN6ADDR_ANY for such a socket, and such an address is not something you can *connect* to. If the ZMQ_LAST_ENDPOINT API is to be theoretically sound, what guarantees are we providing to the caller, exactly? If the guarantee is "Return an endpoint I can connect to", then you have a problem with INADDR_ANY. I don't see how this can be solved. Any ideas? Off the top of my head trawling through all local interface IP addresses and returning a list of endpoints but that rapidly becomes pretty horrible. <philosophy> We (Martin Sustrik and myself) have always tried to design the ZeroMQ APIs to behave consistently and give explicit guarantees which can be applied to all transports/patterns where possible. In my opinion this is a major win for libzmq, and is an often misunderstood aspect of the library. You don't notice it because it "just works". Many times, the reason something has *not* been implemented is that we'd rather have no implementation than a theoretically unsound one. </philosophy> Cheers, -mato _______________________________________________ zeromq-dev mailing list [email protected] http://lists.zeromq.org/mailman/listinfo/zeromq-dev
