[libvirt] [PATCH] virsh: Avoid division by 0 in vshCalloc
vshCalloc function uses xalloc_oversized macro that can't take 0 as it's second argument. If vshCalloc is called with size 0, virsh ends with a floating point exception. This patch changes vshCalloc to return NULL if no memory is requested. --- tools/virsh.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 53d1825..d3d5c6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x; + if (!size) + return NULL; + if (!xalloc_oversized(nmemb, size) && VIR_ALLOC_N(x, nmemb * size) == 0) return x; -- 1.7.8.6
On Wed, Jul 04, 2012 at 11:05:40AM +0200, Peter Krempa wrote:
vshCalloc function uses xalloc_oversized macro that can't take 0 as it's second argument. If vshCalloc is called with size 0, virsh ends with a floating point exception.
This patch changes vshCalloc to return NULL if no memory is requested. --- tools/virsh.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 53d1825..d3d5c6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x;
+ if (!size) + return NULL; + if (!xalloc_oversized(nmemb, size) &&
IMHO this div-by-zero problem is a bug in the xalloc_oversized macro & we should fix it there. The scenario seen here in virsh is a fairly common and so div-by-zero could affect any other usage of that macro Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/04/12 11:09, Daniel P. Berrange wrote:
On Wed, Jul 04, 2012 at 11:05:40AM +0200, Peter Krempa wrote:
vshCalloc function uses xalloc_oversized macro that can't take 0 as it's second argument. If vshCalloc is called with size 0, virsh ends with a floating point exception.
This patch changes vshCalloc to return NULL if no memory is requested. --- tools/virsh.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 53d1825..d3d5c6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x;
+ if (!size) + return NULL; + if (!xalloc_oversized(nmemb, size) &&
IMHO this div-by-zero problem is a bug in the xalloc_oversized macro & we should fix it there. The scenario seen here in virsh is a fairly common and so div-by-zero could affect any other usage of that macro
Yes it could. But the docs for the macro state that it shouldn't be called with 0 as the second argument: /* 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. But assuming that 0 elements of something will never overflow we could change the macro to: from: # ifndef xalloc_oversized # define xalloc_oversized(n, s) \ ((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)) # endif to: (s?((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)):0) which would take care of the 0 argument. Is this what you had in mind? Peter
Daniel
On Wed, Jul 04, 2012 at 11:38:10AM +0200, Peter Krempa wrote:
On 07/04/12 11:09, Daniel P. Berrange wrote:
On Wed, Jul 04, 2012 at 11:05:40AM +0200, Peter Krempa wrote:
vshCalloc function uses xalloc_oversized macro that can't take 0 as it's second argument. If vshCalloc is called with size 0, virsh ends with a floating point exception.
This patch changes vshCalloc to return NULL if no memory is requested. --- tools/virsh.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 53d1825..d3d5c6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x;
+ if (!size) + return NULL; + if (!xalloc_oversized(nmemb, size) &&
IMHO this div-by-zero problem is a bug in the xalloc_oversized macro & we should fix it there. The scenario seen here in virsh is a fairly common and so div-by-zero could affect any other usage of that macro
Yes it could. But the docs for the macro state that it shouldn't be called with 0 as the second argument:
/* 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.
But assuming that 0 elements of something will never overflow we could change the macro to:
from: # ifndef xalloc_oversized # define xalloc_oversized(n, s) \ ((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)) # endif
to: (s?((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)):0)
which would take care of the 0 argument.
Is this what you had in mind?
Yes, I think it is wrong to expect that 'S' must be non-zero, so we should change the docs too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/04/12 11:43, Daniel P. Berrange wrote:
On Wed, Jul 04, 2012 at 11:38:10AM +0200, Peter Krempa wrote:
On 07/04/12 11:09, Daniel P. Berrange wrote:
On Wed, Jul 04, 2012 at 11:05:40AM +0200, Peter Krempa wrote:
vshCalloc function uses xalloc_oversized macro that can't take 0 as it's second argument. If vshCalloc is called with size 0, virsh ends with a floating point exception.
This patch changes vshCalloc to return NULL if no memory is requested. --- tools/virsh.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 53d1825..d3d5c6a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x;
+ if (!size) + return NULL; + if (!xalloc_oversized(nmemb, size) &&
IMHO this div-by-zero problem is a bug in the xalloc_oversized macro & we should fix it there. The scenario seen here in virsh is a fairly common and so div-by-zero could affect any other usage of that macro
Yes it could. But the docs for the macro state that it shouldn't be called with 0 as the second argument:
/* 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.
But assuming that 0 elements of something will never overflow we could change the macro to:
from: # ifndef xalloc_oversized # define xalloc_oversized(n, s) \ ((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)) # endif
to: (s?((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)):0)
which would take care of the 0 argument.
Is this what you had in mind?
Yes, I think it is wrong to expect that 'S' must be non-zero, so we should change the docs too.
I found the actual problem for this. I think that this macro is in order. S is the size of the array element, which doesn't make sense to be 0 and N is the count of elements. The floating point exception happened because the count and size arguments were interchanged in the problematic place. I'll send the patch anyways, but I don't think now, that the macro requires fixing. Peter
Daniel
On 07/04/2012 04:14 AM, Peter Krempa wrote:
@@ -460,6 +460,9 @@ _vshCalloc(vshControl *ctl, size_t nmemb, size_t size, const char *filename, int { char *x;
+ if (!size) + return NULL;
NACK to this; vshCalloc is defined as always returning non-NULL if it returns (even if it is a dummy malloc(0), assuming glibc semantics where you get a 1-byte allocation after all).
But assuming that 0 elements of something will never overflow we could change the macro to:
from: # ifndef xalloc_oversized # define xalloc_oversized(n, s) \ ((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)) # endif
to: (s?((size_t) (sizeof(ptrdiff_t) <= sizeof(size_t) ? -1 : -2) / (s) < (n)):0)
NACK to this; this macro was copied from gnulib, and any changes should be made in gnulib first. But you'll have a hard time changing gnulib, since...
I found the actual problem for this. I think that this macro is in order. S is the size of the array element, which doesn't make sense to be 0 and N is the count of elements. The floating point exception happened because the count and size arguments were interchanged in the problematic place.
This is the real problem. We were using the macro incorrectly, and it is not a bug in the macro itself.
I'll send the patch anyways, but I don't think now, that the macro requires fixing.
Agreed - leave the macro alone. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa