Re: [Xen-devel] [PATCH v4 07/33] xen: guestcopy: Provide an helper to safely copy string from guest

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Andrew,

On 31/03/15 14:24, Andrew Cooper wrote:
>> +/*
>> + * The function copies a string from the guest and adds a NUL to
>> + * make sure the string is correctly terminated.
>> + */
>> +void *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
>> +                                  size_t size, size_t max_size)
> 
> If it is a NUL terminated string, you should return char* rather than void*.

Ok.

> Furthermore, two size parameters serves no useful purpose.  The caller
> must always be in a position to decide a plausible upper bound.

I don't understand the problem to have two size parameters...

The first one is the size given by the guest while the second one if the
upper bound.

The maximum size may change from every caller. Hence the second size
parameter.

>> +{
>> +    char *tmp;
>> +
>> +    if ( size > max_size )
>> +        return ERR_PTR(-ENOBUFS);
>> +
>> +    /* Add an extra +1 to append \0 */
>> +    tmp = xmalloc_array(char, size + 1);
> 
> Need to check that size + 1 doesn't overflow to 0.

With the max_size controlled by the caller there is no need to check the
overflow.

If the caller decides to give a too high max_size then it's its problem.

> 
>> +    if ( !tmp )
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    if ( copy_from_guest(tmp, u_buf, size) )
>> +    {
>> +        xfree(tmp);
>> +        return ERR_PTR(-EFAULT);
>> +    }
>> +    tmp[size] = 0;
> 
> '\0' please.

Ok.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel