diff mbox

rev4: support colon in filenames

Message ID 1247041831.6297.12.camel@localhost (mailing list archive)
State New, archived
Headers show

Commit Message

Ram Pai July 8, 2009, 8:30 a.m. UTC
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

the patch applies to qemu-kvm tree only. Will port the patch to qemu tree
after successful test results.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>


 block.c           |   30 +++--------------
 block/raw-posix.c |   35 +++++++++++++++----
 block/raw-win32.c |   26 ++++++++++++--
 block/vvfat.c     |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 cutils.c          |   26 ++++++++++++++
 qemu-common.h     |    1 +
 qemu-option.c     |    8 ++++-
 7 files changed, 185 insertions(+), 38 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kiszka July 8, 2009, 3:05 p.m. UTC | #1
Ram Pai wrote:
> 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
> 
> the patch applies to qemu-kvm tree only. Will port the patch to qemu tree
> after successful test results.

Please focus on upstream as upstream was broken first and the fix will
flow downstream anyway. And note that this patch does not apply on top
of qemu-kvm. So a rebase against qemu head would be very welcome.

Jan
Anthony Liguori July 10, 2009, 1:31 p.m. UTC | #2
Ram Pai wrote:
> 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:
>   

This needs rebasing and I'd like to see a few folks ack it first too.
diff mbox

Patch

diff --git a/block.c b/block.c
index aca5a6d..be10bf9 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))
@@ -330,8 +324,6 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
                BlockDriver *drv)
 {
     int ret, open_flags;
-    char tmp_filename[PATH_MAX];
-    char backing_filename[PATH_MAX];
 
     bs->read_only = 0;
     bs->is_temporary = 0;
@@ -343,9 +335,9 @@  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;
+        char tmp_filename[PATH_MAX];
 
         /* if snapshot, we create a temporary backing file and open it
            instead of opening 'filename' directly */
@@ -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);
-
         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);
@@ -440,11 +422,9 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         /* if there is a backing file, use it */
         BlockDriver *back_drv = NULL;
         bs->backing_hd = bdrv_new("");
-        path_combine(backing_filename, sizeof(backing_filename),
-                     filename, bs->backing_file);
         if (bs->backing_format[0] != '\0')
             back_drv = bdrv_find_format(bs->backing_format);
-        ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
+        ret = bdrv_open2(bs->backing_hd, bs->backing_file, open_flags,
                          back_drv);
         if (ret < 0) {
             bdrv_close(bs);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..fa2559c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -124,6 +124,26 @@  static int fd_open(BlockDriverState *bs);
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+static 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);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, len, filename, '\0');
+        f = myfile;
+    }
+    fd =  open(f, flags, cflags);
+    qemu_free(myfile);
+    return fd;
+}
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
         int flags)
 {
@@ -151,7 +171,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)
@@ -844,7 +864,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;
@@ -889,6 +909,7 @@  static BlockDriver bdrv_raw = {
     .bdrv_getlength = raw_getlength,
 
     .create_options = raw_create_options,
+    .protocol_name = "file",
 };
 
 /***********************************************/
@@ -985,7 +1006,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 {
@@ -1037,7 +1058,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;
@@ -1133,7 +1154,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;
 
@@ -1239,7 +1260,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");
@@ -1399,7 +1420,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..b4ec4a5 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;
+    int fd;
+    char *myfile = qemu_malloc(len);
+
+    if (!strstart(filename, "file:", &f)) {
+        prune_strcpy(myfile, len, filename, '\0');
+        f = myfile;
+    }
+    fd = CreateFile(filename, access_flags, share_flag, 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..6a0410e 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.
+ */
+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'
+ */
+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 6ea0c49..09f5d39 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 > 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;
diff --git a/qemu-common.h b/qemu-common.h
index 2dcb224..230c41e 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -104,6 +104,7 @@  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);
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++;
         }
         if (q && (q - buf) < buf_size - 1)
             *q++ = *p;