[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [fedora-virt] [PATCH libguestfs] use xmalloc and xcalloc in generated code



Richard W.M. Jones wrote:

> On Thu, Jul 02, 2009 at 11:16:37PM +0200, Jim Meyering wrote:
>> Hi Rich,
>>
>> I noticed that there were some unchecked malloc and calloc return values.
>> That could result in NULL deref upon failed allocation.  Based on what
>> you said about there not being much point in trying to recover from OOM
>> errors, I made the small textual change to convert e.g., malloc(n) to
>> xmalloc(g,n) where xmalloc now ends up calling g->abort_cb upon malloc
>> failure.  Then any following dereference of the result is guaranteed to
>> be valid.
>
> I think the patch is fine, but I generally wonder why we now have
> functions which do the same thing called xmalloc and
> [guestfs_]safe_malloc?

I've adjusted to use only safe_malloc and safe_calloc,
instead of xmalloc and xcalloc.  With this, rebased against
the latest, all tests pass and duration is 20m, much better than
the recent 33m, yet slightly worse than pre-virtio time of 15m.


>From 447dae7df43e8a98da59d94f1aef9895e087aa13 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Thu, 2 Jul 2009 15:39:08 +0200
Subject: [PATCH] use safe_malloc and safe_calloc in generated code

* src/generator.ml (safe_malloc): Define to guestfs_safe_malloc.
(safe_calloc): Define to guestfs_safe_calloc.
[most generated code]: Fail immediately upon failure of otherwise-
unchecked malloc and calloc calls.
* src/guestfs.c: Include <stddef.h>.
(xalloc_oversized): Define.
* src/guestfs.h (guestfs_safe_calloc): Declare.
---
 src/generator.ml |   44 +++++++++++++++++++++++---------------------
 src/guestfs.c    |   39 ++++++++++++++++++++++++++++++++++++++-
 src/guestfs.h    |    3 ++-
 3 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/src/generator.ml b/src/generator.ml
index 4b7efa0..c65e717 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -121,7 +121,7 @@ type flags =
   | NotInDocs		  (* do not add this function to documentation *)

 let protocol_limit_warning =
-  "Because of the message protocol, there is a transfer limit 
+  "Because of the message protocol, there is a transfer limit
 of somewhere between 2MB and 4MB.  To transfer large files you should use
 FTP."

@@ -6396,7 +6396,7 @@ Sys::Guestfs - Perl bindings for libguestfs
 =head1 SYNOPSIS

  use Sys::Guestfs;
- 
+
  my $h = Sys::Guestfs->new ();
  $h->add_drive ('guest.img');
  $h->launch ();
@@ -8144,6 +8144,8 @@ and generate_bindtests () =
 #include \"guestfs_protocol.h\"

 #define error guestfs_error
+#define safe_calloc guestfs_safe_calloc
+#define safe_malloc guestfs_safe_malloc

 static void
 print_strings (char * const* const argv)
@@ -8219,70 +8221,70 @@ print_strings (char * const* const argv)
 	     pr "  char **strs;\n";
 	     pr "  int n, i;\n";
 	     pr "  sscanf (val, \"%%d\", &n);\n";
-	     pr "  strs = malloc ((n+1) * sizeof (char *));\n";
+	     pr "  strs = safe_malloc (g, (n+1) * sizeof (char *));\n";
 	     pr "  for (i = 0; i < n; ++i) {\n";
-	     pr "    strs[i] = malloc (16);\n";
+	     pr "    strs[i] = safe_malloc (g, 16);\n";
 	     pr "    snprintf (strs[i], 16, \"%%d\", i);\n";
 	     pr "  }\n";
 	     pr "  strs[n] = NULL;\n";
 	     pr "  return strs;\n"
 	 | RIntBool _ ->
 	     pr "  struct guestfs_int_bool *r;\n";
-	     pr "  r = malloc (sizeof *r);\n";
+	     pr "  r = safe_malloc (g, sizeof *r);\n";
 	     pr "  sscanf (val, \"%%\" SCNi32, &r->i);\n";
 	     pr "  r->b = 0;\n";
 	     pr "  return r;\n"
 	 | RPVList _ ->
 	     pr "  struct guestfs_lvm_pv_list *r;\n";
 	     pr "  int i;\n";
-	     pr "  r = malloc (sizeof *r);\n";
+	     pr "  r = safe_malloc (g, sizeof *r);\n";
 	     pr "  sscanf (val, \"%%d\", &r->len);\n";
-	     pr "  r->val = calloc (r->len, sizeof *r->val);\n";
+	     pr "  r->val = safe_calloc (g, r->len, sizeof *r->val);\n";
 	     pr "  for (i = 0; i < r->len; ++i) {\n";
-	     pr "    r->val[i].pv_name = malloc (16);\n";
+	     pr "    r->val[i].pv_name = safe_malloc (g, 16);\n";
 	     pr "    snprintf (r->val[i].pv_name, 16, \"%%d\", i);\n";
 	     pr "  }\n";
 	     pr "  return r;\n"
 	 | RVGList _ ->
 	     pr "  struct guestfs_lvm_vg_list *r;\n";
 	     pr "  int i;\n";
-	     pr "  r = malloc (sizeof *r);\n";
+	     pr "  r = safe_malloc (g, sizeof *r);\n";
 	     pr "  sscanf (val, \"%%d\", &r->len);\n";
-	     pr "  r->val = calloc (r->len, sizeof *r->val);\n";
+	     pr "  r->val = safe_calloc (g, r->len, sizeof *r->val);\n";
 	     pr "  for (i = 0; i < r->len; ++i) {\n";
-	     pr "    r->val[i].vg_name = malloc (16);\n";
+	     pr "    r->val[i].vg_name = safe_malloc (g, 16);\n";
 	     pr "    snprintf (r->val[i].vg_name, 16, \"%%d\", i);\n";
 	     pr "  }\n";
 	     pr "  return r;\n"
 	 | RLVList _ ->
 	     pr "  struct guestfs_lvm_lv_list *r;\n";
 	     pr "  int i;\n";
-	     pr "  r = malloc (sizeof *r);\n";
+	     pr "  r = safe_malloc (g, sizeof *r);\n";
 	     pr "  sscanf (val, \"%%d\", &r->len);\n";
-	     pr "  r->val = calloc (r->len, sizeof *r->val);\n";
+	     pr "  r->val = safe_calloc (g, r->len, sizeof *r->val);\n";
 	     pr "  for (i = 0; i < r->len; ++i) {\n";
-	     pr "    r->val[i].lv_name = malloc (16);\n";
+	     pr "    r->val[i].lv_name = safe_malloc (g, 16);\n";
 	     pr "    snprintf (r->val[i].lv_name, 16, \"%%d\", i);\n";
 	     pr "  }\n";
 	     pr "  return r;\n"
 	 | RStat _ ->
 	     pr "  struct guestfs_stat *r;\n";
-	     pr "  r = calloc (1, sizeof (*r));\n";
+	     pr "  r = safe_calloc (g, 1, sizeof (*r));\n";
 	     pr "  sscanf (val, \"%%\" SCNi64, &r->dev);\n";
 	     pr "  return r;\n"
 	 | RStatVFS _ ->
 	     pr "  struct guestfs_statvfs *r;\n";
-	     pr "  r = calloc (1, sizeof (*r));\n";
+	     pr "  r = safe_calloc (g, 1, sizeof (*r));\n";
 	     pr "  sscanf (val, \"%%\" SCNi64, &r->bsize);\n";
 	     pr "  return r;\n"
 	 | RHashtable _ ->
 	     pr "  char **strs;\n";
 	     pr "  int n, i;\n";
 	     pr "  sscanf (val, \"%%d\", &n);\n";
-	     pr "  strs = malloc ((n*2+1) * sizeof (*strs));\n";
+	     pr "  strs = safe_malloc (g, (n*2+1) * sizeof (*strs));\n";
 	     pr "  for (i = 0; i < n; ++i) {\n";
-	     pr "    strs[i*2] = malloc (16);\n";
-	     pr "    strs[i*2+1] = malloc (16);\n";
+	     pr "    strs[i*2] = safe_malloc (g, 16);\n";
+	     pr "    strs[i*2+1] = safe_malloc (g, 16);\n";
 	     pr "    snprintf (strs[i*2], 16, \"%%d\", i);\n";
 	     pr "    snprintf (strs[i*2+1], 16, \"%%d\", i);\n";
 	     pr "  }\n";
@@ -8291,9 +8293,9 @@ print_strings (char * const* const argv)
 	 | RDirentList _ ->
 	     pr "  struct guestfs_dirent_list *r;\n";
 	     pr "  int i;\n";
-	     pr "  r = malloc (sizeof *r);\n";
+	     pr "  r = safe_malloc (g, sizeof *r);\n";
 	     pr "  sscanf (val, \"%%d\", &r->len);\n";
-	     pr "  r->val = calloc (r->len, sizeof *r->val);\n";
+	     pr "  r->val = safe_calloc (g, r->len, sizeof *r->val);\n";
 	     pr "  for (i = 0; i < r->len; ++i)\n";
 	     pr "    r->val[i].ino = i;\n";
 	     pr "  return r;\n"
diff --git a/src/guestfs.c b/src/guestfs.c
index c3bce0b..350d848 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -1,5 +1,5 @@
 /* libguestfs
- * Copyright (C) 2009 Red Hat Inc. 
+ * Copyright (C) 2009 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -24,6 +24,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
+#include <stddef.h>
 #include <unistd.h>
 #include <ctype.h>
 #include <string.h>
@@ -453,6 +454,42 @@ guestfs_safe_malloc (guestfs_h *g, size_t nbytes)
   return ptr;
 }

+/* Return 1 if an array of N objects, each of size S, cannot exist due
+   to size arithmetic overflow.  S must be positive and N must be
+   nonnegative.  This is a macro, not an inline function, so that it
+   works correctly even when SIZE_MAX < N.
+
+   By gnulib convention, SIZE_MAX represents overflow in size
+   calculations, so the conservative dividend to use here is
+   SIZE_MAX - 1, since SIZE_MAX might represent an overflowed value.
+   However, malloc (SIZE_MAX) fails on all known hosts where
+   sizeof (ptrdiff_t) <= sizeof (size_t), so do not bother to test for
+   exactly-SIZE_MAX allocations on such hosts; this avoids a test and
+   branch when S is known to be 1.  */
+# define xalloc_oversized(n, s) \
+    ((size_t) (sizeof (ptrdiff_t) <= sizeof (size_t) ? -1 : -2) / (s) < (n))
+
+/* Technically we should add an autoconf test for this, testing for the desired
+   functionality, like what's done in gnulib, but for now, this is fine.  */
+#define HAVE_GNU_CALLOC (__GLIBC__ >= 2)
+
+/* Allocate zeroed memory for N elements of S bytes, with error
+   checking.  S must be nonzero.  */
+void *
+guestfs_safe_calloc (guestfs_h *g, size_t n, size_t s)
+{
+  /* From gnulib's calloc function in xmalloc.c.  */
+  void *p;
+  /* Test for overflow, since some calloc implementations don't have
+     proper overflow checks.  But omit overflow and size-zero tests if
+     HAVE_GNU_CALLOC, since GNU calloc catches overflow and never
+     returns NULL if successful.  */
+  if ((! HAVE_GNU_CALLOC && xalloc_oversized (n, s))
+      || (! (p = calloc (n, s)) && (HAVE_GNU_CALLOC || n != 0)))
+    g->abort_cb ();
+  return p;
+}
+
 void *
 guestfs_safe_realloc (guestfs_h *g, void *ptr, int nbytes)
 {
diff --git a/src/guestfs.h b/src/guestfs.h
index 201d60c..264986f 100644
--- a/src/guestfs.h
+++ b/src/guestfs.h
@@ -1,5 +1,5 @@
 /* libguestfs
- * Copyright (C) 2009 Red Hat Inc. 
+ * Copyright (C) 2009 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -77,6 +77,7 @@ extern void guestfs_error (guestfs_h *g, const char *fs, ...)
 extern void guestfs_perrorf (guestfs_h *g, const char *fs, ...)
   __attribute__((format (printf,2,3)));
 extern void *guestfs_safe_malloc (guestfs_h *g, size_t nbytes);
+extern void *guestfs_safe_calloc (guestfs_h *g, size_t n, size_t s);
 extern void *guestfs_safe_realloc (guestfs_h *g, void *ptr, int nbytes);
 extern char *guestfs_safe_strdup (guestfs_h *g, const char *str);
 extern void *guestfs_safe_memdup (guestfs_h *g, void *ptr, size_t size);
-- 
1.6.3.3.507.gc6b5a


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]