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

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

David, Anil, please see below.

On Thu, Jul 03, 2014 at 07:06:24PM +0200, Luis R. Rodriguez wrote:
> 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:

Dave, Anil, or other ocaml folks -- feedback is is appreciated on the questions
below.

> 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

  Luis