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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

Hi Luis,

Sorry I missed this question earlier.

On 17 Jul 2014, at 20:45, Luis R. Rodriguez <mcgrof@xxxxxxxx> wrote:

> 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.

OCaml strings are best thought of as a pair of a byte array and a length. From 
the point of view of the OCaml program they don’t contain NULLs unless you put 
some in there yourself. OCaml code which actually writes NULLs into strings is 
fairly rare.

The manual, although terse, is trustworthy. If you use String_val on an OCaml 
string you’ll get a char* which is guaranteed to be 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 believe that’s true.

>> 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.

If the strings compare equal with ‘strcmp’ then I would have thought either 
would work, unless the consumer is holding onto the string pointer for longer 
than the call duration. As soon as your function returns (or allocates) the 
OCaml strings can be moved around which invalidates the result of String_val— 
perhaps this is the cause of the difference between the String_val result and 
your static strings?

Dave

>> 
>> [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