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

[Cole Robinson]

On 05/21/2010 02:33 PM, Eric Blake wrote:
> On 05/21/2010 11:05 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.
>>
>> v2: Leading // must be preserved, properly sanitize path=/, sanitize
>>     away /./ -> /
> 
> Nice - you caught one case that my review did not ("/" as the entire path).
> 
>> +/* 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;
>> +    }
>> +
>> +    /* Starting with // is valid posix, but ///foo == /foo */
>> +    if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') {
>> +        cleanpath[0] = '/';
>> +        cleanpath[1] = '/';
> 
> Delete these two lines; since cleanpath was created by strdup(), they
> are redundant assignments.
> 

Will do

>> +        idx = 2;
>> +        cur += 2;
>> +    }
>> +
>> +    /* Sanitize path in place */
>> +    while (*cur != '\0') {
>> +        if (*cur != '/') {
>> +            cleanpath[idx++] = *cur++;
>> +            continue;
>> +        }
>> +
>> +        /* Skip all extra / */
>> +        while (*cur == '/') {
>> +            cur++;
>> +
>> +            /* Resolve away /./ to just / */
>> +            if (cur[0] == '.' && cur[1] == '/')
>> +                cur++;
>> +        }
> 
> That doesn't simplify "./a" to "a", nor "b/." to "b"
> 

Argh, good point.

>> +
>> +        /* Don't add a trailing / unless path is only made of / */
>> +        if (idx != 0 && *cur == '\0')
>> +            break;
> 
> This incorrectly collapses plain "//" to "/".
> 

Actually we never entered the while() loop with a path of just //, so
this worked in practice.

>> +
>> +        cleanpath[idx++] = '/';
>> +    }
> 
> Maybe a better approach is this pseudo-code:
> 
> if (2 leading slashes) {
>   advance 2
>   if (end) return
> } else if (leading slash)
>   advance 1
> do {
>   advance past all slashes - they are redundant from previous round
>   if (./ or .-end) advance 1 and continue
>   copy all non-slashes
>   copy one slash
> } while (!end)
> if (trailing slash)
>   back up 1
> 

I didn't end up going with this configuration, but the resulting code
isn't too bad, and seems to cover all the tests I could think up.
Updated patch sending shortly.

Thanks,
Cole