[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, 2014-07-02 at 21:00 +0200, Luis R. Rodriguez wrote: > 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. Yeah, I think we should write the C version according to normal C string conventions. If the ocaml idea of a string differs then perhaps that version needs to be different. > > 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. OK. BTW, the permissions on these two sockets should be the same (read/write), it's the owner/group which might differ. (You need to be able to write to the ro socket to send operations, the distinction is that the ro socket will reject XS_WRITE messages) > > > +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. OK. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |