Xen devel - Dec 2010 - [PATCH]: xl: don''t segfault parsing disk configs, support NULL physpath and ioemu:

Resending due to another user reported running in to this. Applies with
offsets.

----------------8<--------------8<----------------

Switch to a state machine parser since it''s easier to handle all these
exotic cases without segfaulting. NULL physpaths are now allowed and a
dodgy hack is introduced to skip over the "ioemu:" prefix for a
virtpath. Also fixes a leak of buf2.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff -r 6bd04080ab99 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Aug 18 18:06:10 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Aug 19 15:34:20 2010 +0100
@@ -539,6 +539,134 @@ static int parse_action_on_shutdown(cons
     return 0;
 }
 
+#define DSTATE_INITIAL   0
+#define DSTATE_TAP       1
+#define DSTATE_PHYSPATH  2
+#define DSTATE_VIRTPATH  3
+#define DSTATE_VIRTTYPE  4
+#define DSTATE_RW        5
+#define DSTATE_TERMINAL  6
+
+static int parse_disk_config(libxl_device_disk *disk, char *buf2)
+{
+    int state = DSTATE_INITIAL;
+    char *p, *end, *tok;
+
+    memset(disk, 0, sizeof(*disk));
+
+    for(tok = p = buf2, end = buf2 + strlen(buf2) + 1; p < end; p++) {
+        switch(state){
+        case DSTATE_INITIAL:
+            if ( *p == '':'' ) {
+                *p = ''\0'';
+                if ( !strcmp(tok, "phy") ) {
+                    state = DSTATE_PHYSPATH;
+                    disk->phystype = PHYSTYPE_PHY;
+                }else if ( !strcmp(tok, "file") ) {
+                    state = DSTATE_PHYSPATH;
+                    disk->phystype = PHYSTYPE_FILE;
+                }else if ( !strcmp(tok, "tap") ) {
+                    state = DSTATE_TAP;
+                }else{
+                    fprintf(stderr, "Unknown disk type: %s\n", tok);
+                    return 0;
+                }
+                tok = p + 1;
+            }
+            break;
+        case DSTATE_TAP:
+            if ( *p == '':'' ) {
+                *p = ''\0'';
+                if ( !strcmp(tok, "aio") ) {
+                    disk->phystype = PHYSTYPE_AIO;
+                }else if ( !strcmp(tok, "vhd") ) {
+                    disk->phystype = PHYSTYPE_VHD;
+                }else if ( !strcmp(tok, "qcow") ) {
+                    disk->phystype = PHYSTYPE_QCOW;
+                }else if ( !strcmp(tok, "qcow2") ) {
+                    disk->phystype = PHYSTYPE_QCOW2;
+                }else {
+                    fprintf(stderr, "Unknown tapdisk type: %s\n",
tok);
+                    return 0;
+                }
+
+                tok = p + 1;
+                state = DSTATE_PHYSPATH;
+            }
+            break;
+        case DSTATE_PHYSPATH:
+            if ( *p == '','' ) {
+                int ioemu_len;
+
+                *p = ''\0'';
+                disk->physpath = (*tok) ? strdup(tok) : NULL;
+                tok = p + 1;
+
+                /* hack for ioemu disk spec */
+                ioemu_len = strlen("ioemu:");
+                state = DSTATE_VIRTPATH;
+                if ( tok + ioemu_len < end &&
+                    !strncmp(tok, "ioemu:", ioemu_len)) {
+                    tok += ioemu_len;
+                    p += ioemu_len;
+                }
+            }
+            break;
+        case DSTATE_VIRTPATH:
+            if ( *p == '','' || *p == '':'' ||
*p == ''\0'' ) {
+                switch(*p) {
+                case '':'':
+                    state = DSTATE_VIRTTYPE;
+                    break;
+                case '','':
+                    state = DSTATE_RW;
+                    break;
+                case ''\0'':
+                    state = DSTATE_TERMINAL;
+                    break;
+                }
+                if ( tok == p )
+                    goto out;
+                *p = ''\0'';
+                disk->virtpath = (*tok) ? strdup(tok) : NULL;
+                tok = p + 1;
+            }
+            break;
+        case DSTATE_VIRTTYPE:
+            if ( *p == '','' || *p == ''\0'' )
{
+                *p = ''\0'';
+                if ( !strcmp(tok, "cdrom") ) {
+                    disk->is_cdrom = 1;
+                    disk->unpluggable = 1;
+                }else{
+                    fprintf(stderr, "Unknown virtual disk type:
%s\n", tok);
+                    return 0;
+                }
+                tok = p + 1;
+                state = (*p == '','') ? DSTATE_RW :
DSTATE_TERMINAL;
+            }
+            break;
+        case DSTATE_RW:
+            if ( *p == ''\0'' ) {
+                disk->readwrite = (tok[0] == ''w'');
+                tok = p + 1;
+                state = DSTATE_TERMINAL;
+            }
+            break;
+        case DSTATE_TERMINAL:
+            goto out;
+        }
+    }
+
+out:
+    if ( tok != p || state != DSTATE_TERMINAL ) {
+        fprintf(stderr, "parse error in disk config near
''%s''\n", tok);
+        return 0;
+    }
+
+    return 1;
+}
+
 static void parse_config_data(const char *configfile_filename_report,
                               const char *configfile_data,
                               int configfile_len,
@@ -716,59 +844,13 @@ static void parse_config_data(const char
         while ((buf = xlu_cfg_get_listitem (vbds, d_config->num_disks)) !=
NULL) {
             libxl_device_disk *disk;
             char *buf2 = strdup(buf);
-            char *p, *p2;
 
             d_config->disks = (libxl_device_disk *)
realloc(d_config->disks, sizeof (libxl_device_disk) * (d_config->num_disks
+ 1));
             disk = d_config->disks + d_config->num_disks;
-
-            disk->backend_domid = 0;
-            disk->domid = 0;
-            disk->unpluggable = 0;
-
-            p = strtok(buf2, ",:");
-            while (*p == '' '')
-                p++;
-            if (!strcmp(p, "phy")) {
-                disk->phystype = PHYSTYPE_PHY;
-            } else if (!strcmp(p, "file")) {
-                disk->phystype = PHYSTYPE_FILE;
-            } else if (!strcmp(p, "tap")) {
-                p = strtok(NULL, ":");
-                if (!strcmp(p, "aio")) {
-                    disk->phystype = PHYSTYPE_AIO;
-                } else if (!strcmp(p, "vhd")) {
-                    disk->phystype = PHYSTYPE_VHD;
-                } else if (!strcmp(p, "qcow")) {
-                    disk->phystype = PHYSTYPE_QCOW;
-                } else if (!strcmp(p, "qcow2")) {
-                    disk->phystype = PHYSTYPE_QCOW2;
-                }
+            if ( !parse_disk_config(disk, buf2) ) {
+                exit(1);
             }
-            p = strtok(NULL, ",");
-            while (*p == '' '')
-                p++;
-            disk->physpath= strdup(p);
-            p = strtok(NULL, ",");
-            while (*p == '' '')
-                p++;
-            p2 = strchr(p, '':'');
-            if (p2 == NULL) {
-                disk->virtpath = strdup(p);
-                disk->is_cdrom = 0;
-                disk->unpluggable = 1;
-            } else {
-                *p2 = ''\0'';
-                disk->virtpath = strdup(p);
-                if (!strcmp(p2 + 1, "cdrom")) {
-                    disk->is_cdrom = 1;
-                    disk->unpluggable = 1;
-                } else
-                    disk->is_cdrom = 0;
-            }
-            p = strtok(NULL, ",");
-            while (*p == '' '')
-                p++;
-            disk->readwrite = (p[0] == ''w'') ? 1 : 0;
+
             free(buf2);
             d_config->num_disks++;
         }






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don''t segfault
parsing disk configs, support NULL physpath and
ioemu:"):
> Resending due to another user reported running in to this. Applies with
> offsets.
...
> Switch to a state machine parser since it''s easier to handle all
these
> exotic cases without segfaulting. NULL physpaths are now allowed and a
> dodgy hack is introduced to skip over the "ioemu:" prefix for a
> virtpath. Also fixes a leak of buf2.
Thanks, but I think the last time we had this we had some negative
feedback about the comprehensibility of this approach.

Perhaps a regexp- or flex- or bison- based approach would be better ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, 2010-12-14 at 17:40 +0000, Ian Jackson wrote:
> Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don''t
segfault parsing disk configs, support NULL physpath and ioemu:"):
> > Resending due to another user reported running in to this. Applies
with
> > offsets.
> ...
> > Switch to a state machine parser since it''s easier to handle
all these
> > exotic cases without segfaulting. NULL physpaths are now allowed and a
> > dodgy hack is introduced to skip over the "ioemu:" prefix
for a
> > virtpath. Also fixes a leak of buf2.
> 
> Thanks, but I think the last time we had this we had some negative
> feedback about the comprehensibility of this approach.
Yes... from you :)
> Perhaps a regexp- or flex- or bison- based approach would be better ?
ISTR you said you''d do it in flex but since:

1. people are running to the bugs "out in the world"
2. we have an existing fix
3. I have no flex-fu
4. and we were in agreement that this code is as readable or more-so
   than a correct strtok-based implementation

then why not take the fix we have now and improve if/when we need to in
the future? Evidently nobody has touched this code for 6 months or more.
> Ian.
Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don''t segfault
parsing disk configs, support NULL physpath and
ioemu:"):
> Resending due to another user reported running in to this. Applies with
> offsets.
I''ve applied this, thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel