Message ID | 2107a14b9727454c3a515b3c8c1912c30abbf25e.1533686335.git.geoff@infradead.org |
---|---|
State | Accepted |
Headers | show |
Series | [v1,1/6] docker: Add strace for interactive debugging | expand |
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@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 > } >
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"); } }
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@infradead.org> --- lib/talloc/talloc.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)