| Message ID | 1246511321.6429.31.camel@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <linuxram@us.ibm.com> > > 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 -- 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
Kevin Wolf wrote: > 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. > It ends up working out for Windows users because colons are invalid in Windows file names. What's the solution to this problem is we don't escape? >> 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. > For 0.12, maybe we should take a hard look at refactoring -drive and completely splitting this stuff. I think we ought to come up with a syntax where we can pass file names as independent arguments so that no special escaping is required. Regards, Anthony Liguori -- 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
Anthony Liguori schrieb: > Kevin Wolf wrote: >> 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. >> > > It ends up working out for Windows users because colons are invalid in > Windows file names. It could work as long as combinations of backslash + random character are interpreted this way instead of having a special meaning for everything after a backslash. > What's the solution to this problem is we don't escape? The majority of cases is covered by the file: protocol. I haven't thought of things like vvfat before though, so the combination of vvfat and a directory named :floppy: actually wouldn't work. Can we at least allow \, instead of ,, in parameter parsing, so that the backslash has the practical benefit of being a single universal escape character? >>> 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. > > For 0.12, maybe we should take a hard look at refactoring -drive and > completely splitting this stuff. I think we ought to come up with a > syntax where we can pass file names as independent arguments so that no > special escaping is required. What makes such a change more difficult is that it's not only -drive. The string is parsed in multiple locations. -drive only introduces the problem of commas, escaped by double comma. The next one is the generic block code which uses a colon to determine the protocol. And then vvfat comes and uses even more colons to find its arguments. Each of them are completely separate issues. Btw, I answered a related question in IRC today - I'll leave it uncommented: <DeadPanda> Is there any way to usb_add a fat:rw:/path filesystem? <kwolf> usb_add disk::fat:rw:/tmp/foo Kevin -- 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
Kevin Wolf wrote: > Can we at least allow \, instead of ,, in parameter parsing, so that the > backslash has the practical benefit of being a single universal escape > character? Is there a good reason why we cannot simply use \<char> to escape _any_ character, in every context where a user-supplied string/name/path/file is used? I'm thinking of consistency here. Instead of special cases for filenames, why not a standard scheme for all the places in command lines _and_ the monitor where a name/path/file is needed? There are many examples where it would be useful if unusual characters didn't break things, they simply worked. Examples: -vnc unix: path, -net port: device path, -net script path, -net sock= path, -net group= groupname, tap and bt device names. \<char> is an obvious scheme to standardise on given QEMU's unix shell heritage. It would work equally well for command line options (which are often comma-separated) and for monitor commands (which are often space-separated). It would have the nice property of being easy for management programs/scripts to quote, without them having a special list of characters to quote, without needing to update them if QEMU needs to quote more characters in future for some reason. Now, I see one significant hurdle with that: it's quite inconvenient for Windows users, typing paths like c:\path\to\dir\file, if those backslashes are stipped. So I propose this as a universal quoting scheme: \<char> where <char> is not ASCII alphanumeric. Shell quoting is easy: qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'` qemu -drive file="$qfile",if=scsi,media=disk Same quoting applied when sending the monitor a command to change a CD-ROM file or add a USB disk, for example. -- Jamie -- 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
Jamie Lokier wrote: > Kevin Wolf wrote: >> Can we at least allow \, instead of ,, in parameter parsing, so that the >> backslash has the practical benefit of being a single universal escape >> character? > > Is there a good reason why we cannot simply use \<char> to escape > _any_ character, in every context where a user-supplied > string/name/path/file is used? > > I'm thinking of consistency here. Instead of special cases for > filenames, why not a standard scheme for all the places in command > lines _and_ the monitor where a name/path/file is needed? Yeah, consistency. Very good point. > > There are many examples where it would be useful if unusual characters > didn't break things, they simply worked. > > Examples: -vnc unix: path, -net port: device path, -net script path, > -net sock= path, -net group= groupname, tap and bt device names. > > \<char> is an obvious scheme to standardise on given QEMU's unix shell > heritage. It would work equally well for command line options (which > are often comma-separated) and for monitor commands (which are often > space-separated). > > It would have the nice property of being easy for management > programs/scripts to quote, without them having a special list of > characters to quote, without needing to update them if QEMU needs to > quote more characters in future for some reason. > > Now, I see one significant hurdle with that: it's quite inconvenient > for Windows users, typing paths like c:\path\to\dir\file, if those > backslashes are stipped. We could exclude Windows from this (I think to remember that filenames are more restricted there anyway) or define a different, Windows-only escape character. > > So I propose this as a universal quoting scheme: > > \<char> where <char> is not ASCII alphanumeric. > > Shell quoting is easy: > > qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'` > > qemu -drive file="$qfile",if=scsi,media=disk > > Same quoting applied when sending the monitor a command to change a > CD-ROM file or add a USB disk, for example. > To me this direction looks more promising than any other proposal so far. Jan
Jan Kiszka wrote: > > Now, I see one significant hurdle with that: it's quite inconvenient > > for Windows users, typing paths like c:\path\to\dir\file, if those > > backslashes are stipped. > > We could exclude Windows from this (I think to remember that filenames > are more restricted there anyway) or define a different, Windows-only > escape character. I think both of those are bad ideas, because the same management scripts can run on Windows, and for consistency it's not just file names. Even Windows has block devices and network devices :-) Fortunately "where <char> is not ASCII alphanumeric" solves the practical cases where the user types an ordinary pathname. Or the user can type forward slashes just like they do in unix. > > So I propose this as a universal quoting scheme: > > > > \<char> where <char> is not ASCII alphanumeric. > > > > Shell quoting is easy: > > > > qfile=`printf %s "$file" | sed 's/[^0-9a-zA-Z]/\\\\&/g'` > > > > qemu -drive file="$qfile",if=scsi,media=disk I forgot a very obscure corner case, where the last character of the filename is a newline character. To do the right thing (with Bash at least), it should say '%s\n' instead of %s. Sue me :-) > > Same quoting applied when sending the monitor a command to change a > > CD-ROM file or add a USB disk, for example. > > To me this direction looks more promising than any other proposal so far. I wondered if it was just me... -- Jamie -- 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
Jamie Lokier wrote: > Jan Kiszka wrote: >>> Now, I see one significant hurdle with that: it's quite inconvenient >>> for Windows users, typing paths like c:\path\to\dir\file, if those >>> backslashes are stipped. >> We could exclude Windows from this (I think to remember that filenames >> are more restricted there anyway) or define a different, Windows-only >> escape character. > > I think both of those are bad ideas, because the same management > scripts can run on Windows, and for consistency it's not just file > names. Even Windows has block devices and network devices :-) I'm not sure if there is actually so much portability/reusability between Windows and the rest of the universe, but I'm surely not an expert in this. > > Fortunately "where <char> is not ASCII alphanumeric" solves the > practical cases where the user types an ordinary pathname. > > Or the user can type forward slashes just like they do in unix. We would still have to deal with the fact that so far '\' had no special meaning on Windows - except that is was the well-known path separator. So redefining its meaning would break a bit... Jan
Jan Kiszka wrote: > Jamie Lokier wrote: > > Jan Kiszka wrote: > >>> Now, I see one significant hurdle with that: it's quite inconvenient > >>> for Windows users, typing paths like c:\path\to\dir\file, if those > >>> backslashes are stipped. > >> We could exclude Windows from this (I think to remember that filenames > >> are more restricted there anyway) or define a different, Windows-only > >> escape character. > > > > I think both of those are bad ideas, because the same management > > scripts can run on Windows, and for consistency it's not just file > > names. Even Windows has block devices and network devices :-) > > I'm not sure if there is actually so much portability/reusability > between Windows and the rest of the universe, but I'm surely not an > expert in this. In my experience, shell scripts and Perl scripts tend to work either with no changes, or very small changes. > > Fortunately "where <char> is not ASCII alphanumeric" solves the > > practical cases where the user types an ordinary pathname. > > > > Or the user can type forward slashes just like they do in unix. > > We would still have to deal with the fact that so far '\' had no special > meaning on Windows - except that is was the well-known path separator. > So redefining its meaning would break a bit... The point is that paths tend to have alphanumeric characters at the start of each component, so it doesn't matter in most cases that it's redefined. People won't notice because c:\path\to\file will continue to work, whether it's by itself or part of a multi-option option. Exceptions are \\host\path and \\.\device, where the error will be so obvious they'll learn quickly. We could find a more complex scheme where \\ is unaffected, but complex is not good and will be wrongly implemented by other programs. Whereas \<char> is very common, well known and easy to get right, even when people guess how it's done, like they do when working out how to quote paths for rsync and ssh. Oh, I'm suddenly thinking that "." should be included in "alphanumeric" :-) -- Jamie -- 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
Jan Kiszka wrote: > We would still have to deal with the fact that so far '\' had no special > meaning on Windows - except that is was the well-known path separator. > So redefining its meaning would break a bit... > That's the problem. You will break existing Windows users. I know this goes against the current momentum in qemu, but overloading one option with a bunch of parameters seems absolutely silly to me. IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at least three parameters.
Anthony Liguori wrote: > Jan Kiszka wrote: > >We would still have to deal with the fact that so far '\' had no special > >meaning on Windows - except that is was the well-known path separator. > >So redefining its meaning would break a bit... > > > > That's the problem. You will break existing Windows users. > > I know this goes against the current momentum in qemu, but overloading > one option with a bunch of parameters seems absolutely silly to me. > > IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at > least three parameters. That's fine for command lines. I don't necessarily disagree with you. But how do you propose to handle paths in monitor commands, when the path contains a space/quote/whatever as it often does on Windows ("My Documents", "Program Files")? -- Jamie -- 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
Jamie Lokier wrote: > Anthony Liguori wrote: > >> Jan Kiszka wrote: >> >>> We would still have to deal with the fact that so far '\' had no special >>> meaning on Windows - except that is was the well-known path separator. >>> So redefining its meaning would break a bit... >>> >>> >> That's the problem. You will break existing Windows users. >> >> I know this goes against the current momentum in qemu, but overloading >> one option with a bunch of parameters seems absolutely silly to me. >> >> IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at >> least three parameters. >> > > That's fine for command lines. I don't necessarily disagree with you. > > But how do you propose to handle paths in monitor commands, when the > path contains a space/quote/whatever as it often does on Windows ("My > Documents", "Program Files")? > Same basic rules apply. The monitor should use shell-style quoting.
Anthony Liguori wrote: > Jamie Lokier wrote: > >Anthony Liguori wrote: > > > >>Jan Kiszka wrote: > >> > >>>We would still have to deal with the fact that so far '\' had no special > >>>meaning on Windows - except that is was the well-known path separator. > >>>So redefining its meaning would break a bit... > >>> > >>> > >>That's the problem. You will break existing Windows users. > >> > >>I know this goes against the current momentum in qemu, but overloading > >>one option with a bunch of parameters seems absolutely silly to me. > >> > >>IMHO, -drive file=foo.img,if=virtio,cache=off should have always been at > >>least three parameters. > >> > > > >That's fine for command lines. I don't necessarily disagree with you. > > > >But how do you propose to handle paths in monitor commands, when the > >path contains a space/quote/whatever as it often does on Windows ("My > >Documents", "Program Files")? > > > > Same basic rules apply. The monitor should use shell-style quoting. So instead of consistency, you like the idea of using different quoting rules for the monitor than for command line arguments? -- Jamie -- 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
Jamie Lokier wrote: > So instead of consistency, you like the idea of using different > quoting rules for the monitor than for command line arguments? > Your proposal breaks Windows in a catastrophic way. It's almost certain that all existing front-ends/scripts will stop working after such a change. Or you have to quote differently on Windows which means you throw consistency out the Window. I certainly like consistency but I don't see a viable proposal that offers that. Regards, Anthony Liguori > -- Jamie > -- > 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 > -- 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
Anthony Liguori wrote: > Jan Kiszka wrote: >> We would still have to deal with the fact that so far '\' had no special >> meaning on Windows - except that is was the well-known path separator. >> So redefining its meaning would break a bit... >> > > That's the problem. You will break existing Windows users. > > I know this goes against the current momentum in qemu, but overloading > one option with a bunch of parameters seems absolutely silly to me. It's surely not perfect in every detail. On the other hand, it's fairly compact, concentrating device attributes in one place. Jan
Anthony Liguori wrote: > Jamie Lokier wrote: >> So instead of consistency, you like the idea of using different >> quoting rules for the monitor than for command line arguments? >> > > Your proposal breaks Windows in a catastrophic way. It's almost certain > that all existing front-ends/scripts will stop working after such a change. Breakage of existing users is surely a no-go, so '\'-escaping remains taboo for Windows. > > Or you have to quote differently on Windows which means you throw > consistency out the Window. I'm still not convinced that we actually need that much consistency here. This is *mostly* about path names, and path names are not directly portable between Windows and the rest anyway. Jan
Jamie Lokier schrieb: > Kevin Wolf wrote: >> Can we at least allow \, instead of ,, in parameter parsing, so that the >> backslash has the practical benefit of being a single universal escape >> character? > > Is there a good reason why we cannot simply use \<char> to escape > _any_ character, in every context where a user-supplied > string/name/path/file is used? > > I'm thinking of consistency here. Instead of special cases for > filenames, why not a standard scheme for all the places in command > lines _and_ the monitor where a name/path/file is needed? I absolutely agree with your intention here (maybe except Windows, haven't thought about that a lot). But from an implementation POV, this would need a major rework of the parsing code. The problem is that to do this universally you need to have one central place where everything is parsed. We currently don't have that. We have the command line parser that needs comma and equals for its parsing. We have the block code that needs the colon for protocols. We have block drivers that again separate options by colons. And so on. So currently we can't handle backslashes when parsing command line options. They would be missing in the block code for escaping colons. We can't handle all colons in the generic block code, the stripped backslashes would be missing in vvfat and nbd. Once we have decided what the solution should look like (including Windows and other problems), it might be worth the effort. But I can promise that it's going to be much more than just one patch. Kevin -- 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
> So I propose this as a universal quoting scheme: > > \<char> where <char> is not ASCII alphanumeric. No thank you. This sounds dangerously like the windows command shell quoting rules. At first clance they appear to "just work", however when you actually try to figure out what's going on it gets horribly messy. For example UNC paths (which start with a double backslash) come out really weird with your suggestion. Paul -- 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
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, ':'); + 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; 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; + 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 +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; +} + + 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); +} 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))
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. 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 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 <linuxram@us.ibm.com> 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(-) -- 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