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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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