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

[Kevin Wolf <kwolf@redhat.com>]

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 espace 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 verbatin. However if "file:"
> tag is omitted then any colon characters in the string must be escaped using
> backslash.
> 
> 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.
> 
> 
> 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 codying style
>    5) patch against upstream qemu tree
> 
> Changelog w.r.t to iteration 2:
>    1) really really fixes buffer overflow seen in 
> 	fill_token() (if not, beat me :)
>    2) the centralized 'filename' pruning had a side effect with
> 	qcow2 files and other files. Fixed it. _open() is back.
> 
> Changelog w.r.t to iteration 3:
>    1) support added to raw-win32.c (i do not have the setup to 
> 		test this change. Request help with testing)
>    2) ability to espace option-values containing commas using 
> 	backslashes 
> 	eg  file=file:abc,,  can also be expressed as file=file:abc\, 
> 		where 'abc,' is a filename
>    3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
>    4) renamed _open() to qemu_open() and removed dependency on PATH_MAX
> 
> Changelog w.r.t to iteration 4:
>    1) applies to upstream qemu tree
> 
> Changelog w.r.t to iteration 5:
>    1) fixed a issue with backing_filename for qcow2 files,
> 		reported by Jamie Lokier.
>    2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
>     		(I do not have the setup to test win32 changes. 
> 		 Request help with testing)
> 
> Changelog w.r.t to iteration 6:
>    1) fixed all the issues found with win32. 
> 	a) changed the call to strnlen() to qemu_strlen() in cutils.c
>         b) fixed the call to CreateFile() in qemu_CreateFile()
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>          
> 
>  block.c           |   38 ++++++++-------------
>  block/raw-posix.c |   15 ++++----
>  block/raw-win32.c |   26 ++++++++++++--
>  block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  cutils.c          |   46 +++++++++++++++++++++++++
>  qemu-common.h     |    2 +
>  qemu-option.c     |    8 ++++-
>  7 files changed, 195 insertions(+), 37 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 39f726c..da6eaf7 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, sizeof(protocol), filename, ':');
> +    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))
> @@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>  {
>      int ret, open_flags;
>      char tmp_filename[PATH_MAX];
> -    char backing_filename[PATH_MAX];
>  
>      bs->read_only = 0;
>      bs->is_temporary = 0;
> @@ -343,7 +336,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>      if (flags & BDRV_O_SNAPSHOT) {
>          BlockDriverState *bs1;
>          int64_t total_size;
> -        int is_protocol = 0;
>          BlockDriver *bdrv_qcow2;
>          QEMUOptionParameter *options;
>  
> @@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>          }
>          total_size = bdrv_getlength(bs1) >> SECTOR_BITS;
>  
> -        if (bs1->drv && bs1->drv->protocol_name)
> -            is_protocol = 1;
> -
>          bdrv_delete(bs1);
>  
>          get_tmp_filename(tmp_filename, sizeof(tmp_filename));
>  
> -        /* Real path is meaningless for protocols */
> -        if (is_protocol)
> -            snprintf(backing_filename, sizeof(backing_filename),
> -                     "%s", filename);
> -        else
> -            realpath(filename, backing_filename);
> -

How is this change related to the colon problem? Is it really needed? It
seems to me that you just change the details of the implementation (you
add an if instead below).

I can't see how this is needed, but if it is, I'm pretty sure that it's
something for a different patch.

>          bdrv_qcow2 = bdrv_find_format("qcow2");
>          options = parse_option_parameters("", bdrv_qcow2->create_options, NULL);
>  
>          set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size * 512);
> -        set_option_parameter(options, BLOCK_OPT_BACKING_FILE, backing_filename);
> +        set_option_parameter(options, BLOCK_OPT_BACKING_FILE, filename);
>          if (drv) {
>              set_option_parameter(options, BLOCK_OPT_BACKING_FMT,
>                  drv->format_name);
> @@ -437,15 +419,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>      }
>  #endif
>      if (bs->backing_file[0] != '\0') {
> +        char *backing_file;
> +
>          /* if there is a backing file, use it */
>          BlockDriver *back_drv = NULL;
>          bs->backing_hd = bdrv_new("");
> -        path_combine(backing_filename, sizeof(backing_filename),
> +
> +        if (flags & BDRV_O_SNAPSHOT) {
> +            backing_file = bs->backing_file;
> +        } else {
> +            path_combine(tmp_filename, sizeof(tmp_filename),
>                       filename, bs->backing_file);
> +            backing_file = tmp_filename;
> +        }
> +
>          if (bs->backing_format[0] != '\0')
>              back_drv = bdrv_find_format(bs->backing_format);
> -        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
> -                         back_drv);
> +        ret = bdrv_open2(bs->backing_hd, backing_file, open_flags, back_drv);
>          if (ret < 0) {
>              bdrv_close(bs);
>              return ret;
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 389903e..030796d 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -154,7 +154,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 = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> @@ -863,7 +863,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0) {
>          result = -errno;
> @@ -914,6 +914,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength = raw_getlength,
>  
>      .create_options = raw_create_options,
> +    .protocol_name = "file",
>  };
>  
>  /***********************************************/
> @@ -1010,7 +1011,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 = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -1062,7 +1063,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = qemu_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;
> @@ -1158,7 +1159,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = qemu_open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -EIO;
>  
> @@ -1263,7 +1264,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 = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1422,7 +1423,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = qemu_open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 72acad5..24d706e 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -38,6 +38,25 @@ typedef struct BDRVRawState {
>      char drive_path[16]; /* format: "d:\" */
>  } BDRVRawState;
>  
> +static HANDLE qemu_CreateFile(const char *filename, int access_flags,
> +                       int share_flags, LPSECURITY_ATTRIBUTES sec,
> +                       int create_flags, DWORD overlapped, HANDLE handle)
> +{
> +    const char *f;
> +    int len = strlen(filename)+1;
> +    HANDLE fd;
> +    char *myfile = qemu_malloc(len);
> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, len, filename, '\0');
> +        f = myfile;
> +    }
> +    fd = CreateFile(f, access_flags, share_flags, sec,
> +                          create_flags, overlapped, handle);
> +    qemu_free(myfile);
> +    return fd;
> +}
> +
>  int qemu_ftruncate64(int fd, int64_t length)
>  {
>      LARGE_INTEGER li;
> @@ -96,7 +115,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
>          overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH;
>      else if (!(flags & BDRV_O_CACHE_WB))
>          overlapped |= FILE_FLAG_WRITE_THROUGH;
> -    s->hfile = CreateFile(filename, access_flags,
> +    s->hfile = qemu_CreateFile(filename, access_flags,
>                            FILE_SHARE_READ, NULL,
>                            create_flags, overlapped, NULL);
>      if (s->hfile == INVALID_HANDLE_VALUE) {
> @@ -223,7 +242,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>                0644);
>      if (fd < 0)
>          return -EIO;
> @@ -255,6 +274,7 @@ static BlockDriver bdrv_raw = {
>      .bdrv_getlength	= raw_getlength,
>  
>      .create_options = raw_create_options,
> +    .protocol_name  = "file",
>  };
>  
>  /***********************************************/
> @@ -349,7 +369,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>          overlapped |= FILE_FLAG_NO_BUFFERING | FILE_FLAG_WRITE_THROUGH;
>      else if (!(flags & BDRV_O_CACHE_WB))
>          overlapped |= FILE_FLAG_WRITE_THROUGH;
> -    s->hfile = CreateFile(filename, access_flags,
> +    s->hfile = qemu_CreateFile(filename, access_flags,
>                            FILE_SHARE_READ, NULL,
>                            create_flags, overlapped, NULL);
>      if (s->hfile == INVALID_HANDLE_VALUE) {
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 1e37b9f..36fd516 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -76,6 +76,99 @@ 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;
> +            default: *q++=*p++;
> +                      break;
> +            }
> +	    break;
> +        case ESCAPED:
> +            switch (*p) {
> +            case '\\' :
> +            case ',' :
> +            case ':': break;
> +            default: *q++='\\';
> +                     break;
> +            }
> +            state = NORMAL;
> +            *q++=*p++;
> +            break;
> +        }
> +   }
> +   *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 +975,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 +1148,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 */
> diff --git a/cutils.c b/cutils.c
> index bd9a019..9da3d1f 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 = qemu_strnlen(str, (len > 0 ? len-1 :  0));
> +    while (p < str+len) {
> +        if (*p == c)
> +            break;
> +        if (*p == '\\') {
> +            p++;
> +            if (*p == '\0')
> +                break;
> +        }
> +        *q++ = *p++;
> +    }
> +    *q='\0';
> +    return p;
> +}
> +
> +
>  void pstrcpy(char *buf, int buf_size, const char *str)
>  {
>      int c;
> @@ -192,3 +218,23 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count)
>          count -= copy;
>      }
>  }
> +
> +int qemu_open(const char *filename, int flags, ...)
> +{
> +    const char *f;
> +    int len = strlen(filename)+1;
> +    int fd, cflags;
> +    va_list ap;
> +    char *myfile = qemu_malloc(len);
> +
> +    va_start(ap, flags);
> +    cflags = va_arg(ap, int);

You shouldn't do this when there is no other parameter.

> +
> +    if (!strstart(filename, "file:", &f)) {
> +        prune_strcpy(myfile, len, filename, '\0');
> +        f = myfile;
> +    }
> +    fd =  open(f, flags, cflags);
> +    qemu_free(myfile);
> +    return fd;
> +}

Should this function be moved to the raw block driver?

> diff --git a/qemu-common.h b/qemu-common.h
> index 6a15f89..94db4a0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -104,11 +104,13 @@ 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);
>  int qemu_strnlen(const char *s, int max_len);
> +int qemu_open(const char *filename, int flags, ...);
>  time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  
> diff --git a/qemu-option.c b/qemu-option.c
> index 646bbad..75c2fc0 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -62,7 +62,7 @@ const char *get_opt_name(char *buf, int buf_size, const char *p, char delim)
>   *
>   * This function is comparable to get_opt_name with the difference that the
>   * delimiter is fixed to be comma which starts a new option. To specify an
> - * option value that contains commas, double each comma.
> + * option value that contains commas, double each comma or backslash the comma.
>   */
>  const char *get_opt_value(char *buf, int buf_size, const char *p)
>  {
> @@ -74,6 +74,12 @@ const char *get_opt_value(char *buf, int buf_size, const char *p)
>              if (*(p + 1) != ',')
>                  break;
>              p++;
> +        } else if (*p == '\\') {
> +            if (*(p + 1) == ',')
> +                 p++;
> +            if (q && (q - buf) < buf_size - 1)
> +                *q++ = *p;
> +            p++;

You can't express a backslash at the end of an option any more. We
should at least allow expressing a backslash as \\. Probably a backslash
should even be an escape character in all cases and never be used literally.

>          }
>          if (q && (q - buf) < buf_size - 1)
>              *q++ = *p;

If the string ends in \ or \, we're skipping the \0 here and run beyond
the end of the buffer.

Kevin