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

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



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.

> +        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"

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

This incorrectly collapses plain "//" to "/".

> +
> +        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

> +    cleanpath[idx] = '\0';
> +
> +    return cleanpath;
> +}
> +

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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