[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] absolutePathFromBaseFile: don't leak when first arg contains no "/"



Daniel Veillard wrote:
>>  absolutePathFromBaseFile(const char *base_file, const char *path)
>>  {
>> -    size_t base_size, path_size;
>> -    char *res, *p;
>> +    char *res;
>> +    size_t d_len = dir_len (base_file);
>>
>> -    if (*path == '/')
>> +    /* If path is already absolute, or if dirname(base_file) is ".",
>> +       just return a copy of path.  */
>> +    if (*path == '/' || d_len == 0)
>>          return strdup(path);
>>
>> -    base_size = strlen(base_file) + 1;
>> -    path_size = strlen(path) + 1;
>> -    if (VIR_ALLOC_N(res, base_size - 1 + path_size) < 0)
>> +    if (VIR_ALLOC_N(res, d_len + 1 + strlen(path) + 1) < 0)
>>          return NULL;
>> -    memcpy(res, base_file, base_size);
>> -    p = strrchr(res, '/');
>> -    if (p != NULL)
>> -        p++;
>> -    else
>> -        p = res;
>> -    memcpy(p, path, path_size);
>> -    if (VIR_REALLOC_N(res, (p + path_size) - res) < 0) {
>> -        /* Ignore failure */
>> -    }
>> +
>> +    stpcpy(stpcpy(stpncpy(res, base_file, d_len), "/"), path);
>>      return res;
>
>   Okay, but while we're cleaning it up, I would suggest to do a
> virReportOOMError(NULL) in case of allocation failure and remove
> the one from virStorageFileGetMetadataFromFD() since it's used only
> once. I think the conn arg is not useful anymore and it's better
> to report the error when it occurs than at the caller level.
>   BTW I didn't know about stp(n)cpy ...

Since the "return strdup(..." can also fail due to an OOM error,
isn't it better to handle all NULL returns in the caller?


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]