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

[Daniel P. Berrange]

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 :|