[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
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |