[libvirt] [PATCH] Introduce virStrcpy.

[Chris Lalancette]

Daniel P. Berrange wrote:
> On Fri, Aug 07, 2009 at 11:06:29AM +0200, Chris Lalancette wrote:
>> diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
>> index e008a7f..42084c7 100644
>> --- a/proxy/libvirt_proxy.c
>> +++ b/proxy/libvirt_proxy.c
>> @@ -167,10 +167,13 @@ proxyListenUnixSocket(const char *path) {
>>       * Abstract socket do not hit the filesystem, way more secure and
>>       * guaranteed to be atomic
>>       */
>> -    memset(&addr, 0, sizeof(addr));
>>      addr.sun_family = AF_UNIX;
>>      addr.sun_path[0] = '\0';
>> -    strncpy(&addr.sun_path[1], path, (sizeof(addr) - 4) - 2);
>> +    if (virStrcpy(&addr.sun_path[1], path, sizeof(addr.sun_path) - 1) == NULL) {
>> +        fprintf(stderr, "Path %s too long to fit into destination\n", path);
>> +        close(fd);
>> +        return -1;
>> +    }
> 
> Removing the memset is not safe - the abstract namesapce defines
> the name to be the *entire* addr.sun_path  array. So it must be
> null filled, otherwise the name will be the string copied + whatever
> trailing garbage exists

Ug, OK, thanks, I've fixed all instances of this.

>> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
>> index 37fdec2..bcacd41 100644
>> --- a/src/qemu_driver.c
>> +++ b/src/qemu_driver.c
>> @@ -5802,8 +5806,11 @@ static int qemuGetSchedulerParameters(virDomainPtr dom,
>>          goto cleanup;
>>      }
>>      params[0].value.ul = val;
>> -    strncpy(params[0].field, "cpu_shares", sizeof(params[0].field));
>> -    params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG;
>> +    if (virStrcpy(params[0].field, "cpu_shares", sizeof(params[0].field)) == NULL) {
>> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
>> +                         "%s", _("Field cpu_shares too long for destination"));
>> +        goto cleanup;
>> +    }
> 
> Why remove the assginemnt to 'type' here ?

Oversight on my part, thanks for pointing it out.  Fixed now.

>> @@ -1573,6 +1572,28 @@ virAsprintf(char **strp, const char *fmt, ...)
>>      return ret;
>>  }
>>  
>> +/**
>> + * virStrcpy
>> + *
>> + * A safe version of strcpy.  The last parameter is the number of bytes
>> + * available in the destination string, *not* the number of bytes you want
>> + * to copy.  If the destination is not large enough to hold all of the
>> + * src string, NULL is returned and no data is copied.  If the destination
>> + * is large enough to hold all of the src, then the string is copied and
>> + * a pointer to the destination string is returned.
>> + */
>> +char *
>> +virStrcpy(char *dest, const char *src, size_t destbytes)
>> +{
>> +    int len;
>> +
>> +    len = strlen(src) + 1;
>> +    if (len > destbytes)
>> +        return NULL;
>> +
>> +    return memcpy(dest, src, len);
>> +}
> 
> Why not just use strncpy() and ensure the trailing null. Seems
> like it would be more efficient than requiring 2 passes over
> the string.

Hm, I'm not entirely sure I understand you.  Do you mean something like:

char *
virStrcpy(char *dest, const char *src, size_t destbytes)
{
    char *ret;

    ret = strncpy(dest, src, destbytes);
    if (ret)
       ret[destbytes - 1] = '\0';

    return ret;
}

But in this case, there is nowhere that I can return NULL if the destination is
too short.  Any solution I can think of in the current form requires me to run
strlen over the source to check that it isn't too big for the dest.

Hm, here's another thought.  Maybe I should make the signature:

char *
virStrcpy(char *dest, const char *src, size_t srclen, size_t destbytes);

Then, the first three parameters would actually be exactly the same as for
strncpy() (meaning that you could tell it to stop copying short of the \0 in
src).  The additional destbytes parameter is the safety parameter to make sure
the dest is big enough.  Of course, this sort of just punts the two-pass
strlen() problem off to the callers, but for callers that already know the
length they can just pass it in here without an additional penalty.

Or did you have another idea?

>> index b3e628a..ffbf2b3 100644
>> --- a/src/util.h
>> +++ b/src/util.h
>> @@ -164,6 +164,7 @@ void virSkipSpaces(const char **str);
>>  int virParseNumber(const char **str);
>>  int virAsprintf(char **strp, const char *fmt, ...)
>>      ATTRIBUTE_FMT_PRINTF(2, 3);
>> +char *virStrcpy(char *dest, const char *src, size_t destbytes);
> 
> THis is probably a candidate for ATTRIBUTE_RETURN_CHECK to 
> guarentee that all callers are dealing with the failure case
> at compile time

Done.

> 
> 
>> @@ -1026,42 +1043,46 @@ xenXMDomainConfigParse(virConnectPtr conn, virConfPtr conf) {
>>                  data++;
>>  
>>                  if (STRPREFIX(key, "mac=")) {
>> -                    int len = nextkey ? (nextkey - data) : 17;
>> -                    if (len > 17)
>> -                        len = 17;
>> -                    strncpy(mac, data, len);
>> -                    mac[len] = '\0';
>> +                    if (virStrcpy(mac, data, sizeof(mac)) == NULL) {
>> +                        xenXMError(conn, VIR_ERR_INTERNAL_ERROR,
>> +                                   _("MAC address %s too big for destination"),
>> +                                   data);
>> +                        goto cleanup;
>> +                    }
> 
> This doesn't seem correct to me, since 'data' is not NULL terminated
> at the place we want the copy to stop. The string is of the form
> 
>   foo=bar,mac=XYZ,foo=bar
> 
> and the original code would only copy XYZ, while this new code
> will copy the maximum size of the destination buffer, or the
> remaining of the source string, including the next field.
> I'm surprised if the xmconfigtest test case passes with this,
> unless i'm mis-reading something

No, you are totally correct.  I've changed this up now to a solution that I
think should work, so it will be in the next posting.

-- 
Chris Lalancette