[Top][All Lists]
[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;
> }