[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 02/13] cxenstored: add support for systemd active sockets
On Wed, Jul 02, 2014 at 02:02:38PM +0100, Ian Campbell wrote: > On Thu, 2014-06-12 at 18:18 -0700, Luis R. Rodriguez wrote: > > +int xs_validate_active_socket(const char *connect_to) > > +{ > > + char sock[30]; > > + > > + /* We have to null terminate the socket path */ > > + memset(sock, '\0', sizeof(sock)); > > + memcpy(sock, connect_to, strlen(connect_to)); > > This risks overrunning sock if connect_to is longer than 30 characters. Yuk, yes. A size check is required. > But your use of strlen suggests that connect_to is already NULL > terminated, so what is this for? strlen() seems to want the string to also be null terminated in order to work, and I also see that snprintf() is ultimately used on the C version of the library, that should ensure its null terminated. This however is not true for the ocaml version and I suppose that is the root of the issue I saw that got me to force null terimination as I did run into issues with this path IIRC when not null terminated. > > + if ((strncmp("/var/run/xenstored/socket_ro", sock, 28) != 0) && > > + (strncmp("/var/run/xenstored/socket", sock, 25) != 0)) { > > Given that sock (or connect_to) is NULL terminated, why strncmp and not > the straightforward strcmp? See above. > As it is I think your code would accept > e.g. /var/run/xenstored/socketwibble, no? It indeed would, its best if we resolve the null termination issue internally then. > Is it common in systemd support to validate the input socket's path in > this way? As opposed to trusting that the systemd unit file is correct. sd_is_socket_unix() is certainly advised and highly encourged. As for the other check -- yes we want that given that we are using two socket files specifically in order to avoid having to chmod() the socket ourelves. Systemd currently doesn't support having two separate sockets in one file with different permissions. From what I have seen the order in which systemd sets the sockets and maps them to fds will very on the unit socket files you use, this approach lets us allow systemd to give them to us in any order while my hope is that systemd upstream gets support for just one stocket file with multiple permissions. > > +static void xen_claim_active_sockets(int **psock, int **pro_sock) > > +{ > > + int *sock, *ro_sock; > > + const char *soc_str = xs_daemon_socket(); > > + const char *soc_str_ro = xs_daemon_socket_ro(); > > + int n; > > + > > + n = sd_listen_fds(0); > > + if (n <= 0) { > > + sd_notifyf(0, "STATUS=Failed to get any active sockets: %s\n" > > + "ERRNO=%i", > > + strerror(errno), > > + errno); > > + barf_perror("sd_listen_fds() failed\n"); > > + } else if (n > 2) { > > + fprintf(stderr, SD_ERR "Expected 2 fds but given %d\n", n); > > + sd_notifyf(0, "STATUS=Mismatch on number (2): %s\n" > > + "ERRNO=%d", > > + strerror(EBADR), > > + EBADR); > > + barf_perror("sd_listen_fds() gave too many fds\n"); > > Need this be fatal? You can do anything you want but IMO yes, otherwise we'd be misconfigured and there is a mismatch. Luis _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |