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

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

On Thu, Jul 03, 2014 at 10:13:05AM +0100, Ian Campbell wrote:
> 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.

OK, or we ensure Ocaml's implementation provides a null terminated
string to try to keep the systemd interface similar, poking Dave for
feedback:

String_val() is used for the static string connect_to passed to the
C wrapper, the String_val() documentation [0] says that "there is a null
character after the last character in the string" however its unclear
if this is guaranteed if the string was originally a static Ocaml string
which was not null terminated. 

Now, its unclear which xenstored (cxenstored or oxenstored) gave me issues
that pushed me to ensure I give systemd null terminated strings but
I do know that it was one for sure and it took me quite a bit to figure
out this was needed and that this was the issue. Based on my review just
now since the cxenstored uses snprintf() for both xs_daemon_socket_ro()
and xs_daemon_socket() and since snprintf() man page says that it will
write at most size bytes (including the terminating null byte ('\0') I
am left to only grow suspcicious of the oxenstored as the probable
cause of the issues I saw. I should also highlight that the socket path
can also come from environment variables on cxenstored, XENSTORED_PATH,
and getenv() documentation doesn't say whether or not strings will be
null terminated for us, that might cause unexpected issues if used and
if it doesn't on systemd.

In v5 systemd integration implementation in which I used static structs
for the strings in C I had no issues but note that in that case I simply
used the Ocaml string (after String_val()) to and compare it to the one
on the static C array with:

	(!strcmp(connect_to, xenstore_active_sockets[i].path))

I then used the C static string for sd_is_socket_unix(), not the one
passed from cxenstored or oxenstored.

[0] http://caml.inria.fr/pub/docs/manual-ocaml-400/manual033.html

> > > 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),

tools/xenstore/xenstored_core.c uses chmod to change the
permissions:

        if (chmod(soc_str, 0600) != 0
            || chmod(soc_str_ro, 0660) != 0)
                barf_perror("Could not chmod sockets");

> it's the owner/group which might differ.

On my latest Debian install of the master branch for legacy init
my sockets end up being owned by root, who should be owning the
sockets?

> (You need to be able to write to the ro socket to send operations,

BTW what is the _ro socket used for exactly? I couldn't find usage for
it in existing code.

> the distinction is that the ro socket will reject XS_WRITE messages)

OK, got it.

  Luis