[PATCH 0/6] libxl: config file string handling cleanups

[PATCH 0/6] libxl: config file string handling cleanups

[Ian Jackson]

These 6 followup patches were developed during a review of the xl
string handling code, prompted by XSA-137.  They were embargoed until
today.

I have reviewed the whole of xl's string handling for other bugs.
My search terms included:
     realloc sn?printf str \bstr \bstrcpy \bstrn \bstrcat \bmemcpy
     \bmemchr \bp\b

Surprisingly, I found not too much untoward.

Of these only two are even backport candidates:

  xl: Do not ignore unparseable PCI BDFs
  xl: Rewrite trim()

[PATCH 1/6] xl: Do not ignore unparseable PCI BDFs

[Ian Jackson]

If xlu_pci_parse_bdf fails, abandon the domain creation, rather than
blundering on.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 08484e4..31d8260 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1942,8 +1942,12 @@ skip_vfb:
             pcidev->power_mgmt = pci_power_mgmt;
             pcidev->permissive = pci_permissive;
             pcidev->seize = pci_seize;
-            if (!xlu_pci_parse_bdf(config, pcidev, buf))
-                d_config->num_pcidevs++;
+            e = xlu_pci_parse_bdf(config, pcidev, buf);
+            if (e) {
+                fprintf(stderr, "unable to parse PCI BDF for passthrough\n");
+                exit(-e);
+            }
+            d_config->num_pcidevs++;
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             libxl_defbool_set(&b_info->u.pv.e820_host, true);
-- 
1.7.10.4

Re: [PATCH 1/6] xl: Do not ignore unparseable PCI BDFs

[Andrew Cooper]

On 07/07/15 17:13, Ian Jackson wrote:
> If xlu_pci_parse_bdf fails, abandon the domain creation, rather than
> blundering on.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 08484e4..31d8260 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1942,8 +1942,12 @@ skip_vfb:
>              pcidev->power_mgmt = pci_power_mgmt;
>              pcidev->permissive = pci_permissive;
>              pcidev->seize = pci_seize;
> -            if (!xlu_pci_parse_bdf(config, pcidev, buf))
> -                d_config->num_pcidevs++;
> +            e = xlu_pci_parse_bdf(config, pcidev, buf);
> +            if (e) {
> +                fprintf(stderr, "unable to parse PCI BDF for passthrough\n");

'%s' including the offending buf ?

~Andrew

> +                exit(-e);
> +            }
> +            d_config->num_pcidevs++;
>          }
>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>              libxl_defbool_set(&b_info->u.pv.e820_host, true);

[PATCH 1/6 v2] xl: Do not ignore unparseable PCI BDFs

[Ian Jackson]

[ resending just 1/6 rather than the others as well ]

If xlu_pci_parse_bdf fails, abandon the domain creation, rather than
blundering on.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Print the offending supposed-BDF too.
---
 tools/libxl/xl_cmdimpl.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index a08c264..5ab4e16 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1945,8 +1945,14 @@ skip_vfb:
             pcidev->power_mgmt = pci_power_mgmt;
             pcidev->permissive = pci_permissive;
             pcidev->seize = pci_seize;
-            if (!xlu_pci_parse_bdf(config, pcidev, buf))
-                d_config->num_pcidevs++;
+            e = xlu_pci_parse_bdf(config, pcidev, buf);
+            if (e) {
+                fprintf(stderr,
+                        "unable to parse PCI BDF `%s' for passthrough\n",
+                        buf);
+                exit(-e);
+            }
+            d_config->num_pcidevs++;
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             libxl_defbool_set(&b_info->u.pv.e820_host, true);
-- 
1.7.10.4

Re: [PATCH 1/6 v2] xl: Do not ignore unparseable PCI BDFs

[Wei Liu]

On Thu, Jul 16, 2015 at 04:47:55PM +0100, Ian Jackson wrote:
> [ resending just 1/6 rather than the others as well ]
> 
> If xlu_pci_parse_bdf fails, abandon the domain creation, rather than
> blundering on.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> v2: Print the offending supposed-BDF too.
> ---
>  tools/libxl/xl_cmdimpl.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index a08c264..5ab4e16 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1945,8 +1945,14 @@ skip_vfb:
>              pcidev->power_mgmt = pci_power_mgmt;
>              pcidev->permissive = pci_permissive;
>              pcidev->seize = pci_seize;
> -            if (!xlu_pci_parse_bdf(config, pcidev, buf))
> -                d_config->num_pcidevs++;
> +            e = xlu_pci_parse_bdf(config, pcidev, buf);
> +            if (e) {
> +                fprintf(stderr,
> +                        "unable to parse PCI BDF `%s' for passthrough\n",
> +                        buf);
> +                exit(-e);
> +            }
> +            d_config->num_pcidevs++;
>          }
>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>              libxl_defbool_set(&b_info->u.pv.e820_host, true);
> -- 
> 1.7.10.4

[PATCH 2/6] xl: Use ARRAY_EXTEND_INIT for vtpms and nics

[Ian Jackson]

This removes two open-coded reallocs.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 31d8260..6ea4e6b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1701,11 +1701,9 @@ static void parse_config_data(const char *config_source,
             char *p, *p2;
             bool got_backend = false;
 
-            d_config->vtpms = (libxl_device_vtpm *) realloc(d_config->vtpms,
-                  sizeof(libxl_device_vtpm) * (d_config->num_vtpms+1));
-            vtpm = d_config->vtpms + d_config->num_vtpms;
-            libxl_device_vtpm_init(vtpm);
-            vtpm->devid = d_config->num_vtpms;
+            vtpm = ARRAY_EXTEND_INIT(d_config->vtpms,
+                                     d_config->num_vtpms,
+                                     libxl_device_vtpm_init);
 
             p = strtok(buf2, ",");
             if(p) {
@@ -1735,7 +1733,6 @@ static void parse_config_data(const char *config_source,
                exit(1);
             }
             free(buf2);
-            d_config->num_vtpms++;
         }
     }
 
@@ -1825,10 +1822,9 @@ static void parse_config_data(const char *config_source,
             char *buf2 = strdup(buf);
             char *p;
 
-            d_config->nics = (libxl_device_nic *) realloc(d_config->nics, sizeof (libxl_device_nic) * (d_config->num_nics+1));
-            nic = d_config->nics + d_config->num_nics;
-            libxl_device_nic_init(nic);
-            nic->devid = d_config->num_nics;
+            nic = ARRAY_EXTEND_INIT(d_config->nics,
+                                    d_config->num_nics,
+                                    libxl_device_nic_init);
             set_default_nic_values(nic);
 
             p = strtok(buf2, ",");
@@ -1841,7 +1837,6 @@ static void parse_config_data(const char *config_source,
             } while ((p = strtok(NULL, ",")) != NULL);
 skip_nic:
             free(buf2);
-            d_config->num_nics++;
         }
     }
 
-- 
1.7.10.4

[PATCH 3/6] xl: Provide and use ARRAY_EXTEND_INIT_NODEVID for disks, pcidevs and dtdevs

[Ian Jackson]

This replaces 3 sets of open-coded reallocs etc.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6ea4e6b..568d7d2 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -323,16 +323,24 @@ static char *xstrdup(const char *x)
     return r;
 }
 
-#define ARRAY_EXTEND_INIT(array,count,initfn)                           \
+#define ARRAY_EXTEND_INIT__CORE(array,count,initfn,more)                \
     ({                                                                  \
         typeof((count)) array_extend_old_count = (count);               \
         (count)++;                                                      \
         (array) = xrealloc((array), sizeof(*array) * (count));          \
         (initfn)(&(array)[array_extend_old_count]);                     \
-        (array)[array_extend_old_count].devid = array_extend_old_count; \
+        more;                                                           \
         &(array)[array_extend_old_count];                               \
     })
 
+#define ARRAY_EXTEND_INIT(array,count,initfn)                           \
+    ARRAY_EXTEND_INIT__CORE((array),(count),(initfn), ({                \
+        (array)[array_extend_old_count].devid = array_extend_old_count; \
+        }))
+
+#define ARRAY_EXTEND_INIT_NODEVID(array,count,initfn) \
+    ARRAY_EXTEND_INIT__CORE((array),(count),(initfn), /* nothing */ )
+
 #define LOG(_f, _a...)   dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
 
 static void dolog(const char *file, int line, const char *func, char *fmt, ...)
@@ -1683,12 +1691,12 @@ static void parse_config_data(const char *config_source,
             libxl_device_disk *disk;
             char *buf2 = strdup(buf);
 
-            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 = ARRAY_EXTEND_INIT_NODEVID(d_config->disks,
+                                             d_config->num_disks,
+                                             libxl_device_disk_init);
             parse_disk_config(&config, buf2, disk);
 
             free(buf2);
-            d_config->num_disks++;
         }
     }
 
@@ -1929,10 +1937,9 @@ skip_vfb:
         for(i = 0; (buf = xlu_cfg_get_listitem (pcis, i)) != NULL; i++) {
             libxl_device_pci *pcidev;
 
-            d_config->pcidevs = (libxl_device_pci *) realloc(d_config->pcidevs, sizeof (libxl_device_pci) * (d_config->num_pcidevs + 1));
-            pcidev = d_config->pcidevs + d_config->num_pcidevs;
-            libxl_device_pci_init(pcidev);
-
+            pcidev = ARRAY_EXTEND_INIT_NODEVID(d_config->pcidevs,
+                                               d_config->num_pcidevs,
+                                               libxl_device_pci_init);
             pcidev->msitranslate = pci_msitranslate;
             pcidev->power_mgmt = pci_power_mgmt;
             pcidev->permissive = pci_permissive;
@@ -1942,7 +1949,6 @@ skip_vfb:
                 fprintf(stderr, "unable to parse PCI BDF for passthrough\n");
                 exit(-e);
             }
-            d_config->num_pcidevs++;
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             libxl_defbool_set(&b_info->u.pv.e820_host, true);
@@ -1954,17 +1960,15 @@ skip_vfb:
         for (i = 0; (buf = xlu_cfg_get_listitem(dtdevs, i)) != NULL; i++) {
             libxl_device_dtdev *dtdev;
 
-            d_config->dtdevs = xrealloc(d_config->dtdevs,
-                                        sizeof (libxl_device_dtdev) * (i + 1));
-            dtdev = d_config->dtdevs + d_config->num_dtdevs;
-            libxl_device_dtdev_init(dtdev);
+            dtdev = ARRAY_EXTEND_INIT_NODEVID(d_config->dtdevs,
+                                              d_config->num_dtdevs,
+                                              libxl_device_dtdev_init);
 
             dtdev->path = strdup(buf);
             if (dtdev->path == NULL) {
                 fprintf(stderr, "unable to duplicate string for dtdevs\n");
                 exit(-1);
             }
-            d_config->num_dtdevs++;
         }
     }
 
-- 
1.7.10.4

[PATCH 4/6] xl: Provide and use xvasprintf and xasprintf internally

[Ian Jackson]

Replace all calls to [v]asprintf with this new function.  This removes
a fair amount of bespoke error handling.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   96 +++++++++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 568d7d2..c876d3e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -362,6 +362,27 @@ static void dolog(const char *file, int line, const char *func, char *fmt, ...)
     free(s);
 }
 
+static void xvasprintf(char **strp, const char *fmt, va_list ap)
+    __attribute__((format(printf,2,0)));
+static void xvasprintf(char **strp, const char *fmt, va_list ap)
+{
+    int r = vasprintf(strp, fmt, ap);
+    if (r == -1) {
+        perror("asprintf failed");
+        exit(-ERROR_FAIL);
+    }
+}
+
+static void xasprintf(char **strp, const char *fmt, ...)
+    __attribute__((format(printf,2,3)));
+static void xasprintf(char **strp, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    xvasprintf(strp, fmt, ap);
+    va_end(ap);
+}
+
 static yajl_gen_status printf_info_one_json(yajl_gen hand, int domid,
                                             libxl_domain_config *d_config)
 {
@@ -840,11 +861,9 @@ static char *parse_cmdline(XLU_Config *config)
                     "in favour of cmdline=\n");
     } else {
         if (root && extra) {
-            if (asprintf(&cmdline, "root=%s %s", root, extra) == -1)
-                cmdline = NULL;
+            xasprintf(&cmdline, "root=%s %s", root, extra);
         } else if (root) {
-            if (asprintf(&cmdline, "root=%s", root) == -1)
-                cmdline = NULL;
+            xasprintf(&cmdline, "root=%s", root);
         } else if (extra) {
             cmdline = strdup(extra);
         }
@@ -2335,14 +2354,11 @@ static int handle_domain_death(uint32_t *r_domid,
         char *corefile;
         int rc;
 
-        if (asprintf(&corefile, XEN_DUMP_DIR "/%s", d_config->c_info.name) < 0) {
-            LOG("failed to construct core dump path");
-        } else {
-            LOG("dumping core to %s", corefile);
-            rc=libxl_domain_core_dump(ctx, *r_domid, corefile, NULL);
-            if (rc) LOG("core dump failed (rc=%d).", rc);
-            free(corefile);
-        }
+        xasprintf(&corefile, XEN_DUMP_DIR "/%s", d_config->c_info.name);
+        LOG("dumping core to %s", corefile);
+        rc = libxl_domain_core_dump(ctx, *r_domid, corefile, NULL);
+        if (rc) LOG("core dump failed (rc=%d).", rc);
+        free(corefile);
         /* No point crying over spilled milk, continue on failure. */
 
         if (action == LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY)
@@ -2689,11 +2705,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
             common_domname = d_config.c_info.name;
             d_config.c_info.name = 0; /* steals allocation from config */
 
-            if (asprintf(&d_config.c_info.name,
-                         "%s--incoming", common_domname) < 0) {
-                fprintf(stderr, "Failed to allocate memory in asprintf\n");
-                exit(1);
-            }
+            xasprintf(&d_config.c_info.name, "%s--incoming", common_domname);
             *dom_info->migration_domname_r = strdup(d_config.c_info.name);
         }
     }
@@ -2777,10 +2789,7 @@ start:
     if (need_daemon) {
         char *name;
 
-        if (asprintf(&name, "xl-%s", d_config.c_info.name) < 0) {
-            LOG("Failed to allocate memory in asprintf");
-            exit(1);
-        }
+        xasprintf(&name, "xl-%s", d_config.c_info.name);
         ret = do_daemonize(name);
         free(name);
         if (ret) {
@@ -3173,11 +3182,8 @@ static int cd_insert(uint32_t domid, const char *virtdev, char *phys)
     struct stat b;
     int rc = 0;
 
-    if (asprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
-                 virtdev, phys ? phys : "") < 0) {
-        fprintf(stderr, "out of memory\n");
-        return 1;
-    }
+    xasprintf(&buf, "vdev=%s,access=r,devtype=cdrom,target=%s",
+              virtdev, phys ? phys : "");
 
     parse_disk_config(&config, buf, &disk);
 
@@ -4178,8 +4184,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     fprintf(stderr, "migration sender: Target has acknowledged transfer.\n");
 
     if (common_domname) {
-        if (asprintf(&away_domname, "%s--migratedaway", common_domname) < 0)
-            goto failed_resume;
+        xasprintf(&away_domname, "%s--migratedaway", common_domname);
         rc = libxl_domain_rename(ctx, domid, common_domname, away_domname);
         if (rc) goto failed_resume;
     }
@@ -4587,13 +4592,12 @@ int main_migrate(int argc, char **argv)
         } else {
             verbose_len = (minmsglevel_default - minmsglevel) + 2;
         }
-        if (asprintf(&rune, "exec %s %s xl%s%.*s migrate-receive%s%s",
-                     ssh_command, host,
-                     pass_tty_arg ? " -t" : "",
-                     verbose_len, verbose_buf,
-                     daemonize ? "" : " -e",
-                     debug ? " -d" : "") < 0)
-            return 1;
+        xasprintf(&rune, "exec %s %s xl%s%.*s migrate-receive%s%s",
+                  ssh_command, host,
+                  pass_tty_arg ? " -t" : "",
+                  verbose_len, verbose_buf,
+                  daemonize ? "" : " -e",
+                  debug ? " -d" : "");
     }
 
     migrate_domain(domid, rune, debug, config_filename);
@@ -6862,7 +6866,6 @@ static char *uptime_to_string(unsigned long uptime, int short_mode)
 {
     int sec, min, hour, day;
     char *time_string;
-    int ret;
 
     day = (int)(uptime / 86400);
     uptime -= (day * 86400);
@@ -6874,21 +6877,19 @@ static char *uptime_to_string(unsigned long uptime, int short_mode)
 
     if (short_mode)
         if (day > 1)
-            ret = asprintf(&time_string, "%d days, %2d:%02d", day, hour, min);
+            xasprintf(&time_string, "%d days, %2d:%02d", day, hour, min);
         else if (day == 1)
-            ret = asprintf(&time_string, "%d day, %2d:%02d", day, hour, min);
+            xasprintf(&time_string, "%d day, %2d:%02d", day, hour, min);
         else
-            ret = asprintf(&time_string, "%2d:%02d", hour, min);
+            xasprintf(&time_string, "%2d:%02d", hour, min);
     else
         if (day > 1)
-            ret = asprintf(&time_string, "%d days, %2d:%02d:%02d", day, hour, min, sec);
+            xasprintf(&time_string, "%d days, %2d:%02d:%02d", day, hour, min, sec);
         else if (day == 1)
-            ret = asprintf(&time_string, "%d day, %2d:%02d:%02d", day, hour, min, sec);
+            xasprintf(&time_string, "%d day, %2d:%02d:%02d", day, hour, min, sec);
         else
-            ret = asprintf(&time_string, "%2d:%02d:%02d", hour, min, sec);
+            xasprintf(&time_string, "%2d:%02d:%02d", hour, min, sec);
 
-    if (ret < 0)
-        return NULL;
     return time_string;
 }
 
@@ -8040,10 +8041,9 @@ int main_remus(int argc, char **argv)
         if (!ssh_command[0]) {
             rune = host;
         } else {
-            if (asprintf(&rune, "exec %s %s xl migrate-receive -r %s",
-                         ssh_command, host,
-                         daemonize ? "" : " -e") < 0)
-                return 1;
+            xasprintf(&rune, "exec %s %s xl migrate-receive -r %s",
+                      ssh_command, host,
+                      daemonize ? "" : " -e");
         }
 
         save_domain_core_begin(domid, NULL, &config_data, &config_len);
-- 
1.7.10.4

[PATCH 5/6] xl: Use xasprintf for cpupoolnumsplit names

[Ian Jackson]

Otherwise we have to do complicated reasoning about the length that %d
might produce.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c876d3e..4396095 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7735,7 +7735,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
     int n_pools;
     int node;
     int n_cpus;
-    char name[16];
+    char *name = NULL;
     libxl_uuid uuid;
     libxl_bitmap cpumap;
     libxl_cpupoolinfo *poolinfo;
@@ -7783,7 +7783,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
         goto out;
     }
 
-    snprintf(name, 15, "Pool-node%d", node);
+    xasprintf(&name, "Pool-node%d", node);
     if (libxl_cpupool_rename(ctx, name, 0)) {
         fprintf(stderr, "error on renaming Pool 0\n");
         goto out;
@@ -7828,7 +7828,8 @@ int main_cpupoolnumasplit(int argc, char **argv)
             goto out;
         }
 
-        snprintf(name, 15, "Pool-node%d", node);
+        free(name);
+        xasprintf(&name, "Pool-node%d", node);
         libxl_uuid_generate(&uuid);
         poolid = 0;
         if (libxl_cpupool_create(ctx, name, sched, cpumap, &uuid, &poolid)) {
@@ -7853,6 +7854,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
 out:
     libxl_cputopology_list_free(topology, n_cpus);
     libxl_bitmap_dispose(&cpumap);
+    free(name);
 
     return rc;
 }
-- 
1.7.10.4

[PATCH 6/6] xl: Rewrite trim()

[Ian Jackson]

This function would produce a NULL output pointer if the input was an
empty string, leading to a crash.

I don't think this is likely to be a security problem, as the two call
sites involve configuration options which callers are unlikely to
expose to other-than-fully-trusted input.

Also, the function would needlessly copy the input string (which I
care about not for performance reasons but because it makes the memory
handling more confusing), and would mishandle strings which contained
only predicate-true characters.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4396095..1966316 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -647,26 +647,23 @@ typedef int (*char_predicate_t)(const int c);
 
 static void trim(char_predicate_t predicate, const char *input, char **output)
 {
-    char *p, *q, *tmp;
+    const char *first, *after;
 
-    *output = NULL;
-    if (*input == '\000')
-        return;
-    /* Input has length >= 1 */
-
-    p = tmp = xstrdup(input);
-    /* Skip past the characters for which predicate is true */
-    while ((*p != '\000') && (predicate((unsigned char)*p)))
-        p ++;
-    q = p + strlen(p) - 1;
-    /* q points to the last non-NULL character */
-    while ((q > p) && (predicate((unsigned char)*q)))
-        q --;
-    /* q points to the last character we want */
-    q ++;
-    *q = '\000';
-    *output = xstrdup(p);
-    free(tmp);
+    for (first = input;
+         *first && predicate((unsigned char)first[0]);
+         first++)
+        ;
+
+    for (after = first + strlen(first);
+         after > first && predicate((unsigned char)after[-1]);
+         after--)
+        ;
+
+    size_t len_nonnull = after - first;
+
+    *output = xmalloc(len_nonnull + 1);
+    memcpy(output, first, len_nonnull);
+    output[len_nonnull] = 0;
 }
 
 static int split_string_into_pair(const char *str,
-- 
1.7.10.4

Re: [PATCH 0/6] libxl: config file string handling cleanups

[Ian Jackson]

Ian Jackson writes ("[PATCH 0/6] libxl: config file string handling cleanups"):
> These 6 followup patches were developed during a review of the xl
> string handling code, prompted by XSA-137.  They were embargoed until
> today.
> 
> I have reviewed the whole of xl's string handling for other bugs.
> My search terms included:
>      realloc sn?printf str \bstr \bstrcpy \bstrn \bstrcat \bmemcpy
>      \bmemchr \bp\b
> 
> Surprisingly, I found not too much untoward.
> 
> Of these only two are even backport candidates:
> 
>   xl: Do not ignore unparseable PCI BDFs
>   xl: Rewrite trim()

FYI, these were acked by Ian Campbell with his security hat on.  But
they obviously need to be posted publicly.

If there are no objections I will commit them later this week.

Ian.

Re: [PATCH 0/6] libxl: config file string handling cleanups

[Wei Liu]

On Tue, Jul 07, 2015 at 05:21:07PM +0100, Ian Jackson wrote:
> Ian Jackson writes ("[PATCH 0/6] libxl: config file string handling cleanups"):
> > These 6 followup patches were developed during a review of the xl
> > string handling code, prompted by XSA-137.  They were embargoed until
> > today.
> > 
> > I have reviewed the whole of xl's string handling for other bugs.
> > My search terms included:
> >      realloc sn?printf str \bstr \bstrcpy \bstrn \bstrcat \bmemcpy
> >      \bmemchr \bp\b
> > 
> > Surprisingly, I found not too much untoward.
> > 
> > Of these only two are even backport candidates:
> > 
> >   xl: Do not ignore unparseable PCI BDFs
> >   xl: Rewrite trim()
> 
> FYI, these were acked by Ian Campbell with his security hat on.  But
> they obviously need to be posted publicly.
> 
> If there are no objections I will commit them later this week.
> 

All patches look good to me:

Acked-by: Wei Liu <wei.liu2@citrix.com>

> Ian.