On Fri, Aug 08, 2014 at 02:07:41PM +0200, Pino Toscano wrote:

When dealing with DeviceList parameters, the generator produces code similar to the following:

CLEANUP_FREE_STRING_LIST char **devices = NULL; [...] devices = malloc (sizeof (char *) * (args.devices.devices_len+1)); { size_t i; for (i = 0; i < args.devices.devices_len; ++i) RESOLVE_DEVICE (args.devices.devices_val[i], devices[i], , goto done); devices[i] = NULL; }

The block hidden within the RESOLVE_DEVICE macro is supposed to assign something to devices[i]; on the other hand, the code in RESOLVE_DEVICE can cause to just end (with an error) the current RPC, which would cause the cleanup of the "devices" array... whose members from the i-th to the (args.devices.devices_len-1)-th would be garbage pointers, causing random memory to be free'd (and thus crashing the daemon).

Avoid the access to garbage memory just by having a cleaned "devices" array, so there will be always a NULL element after the initialized members.

Add a test for vgcreate which passes a wrong device path causing the situation above, to test that vgcreate would fail gracefully. --- generator/actions.ml | 11 ++++++++++- generator/daemon.ml | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/generator/actions.ml b/generator/actions.ml index 48213c5..9570d9b 100644 --- a/generator/actions.ml +++ b/generator/actions.ml @@ -3929,7 +3929,16 @@ as C</dev/sda1>." }; ["vgcreate"; "VG1"; "/dev/sda1 /dev/sda2"]; ["vgcreate"; "VG2"; "/dev/sda3"]; ["vgs"]], - "is_string_list (ret, 2, \"VG1\", \"VG2\")"), [] + "is_string_list (ret, 2, \"VG1\", \"VG2\")"), []; + InitEmpty, Always, TestLastFail ( + [["part_init"; "/dev/sda"; "mbr"]; + ["part_add"; "/dev/sda"; "p"; "64"; "204799"]; + ["part_add"; "/dev/sda"; "p"; "204800"; "409599"]; + ["part_add"; "/dev/sda"; "p"; "409600"; "-64"]; + ["pvcreate"; "/dev/sda1"]; + ["pvcreate"; "/dev/sda2"]; + ["pvcreate"; "/dev/sda3"]; + ["vgcreate"; "VG1"; "/foo/bar /dev/sda2"]]), []; ]; shortdesc = "create an LVM volume group"; longdesc = "\ diff --git a/generator/daemon.ml b/generator/daemon.ml index 951729c..06d49ef 100644 --- a/generator/daemon.ml +++ b/generator/daemon.ml @@ -360,7 +360,7 @@ cleanup_free_mountable (mountable_t *mountable) pr " /* Copy the string list and apply device name translation\n"; pr " * to each one.\n"; pr " */\n"; - pr " %s = malloc (sizeof (char *) * (args.%s.%s_len+1));\n" n n n; + pr " %s = calloc (args.%s.%s_len+1, sizeof (char *));\n" n n n; pr " {\n"; pr " size_t i;\n"; pr " for (i = 0; i < args.%s.%s_len; ++i)\n" n n; -- 1.9.3

ACK Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html