Re: [PATCH v6 02/13] cxenstored: add support for systemd active sockets

["Luis R. Rodriguez" <mcgrof@suse.com>]

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