Re: [libvirt] [PATCH] virsh: Avoid division by 0 in vshCalloc

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