bug-gnulib
[Top][All Lists]
Advanced

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

Re: xmalloc.c's xcalloc performs unnecessary test for N*S overflow


From: Jim Meyering
Subject: Re: xmalloc.c's xcalloc performs unnecessary test for N*S overflow
Date: Fri, 17 Jun 2005 10:08:45 +0200

Paul Eggert <address@hidden> wrote:

> Jim Meyering <address@hidden> writes:
>
>> As it stands, if I use both of the xalloc and calloc modules,
>> calling xcalloc ends up performing the overflow check twice,
>> first in xcalloc itself (above), and then again in calloc.
>
> Also, xcalloc contains a test n != 0 that isn't needed when
> rpl_calloc is being called.
>
> How about the following (untested) change instead?  It omits the tests
> when they're unnecessary, but it doesn't establish a dependency of
> xalloc on calloc.  I'd rather leave out dependencies like that if it's
> easy.

I agree in general, but this dependency would be easy to enforce:
Just use this simple test:

#ifndef HAVE_CALLOC
# error "you must use AC_FUNC_CALLOC with this module"
#endif

Either way is fine with me, but the code below looks a lot cleaner when
you require a working calloc.  More importantly, xmalloc.c would be a
little more maintainable if we didn't have to remember that it may be
calling a buggy calloc function.

This makes me think it'd be worthwhile to support a new section in
the modules file listing `Recommended' modules.
xalloc would recommend at least calloc, malloc, realloc and xalloc-die.
gnulib-tool could inform you of any recommended modules that your
package is not currently using.

> 2005-06-16  Paul Eggert  <address@hidden>
>
>       * xmalloc (HAVE_CALLOC): Define to 1 if not defined.
>       (xcalloc): Omit needless tests if ! HAVE_CALLOC.
>
> --- xmalloc.c 2005-05-13 23:03:58 -0700
> +++ /tmp/xmalloc.c    2005-06-16 17:14:13 -0700
> @@ -30,6 +30,10 @@
>  # define SIZE_MAX ((size_t) -1)
>  #endif
>
> +#ifndef HAVE_CALLOC
> +# define HAVE_CALLOC 1
> +#endif

That looks wrong.  Doesn't `! defined HAVE_CALLOC' mean we haven't
run AC_FUNC_CALLOC's tests?  And HAVE_CALLOC == 1 means we did
run the tests, and the system-supplied calloc passed them.

So do you really want to group the `untested, hence maybe-buggy'
calloc functions with the ones that passed AC_FUNC_CALLOC's tests?

But why reuse the HAVE_CALLOC symbol at all?
It's name isn't really accurate in this context.
How about this instead:

#ifdef HAVE_CALLOC
/* The calloc function is known to work, or we're using rpl_calloc.  */
# define BUGGY_CALLOC 0
#else
/* Without AC_FUNC_CALLOC, assume the worst: that calloc is buggy.  */
# define BUGGY_CALLOC 1
#endif

> +     proper overflow checks.  Omit some tests if !HAVE_CALLOC, since
> +     rpl_calloc catches overflow and never returns NULL if
> +     successful.  */
> +  if ((HAVE_CALLOC && xalloc_oversized (n, s))
> +      || (! (p = calloc (n, s)) && ! (HAVE_CALLOC && n == 0)))
>      xalloc_die ();
>    return p;
>  }




reply via email to

[Prev in Thread] Current Thread [Next in Thread]