Patchwork Do not abort on qemu_malloc(0) in production builds

login
register
mail settings
Submitter Anthony Liguori
Date Dec. 9, 2009, 7:04 p.m.
Message ID <1260385471-8561-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/40757/
State New
Headers show

Comments

Anthony Liguori - Dec. 9, 2009, 7:04 p.m.
qemu_malloc() does not allow size=0 to be passed in and aborts on this behavior.

Unfortunately, there is good reason to believe that within qemu, there are a
number of, so far, undetected places that assume size=0 can be safely passed.
Since we do not want to abort unnecessarily in production builds, return
qemu_malloc(1) whenever the version file indicates that this is a production
build.

Also introduce --enable-zero-malloc/--disable-zero-malloc to make this behavior
overridable.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 configure     |   24 +++++++++++++++++++++++-
 qemu-malloc.c |   17 +++++++++++++----
 2 files changed, 36 insertions(+), 5 deletions(-)
Markus Armbruster - Dec. 9, 2009, 7:28 p.m.
Anthony Liguori <aliguori@us.ibm.com> writes:

> qemu_malloc() does not allow size=0 to be passed in and aborts on this behavior.
>
> Unfortunately, there is good reason to believe that within qemu, there are a
> number of, so far, undetected places that assume size=0 can be safely passed.
> Since we do not want to abort unnecessarily in production builds, return
> qemu_malloc(1) whenever the version file indicates that this is a production
> build.
>
> Also introduce --enable-zero-malloc/--disable-zero-malloc to make this behavior
> overridable.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  configure     |   24 +++++++++++++++++++++++-
>  qemu-malloc.c |   17 +++++++++++++----
>  2 files changed, 36 insertions(+), 5 deletions(-)
>
[...]
> diff --git a/qemu-malloc.c b/qemu-malloc.c
> index 295d185..e82af26 100644
> --- a/qemu-malloc.c
> +++ b/qemu-malloc.c
> @@ -42,21 +42,30 @@ void qemu_free(void *ptr)
>      free(ptr);
>  }
>  
> +static int allow_zero_malloc(void)
> +{
> +#if defined(CONFIG_ZERO_MALLOC)
> +    return 1;
> +#else
> +    return 0;
> +#endif
> +}
> +
>  void *qemu_malloc(size_t size)
>  {
> -    if (!size) {
> +    if (!size && !allow_zero_malloc()) {
>          abort();
>      }
> -    return oom_check(malloc(size));
> +    return oom_check(malloc(size ? size : 1));
>  }
>  
>  void *qemu_realloc(void *ptr, size_t size)
>  {
>      if (size) {
>          return oom_check(realloc(ptr, size));
> -    } else {
> +    } else if (allow_zero_malloc()) {
>          if (ptr) {
> -            return realloc(ptr, size);
> +            return realloc(ptr, size ? size : 1);
>          }
>      }
>      abort();

This still aborts on qemu_realloc(NULL, 0), even with
CONFIG_ZERO_MALLOC.  Intentional?
Anthony Liguori - Dec. 9, 2009, 9:03 p.m.
Markus Armbruster wrote:
> Anthony Liguori <aliguori@us.ibm.com> writes:
>
>   
>> qemu_malloc() does not allow size=0 to be passed in and aborts on this behavior.
>>
>> Unfortunately, there is good reason to believe that within qemu, there are a
>> number of, so far, undetected places that assume size=0 can be safely passed.
>> Since we do not want to abort unnecessarily in production builds, return
>> qemu_malloc(1) whenever the version file indicates that this is a production
>> build.
>>
>> Also introduce --enable-zero-malloc/--disable-zero-malloc to make this behavior
>> overridable.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  configure     |   24 +++++++++++++++++++++++-
>>  qemu-malloc.c |   17 +++++++++++++----
>>  2 files changed, 36 insertions(+), 5 deletions(-)
>>
>>     
> [...]
>   
>> diff --git a/qemu-malloc.c b/qemu-malloc.c
>> index 295d185..e82af26 100644
>> --- a/qemu-malloc.c
>> +++ b/qemu-malloc.c
>> @@ -42,21 +42,30 @@ void qemu_free(void *ptr)
>>      free(ptr);
>>  }
>>  
>> +static int allow_zero_malloc(void)
>> +{
>> +#if defined(CONFIG_ZERO_MALLOC)
>> +    return 1;
>> +#else
>> +    return 0;
>> +#endif
>> +}
>> +
>>  void *qemu_malloc(size_t size)
>>  {
>> -    if (!size) {
>> +    if (!size && !allow_zero_malloc()) {
>>          abort();
>>      }
>> -    return oom_check(malloc(size));
>> +    return oom_check(malloc(size ? size : 1));
>>  }
>>  
>>  void *qemu_realloc(void *ptr, size_t size)
>>  {
>>      if (size) {
>>          return oom_check(realloc(ptr, size));
>> -    } else {
>> +    } else if (allow_zero_malloc()) {
>>          if (ptr) {
>> -            return realloc(ptr, size);
>> +            return realloc(ptr, size ? size : 1);
>>          }
>>      }
>>      abort();
>>     
>
> This still aborts on qemu_realloc(NULL, 0), even with
> CONFIG_ZERO_MALLOC.  Intentional?
>   
I guess not.  Should it?  Seems like a very strange case..

Regards,

Anthony Liguori
Markus Armbruster - Dec. 9, 2009, 9:35 p.m.
Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> Markus Armbruster wrote:
>> This still aborts on qemu_realloc(NULL, 0), even with
>> CONFIG_ZERO_MALLOC.  Intentional?
>>   
> I guess not.  Should it?  Seems like a very strange case..

It is a strange case, but I think the point of this commit is not to
abort on conditions perceived strange ;)

I think it should follow C89 and behave exactly like qemu_malloc(0).

Patch

diff --git a/configure b/configure
index a29839e..ede820e 100755
--- a/configure
+++ b/configure
@@ -256,6 +256,7 @@  blobs="yes"
 pkgversion=""
 check_utests="no"
 user_pie="no"
+zero_malloc=""
 
 # OS specific
 if check_define __linux__ ; then
@@ -600,6 +601,10 @@  for opt do
   ;;
   --enable-docs) docs="yes"
   ;;
+  --enable-zero-malloc) zero_malloc="yes"
+  ;;
+  --disable-zero-malloc) zero_malloc="no"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -752,6 +757,8 @@  echo "  --enable-linux-aio       enable Linux AIO support"
 echo "  --enable-io-thread       enable IO thread"
 echo "  --disable-blobs          disable installing provided firmware blobs"
 echo "  --kerneldir=PATH         look for kernel includes in PATH"
+echo "  --enable-zero-malloc     allow allocations of zero size"
+echo "  --disable-zero-malloc    abort on allocations of zero size"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -1792,8 +1799,9 @@  fi
 
 # Consult white-list to determine whether to enable werror
 # by default.  Only enable by default for git builds
+z_version=`cut -f3 -d. $source_path/VERSION`
+
 if test -z "$werror" ; then
-    z_version=`cut -f3 -d. $source_path/VERSION`
     if test "$z_version" = "50" -a \
         "$linux" = "yes" ; then
         werror="yes"
@@ -1802,6 +1810,16 @@  if test -z "$werror" ; then
     fi
 fi
 
+# Disable zero malloc errors for official releases unless explicitly told to
+# enable/disable
+if test -z "$zero_malloc" ; then
+    if test "$z_version" = "50" ; then
+	zero_malloc="no"
+    else
+	zero_malloc="yes"
+    fi
+fi
+
 if test "$werror" = "yes" ; then
     QEMU_CFLAGS="-Werror $QEMU_CFLAGS"
 fi
@@ -2109,6 +2127,10 @@  fi
 
 echo "CONFIG_UNAME_RELEASE=\"$uname_release\"" >> $config_host_mak
 
+if test "$zero_malloc" = "yes" ; then
+  echo "CONFIG_ZERO_MALLOC=y" >> $config_host_mak
+fi
+
 # USB host support
 case "$usb" in
 linux)
diff --git a/qemu-malloc.c b/qemu-malloc.c
index 295d185..e82af26 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -42,21 +42,30 @@  void qemu_free(void *ptr)
     free(ptr);
 }
 
+static int allow_zero_malloc(void)
+{
+#if defined(CONFIG_ZERO_MALLOC)
+    return 1;
+#else
+    return 0;
+#endif
+}
+
 void *qemu_malloc(size_t size)
 {
-    if (!size) {
+    if (!size && !allow_zero_malloc()) {
         abort();
     }
-    return oom_check(malloc(size));
+    return oom_check(malloc(size ? size : 1));
 }
 
 void *qemu_realloc(void *ptr, size_t size)
 {
     if (size) {
         return oom_check(realloc(ptr, size));
-    } else {
+    } else if (allow_zero_malloc()) {
         if (ptr) {
-            return realloc(ptr, size);
+            return realloc(ptr, size ? size : 1);
         }
     }
     abort();