[PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Gianni Tedesco]

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++;
         }

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Ian Jackson]

Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:"):
> 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.

I'm not convinced that it is clearer.  It's certainly a lot longer:
132 lines added for 50 removed.

Perhaps we should just import a regexp parser and use that ?  Really,
we should have a regexp or some other kind of declarative statement of
the syntax.

Possible regexp parsers include pcre and flex.  flex has the advantage
that we're using it already so it's just another line in the
Makefile.

Ian.

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Gianni Tedesco]

On Thu, 2010-08-19 at 15:54 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("[Xen-devel] [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:"):
> > 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.
> 
> I'm not convinced that it is clearer.  It's certainly a lot longer:
> 132 lines added for 50 removed.

It's true that it's longer but the nature of these types of parsers it's
a lot of very short lines :)

I think it's clearer than a correct strtok() + handling all errors and
variations would be.

> Perhaps we should just import a regexp parser and use that ?  Really,
> we should have a regexp or some other kind of declarative statement of
> the syntax.
> 
> Possible regexp parsers include pcre and flex.  flex has the advantage
> that we're using it already so it's just another line in the
> Makefile.

It's your call, I know nothing of flex and its mysterious ways and my
pcre skills are limited to basic text-editor-fu... I agree that flex
probably makes the most sense.

On the other hand whoever designed these formats seemed to want to make
them difficult to parse. Since it's all python I find myself wondering
why they didn't use a dictionary or a tuple.

Gianni

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Ian Jackson]

Gianni Tedesco (3P) writes ("Re: [Xen-devel] [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:"):
> It's true that it's longer but the nature of these types of parsers it's
> a lot of very short lines :)

It adds 4164 characters and removes 1783.  Discounting leading
whitespace it adds 2550 characters and removes 1085.  However you
count it it's between 2x and 3x as long :-).

I always think state machine based parsers are very hard to read by
eye.  That's why we have parser generators.

> I think it's clearer than a correct strtok() + handling all errors and
> variations would be.

Perhaps so.

> It's your call, I know nothing of flex and its mysterious ways and my
> pcre skills are limited to basic text-editor-fu... I agree that flex
> probably makes the most sense.

I'll see if I can come up with a flex or pcre syntax that works and we
can see what people think of it.

> On the other hand whoever designed these formats seemed to want to make
> them difficult to parse. Since it's all python I find myself wondering
> why they didn't use a dictionary or a tuple.

Just don't go there :-).

Ian.

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Gianni Tedesco]

On Thu, 2010-08-19 at 16:53 +0100, Ian Jackson wrote:
> Gianni Tedesco (3P) writes ("Re: [Xen-devel] [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:"):
> > It's true that it's longer but the nature of these types of parsers it's
> > a lot of very short lines :)
> 
> It adds 4164 characters and removes 1783.  Discounting leading
> whitespace it adds 2550 characters and removes 1085.  However you
> count it it's between 2x and 3x as long :-).
> 
> I always think state machine based parsers are very hard to read by
> eye.  That's why we have parser generators.
> 
> > I think it's clearer than a correct strtok() + handling all errors and
> > variations would be.
> 
> Perhaps so.
> 
> > It's your call, I know nothing of flex and its mysterious ways and my
> > pcre skills are limited to basic text-editor-fu... I agree that flex
> > probably makes the most sense.
> 
> I'll see if I can come up with a flex or pcre syntax that works and we
> can see what people think of it.

Fair enough. While we're on the subject why is there even a separate
libxlutil.so? It's not like the functionality is de-coupled and it seems
to me like this parser stuff should be compiled right in to libxenlight.

> > On the other hand whoever designed these formats seemed to want to make
> > them difficult to parse. Since it's all python I find myself wondering
> > why they didn't use a dictionary or a tuple.
> 
> Just don't go there :-).

OK, I suppose I'd rather not then :)

[PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Gianni Tedesco]

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++;
         }

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Ian Jackson]

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.

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Gianni Tedesco]

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

Re: [PATCH]: xl: don't segfault parsing disk configs, support NULL physpath and ioemu:

[Ian Jackson]

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.