Re: [Qemu-devel] [PATCH] rev3: support colon in filenames

[Kevin Wolf]

Ram Pai schrieb:
> Problem: It is impossible to feed filenames with the character colon because
> qemu interprets such names as a protocol. For example filename scsi:0, is
> interpreted as a protocol by name "scsi".
> 
> This patch allows user to escape colon characters. For example the above
> filename can now be expressed either as 'scsi\:0' or as file:scsi:0
> 
> anything following the "file:" tag is interpreted verbatim. However if "file:"
> tag is omitted then any colon characters in the string must be escaped using
> backslash.

Anthony has already committed version 2 of the patch, so this one
doesn't apply any more.

By the way, I'm still not convinced that this use of backslashes gives
us anything but yet another special character that worked just fine
before. I guess this is going to be annoying for Windows users.

> Here are couple of examples:
> 
> scsi\:0\:abc is a local file scsi:0:abc
> http\://myweb is a local file by name http://myweb
> file:scsi:0:abc is a local file scsi:0:abc
> file:http://myweb is a local file by name http://myweb
> 
> fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
> NOTE:The above example cannot be expressed using the "file:" protocol.

And it doesn't need to. It's already expressed using the "fat:"
protocol, so we won't accidentally mistake c for the protocol name.

You might have a point with a directory named :floppy: or so.

> Changelog w.r.t to iteration 0:
>    1) removes flexibility added to nbd semantics  eg -- nbd:\::9999
>    2) introduce the file: protocol to indicate local file
> 
> Changelog w.r.t to iteration 1:
>    1) generically handles 'file:' protocol in find_protocol
>    2) centralizes 'filename' pruning before the call to open().
>    3) fixes buffer overflow seen in fill_token()
>    4) adheres to coding style
>    5) patch against upstream qemu tree
> 
> Changelog w.r.t to iteration 2:
>    1) really really fixes buffer overflow seen in fill_token()
>    2) the centralized 'filename' pruning had a side effect with
>       qcow2 files and other files. Fixed it. _open() is back.
> 
> Signed-off-by: Ram Pai <address@hidden>
> 
>  block.c           |   10 +----
>  block/raw-posix.c |   15 ++++----
>  block/vvfat.c     |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  cutils.c          |   40 +++++++++++++++++++++
>  qemu-common.h     |    2 +
>  5 files changed, 148 insertions(+), 19 deletions(-)
> 
> diff --git a/block.c b/block.c
> index aca5a6d..7ad4dd9 100644
> --- a/block.c
> +++ b/block.c
> @@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
>  {
>      BlockDriver *drv1;
>      char protocol[128];
> -    int len;
>      const char *p;
>  
>  #ifdef _WIN32
> @@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
>          is_windows_drive_prefix(filename))
>          return bdrv_find_format("raw");
>  #endif
> -    p = strchr(filename, ':');
> -    if (!p)
> +    p = prune_strcpy(protocol, 128, filename, ':');

Maybe better sizeof instead of writing 128 explicitly?

> +    if (*p != ':')
>          return bdrv_find_format("raw");
> -    len = p - filename;
> -    if (len > sizeof(protocol) - 1)
> -        len = sizeof(protocol) - 1;
> -    memcpy(protocol, filename, len);
> -    protocol[len] = '\0';
>      for(drv1 = first_drv; drv1 != NULL; drv1 = drv1->next) {
>          if (drv1->protocol_name &&
>              !strcmp(drv1->protocol_name, protocol))
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 41bfa37..8a0c0df 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -151,7 +151,7 @@ static int raw_open_common(BlockDriverState *bs, const 
> char *filename,
>          s->open_flags |= O_DSYNC;
>  
>      s->fd = -1;
> -    fd = open(filename, s->open_flags, 0644);
> +    fd = _open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -844,7 +844,7 @@ static int raw_create(const char *filename, 
> QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -889,6 +889,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength = raw_getlength,
>  
>      .create_options = raw_create_options,
> +    .protocol_name = "file",
>  };
>  
>  /***********************************************/
> @@ -985,7 +986,7 @@ static int hdev_open(BlockDriverState *bs, const char 
> *filename, int flags)
>          if ( bsdPath[ 0 ] != '\0' ) {
>              strcat(bsdPath,"s0");
>              /* some CDs don't have a partition 0 */
> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
> +            fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1037,7 +1038,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = _open(bs->filename, s->open_flags & ~O_NONBLOCK);
>          if (s->fd < 0) {
>              s->fd_error_time = qemu_get_clock(rt_clock);
>              s->fd_got_error = 1;
> @@ -1133,7 +1134,7 @@ static int hdev_create(const char *filename, 
> QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = _open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
>  
> @@ -1239,7 +1240,7 @@ static int floppy_eject(BlockDriverState *bs, int 
> eject_flag)
>          close(s->fd);
>          s->fd = -1;
>      }
> -    fd = open(bs->filename, s->open_flags | O_NONBLOCK);
> +    fd = _open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1399,7 +1400,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = _open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;

What about raw-win32.c?

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 1e37b9f..4729165 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -76,6 +76,97 @@ typedef struct array_t {
>      unsigned int size,next,item_size;
>  } array_t;
>  
> +/*
> + * prunes out all escape characters as per the following rule
> + * '\\' -> '\'
> + * '\:' -> ':'
> + * '\,' -> ','
> + * '\x' -> '\x'
> + * return a new pruned string.
> + * NOTE: remember to free that string.
> + */
> +static char *escape_strdup(const char *str)
> +{
> +#define NORMAL  0
> +#define ESCAPED 1
> +    int len = strlen(str);
> +    char *s = qemu_malloc(len+1);
> +    char *q = s;
> +    const char *p=str;
> +    int state=NORMAL;
> +
> +    while (p < str+len) {
> +        switch (state) {
> +        case NORMAL:
> +            switch (*p) {
> +            case '\\' : state=ESCAPED; p++ ; break;

One statement per line.

> +            default: *q++=*p++; break;
> +            }
> +         break;
> +        case ESCAPED:
> +            switch (*p) {
> +            case '\\' :
> +            case ',' :
> +            case ':':
> +                     break;
> +
> +            default: *q++='\\';break;
> +            }
> +            state = NORMAL;
> +            *q++=*p++;
> +         break;

Indentation looks wrong here. Tabs?

> +        }
> +   }
> +   *q = '\0';
> +   return s;
> +}
> +
> +/*
> + * return the index of the rightmost delimitor in the string 'str'
> + */
> +static int find_rdelim(const char *str, const char delimitor)
> +{
> +#define NOT_FOUND 1
> +#define MAY_HAVE_FOUND 2
> +#define MAY_NOT_HAVE_FOUND 3
> +    const char *f = str + strlen(str) -1;
> +    char state = NOT_FOUND;
> +    const char *loc = f;
> +
> +    while (f >= str) {
> +        char c = *f--;
> +        switch (state) {
> +        case NOT_FOUND:
> +            if (c == delimitor) {
> +                state=MAY_HAVE_FOUND;
> +                loc=f+1;
> +            }
> +            break;
> +        case MAY_HAVE_FOUND:
> +            if (c == '\\') {
> +                 state=MAY_NOT_HAVE_FOUND;
> +            } else {
> +                 goto out;
> +            }
> +            break;
> +        case MAY_NOT_HAVE_FOUND:
> +            if (c == '\\') {
> +                state=MAY_HAVE_FOUND;
> +            } else if ( c == delimitor ) {
> +                state=MAY_HAVE_FOUND;
> +                loc=f+1;
> +            } else {
> +                state=NOT_FOUND;
> +            }
> +            break;
> +        }
> +    }
> +    loc=f;
> +out:
> +    return (loc-str);
> +}
> +
> +
>  static inline void array_init(array_t* array,unsigned int item_size)
>  {
>      array->pointer = NULL;
> @@ -882,7 +973,7 @@ static int init_directories(BDRVVVFATState* s,
>      mapping->dir_index = 0;
>      mapping->info.dir.parent_mapping_index = -1;
>      mapping->first_mapping_index = -1;
> -    mapping->path = strdup(dirname);
> +    mapping->path = escape_strdup(dirname);
>      i = strlen(mapping->path);
>      if (i > 0 && mapping->path[i - 1] == '/')
>       mapping->path[i - 1] = '\0';
> @@ -1055,7 +1146,7 @@ DLOG(if (stderr == NULL) {
>       bs->read_only = 0;
>      }
>  
> -    i = strrchr(dirname, ':') - dirname;
> +    i = find_rdelim(dirname, ':'); /* find the rightmost unescaped colon */
>      assert(i >= 3);
>      if (dirname[i-2] == ':' && qemu_isalpha(dirname[i-1]))
>       /* workaround for DOS drive names */
> @@ -1081,6 +1172,7 @@ DLOG(if (stderr == NULL) {
>      return 0;
>  }
>  
> +
>  static inline void vvfat_close_current_file(BDRVVVFATState *s)
>  {
>      if(s->current_mapping) {
> @@ -1162,7 +1254,7 @@ static int open_file(BDRVVVFATState* s,mapping_t* 
> mapping)
>      if(!s->current_mapping ||
>           strcmp(s->current_mapping->path,mapping->path)) {
>       /* open file */
> -     int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
> +     int fd = _open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
>       if(fd<0)
>           return -1;
>       vvfat_close_current_file(s);
> @@ -2222,7 +2314,7 @@ static int commit_one_file(BDRVVVFATState* s,
>      for (i = s->cluster_size; i < offset; i += s->cluster_size)
>       c = modified_fat_get(s, c);
>  
> -    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
> +    fd = _open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
>      if (fd < 0) {
>       fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
>               strerror(errno), errno);
> diff --git a/cutils.c b/cutils.c
> index 6ea0c49..05f570b 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -24,6 +24,32 @@
>  #include "qemu-common.h"
>  #include "host-utils.h"
>  
> +/*
> + * copy contents of 'str' into buf until the first unescaped
> + * character 'c'. Escape character '\' is pruned off.
> + * Return pointer to the delimiting character
> + */
> +const char *prune_strcpy(char *buf, int len, const char *str, const char c)
> +{
> +    const char *p=str;
> +    char *q=buf;
> +
> +    len = strnlen(str, len-1);
> +    while (p < str+len) {
> +        if (*p == c)
> +            break;
> +        if (*p == '\\') {
> +            p++;
> +            if (*p == '\0')
> +                break;
> +        }
> +        *q++ = *p++;
> +    }
> +    *q='\0';
> +    return p;
> +}

I like this function so much more now that it behaves like you would
expect from a string function. :-)

Corner case: len = 0 is broken.

> +
> +
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
>      int c;
> @@ -184,3 +210,17 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const 
> void *buf, size_t count)
>          count -= copy;
>      }
>  }
> +
> +int _open(const char *filename, int flags, ...)
> +{
> +    char myfile[PATH_MAX];
> +    const char *f;
> +    va_list ap;
> +    va_start(ap, flags);
> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, PATH_MAX, filename, '\0');
> +        return  open(myfile, flags, ap);
> +    }
> +    return  open(f, flags, ap);

This is still wrong. You can't feed ap to open.

> +}
> diff --git a/qemu-common.h b/qemu-common.h
> index 2dcb224..41da8b0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -104,12 +104,14 @@ void qemu_get_timedate(struct tm *tm, int offset);
>  int qemu_timedate_diff(struct tm *tm);
>  
>  /* cutils.c */
> +const char *prune_strcpy(char *buf, int buf_size, const char *str, const 
> char);
>  void pstrcpy(char *buf, int buf_size, const char *str);
>  char *pstrcat(char *buf, int buf_size, const char *s);
>  int strstart(const char *str, const char *val, const char **ptr);
>  int stristart(const char *str, const char *val, const char **ptr);
>  time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
> +int _open(const char *filename, int flags, ...);
>  
>  #define qemu_isalnum(c)              isalnum((unsigned char)(c))
>  #define qemu_isalpha(c)              isalpha((unsigned char)(c))

Otherwise I see no obvious problems. I haven't actually tested the code,
though.

Kevin