Pino Toscano
2014-Aug-08 12:07 UTC
[Libguestfs] [PATCH] daemon: initialize memory when handling DeviceList params
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
Richard W.M. Jones
2014-Aug-08 18:26 UTC
Re: [Libguestfs] [PATCH] daemon: initialize memory when handling DeviceList params
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.3ACK 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