[libvirt] [PATCH] storage: Sanitize pool target paths

[Cole Robinson]

On 05/20/2010 03:28 PM, Eric Blake wrote:
> On 05/20/2010 10:04 AM, Cole Robinson wrote:
>> Spurious / in a pool target path makes life difficult for apps using the
>> GetVolByPath, and doing other path based comparisons with pools. This
>> has caused a few issues for virt-manager users:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=494005
>> https://bugzilla.redhat.com/show_bug.cgi?id=593565
>>
>> Add a new util API which removes spurious /, virFileSanitizePath. Sanitize
>> target paths when parsing pool XML, and for paths passed to GetVolByPath.
> 
> In general, I like the idea, whether or not we should be moving the
> sanitization into a gnulib function.  But I don't think it's ready for
> an ack quite yet.
> 
>> index e937d39..8f86ed6 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -1921,6 +1921,41 @@ int virFileAbsPath(const char *path, char **abspath)
>>      return 0;
>>  }
>>  
>> +/* Remove spurious / characters from a path. The result must be freed */
>> +char *
>> +virFileSanitizePath(const char *path)
>> +{
>> +    const char *cur = path;
>> +    char *cleanpath;
>> +    int idx = 0;
>> +
>> +    cleanpath = strdup(path);
>> +    if (!cleanpath) {
>> +        virReportOOMError();
>> +        return NULL;
>> +    }
>> +
>> +    /* Sanitize path in place */
>> +    while (*cur != '\0') {
>> +        if (*cur == '/') {
>> +            /* Skip all extra / */
>> +            while (*++cur == '/')
>> +                continue;
> 
> POSIX says that you must be careful of "//file" - you cannot blindly
> simplify to "/file" (case in point - cygwin).  But POSIX also says that
> "///file" sanitizes to "/file".
> 
>> +
>> +            /* Don't add a trailing / */
>> +            if (*cur == '\0')
>> +                break;
>> +
>> +            cleanpath[idx++] = '/';
>> +        }
>> +
>> +        cleanpath[idx++] = *cur++;
>> +    }
>> +    cleanpath[idx] = '\0';
>> +
>> +    return cleanpath;
> 
> Sanitizing "a/b/../c" to "a/c" might not always be the right thing (what
> if a is a symlink?), but it would also be safe to sanitize "a/./b" to "a/b".
> 
>> +}
>> +
>>  /* Like strtol, but produce an "int" result, and check more carefully.
>>     Return 0 upon success;  return -1 to indicate failure.
>>     When END_PTR is NULL, the byte after the final valid digit must be NUL.
>> diff --git a/src/util/util.h b/src/util/util.h
>> index 6bf6bcc..abc2688 100644
>> --- a/src/util/util.h
>> +++ b/src/util/util.h
>> @@ -118,6 +118,8 @@ char *virFindFileInPath(const char *file);
>>  
>>  int virFileExists(const char *path);
>>  
>> +char *virFileSanitizePath(const char *path);
> 
> Mark this with ATTRIBUTE_NONNULL(1).
> 

Thanks for the review, updated patch sent.

- Cole