[PATCH v1 4/6] lib/talloc: Fix TALLOC_ABORT

[Samuel Mendoza-Jonas]

On Wed, 2018-08-08 at 00:01 +0000, Geoff Levand wrote:
> The current TALLOC_ABORT macro had a number of problems.
> Failures were not going to the pb log, but only to stderr.
> If the object passed in was not a talloc object the printing
> of an object name would be printing random data.
> The use of a macro obscured the code.
> 
> To clean this up, remove all reference to TALLOC_ABORT and
> put the logging and abort calls directly into talloc_chunk_from_ptr.
> 
> Signed-off-by: Geoff Levand <geoff at infradead.org>
> ---
>  lib/talloc/talloc.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index d3e2065..6e2fb2f 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -40,13 +40,7 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> -
> -#if !defined(NDEBUG)
>  #include <assert.h>
> -#define TALLOC_ABORT(reason) do{ \
> -	fprintf(stderr, "%s: name: %s\n", __func__, tc->name); \
> -	assert(0 && reason);} while (0)
> -#endif
>  
>  #ifdef HAVE_SYS_TYPES_H
>  #include <sys/types.h>
> @@ -66,6 +60,7 @@
>  #include <stdint.h>
>  #endif
>  
> +#include "log/log.h"
>  #include "talloc.h"
>  
>  /* use this to force every realloc to change the pointer, to stress test
> @@ -78,12 +73,6 @@
>  #define TALLOC_MAGIC_FREE 0x7faebef3
>  #define TALLOC_MAGIC_REFERENCE ((const char *)1)
>  
> -/* by default we abort when given a bad pointer (such as when talloc_free() is
> - * called on a pointer that came from malloc() */
> -#ifndef TALLOC_ABORT
> -#define TALLOC_ABORT(reason) abort()
> -#endif
> -
>  #ifndef discard_const_p
>  #if defined(__intptr_t_defined) || defined(HAVE_INTPTR_T)
>  # define discard_const_p(type, ptr) ((type *)((intptr_t)(ptr)))
> @@ -126,9 +115,13 @@ static struct talloc_chunk *talloc_chunk_from_ptr(const void *ptr)
>  	struct talloc_chunk *tc = discard_const_p(struct talloc_chunk, ptr)-1;
>  	if (tc->u.magic != TALLOC_MAGIC) {
>  		if (tc->u.magic == TALLOC_MAGIC_FREE) {
> -			TALLOC_ABORT("Bad talloc magic value - double free");
> +			pb_log_fn("Failed: Bad talloc magic value - double free, for name: %s\n",
> +				tc->name);
> +			fprintf(stderr, "%s: name: %s\n", __func__, tc->name);
> +			assert(0 && "Bad talloc magic value - double free");
>  		} else {
> -			TALLOC_ABORT("Bad talloc magic value - unknown value");
> +			pb_log_fn("Failed: Bad talloc magic value - unknown value\n");
> +			assert(0 && "Bad talloc magic value - unknown value");
>  		}

Hi Geoff,

We don't call abort() with TALLOC_ABORT removed, so unless DEBUG is
enabled these asserts don't evaluate and we return from this function. We
*do* then segfault immediately afterwards, but should we be calling
abort() here explicitly?

Cheers,
Sam

>  	}
>