From 38220afaffaf4fdf6770cdfad42960e19de164ed Mon Sep 17 00:00:00 2001 From: "Greg A. Woods" Date: Sun, 16 Jul 2023 17:05:25 -0700 Subject: [PATCH] finally fix the old memory leak in yajl_tree_parse() - use wrapped malloc() et al wrappers consistently - update example/parse_config.c to do memory leak detection - add a regression test using example/parse_config Several issues in lloyd/yajl complained about this leak, and comments in lloyd/yajl#102 showed a mostly correct fix though none of these issues mentioned or actually fixed the directly related error reporting problem. Fixes lloyd/yajl#102, fixes lloyd/yajl#113, fixes lloyd/yajl#168, fixes lloyd/yajl#191, fixes lloyd/yajl#223, fixes lloyd/yajl#250. Also fixes lloy/yajl#185. --- example/Makefile | 13 ++++- example/parse_config.c | 119 ++++++++++++++++++++++++++++++++++------ src/Makefile | 2 +- src/yajl/yajl_tree.h | 3 + src/yajl_parser.c | 43 +++++++++------ src/yajl_tree.c | 121 +++++++++++++++++++++++++++-------------- 6 files changed, 220 insertions(+), 81 deletions(-) diff --git a/example/Makefile b/example/Makefile index 1f34882..ace1d29 100644 --- a/example/Makefile +++ b/example/Makefile @@ -9,12 +9,19 @@ MAN = # empty # don't install proginstall: +# xxx hmmm.... stdout and stderr are reversed +# +T1 = [ $$(expr "$$(${.OBJDIR}/${PROG} < ${.CURDIR}/sample.config 2>&1)" : "memory leaks:.0.Logging/timeFormat: utc") -eq 39 ] +T2 = [ $$(expr "$$(echo '{broken:' | ${.OBJDIR}/${PROG} 2>&1)" : "tree_parse_error: lexical error: invalid char in json text.*memory leaks:.0") -eq 165 ] + regress: .if defined(USE_ASAN) - if [ -x /usr/sbin/paxctl ]; then /usr/sbin/paxctl +a ./parse_config; fi - ulimit -v unlimited && [ "$$(${.OBJDIR}/${PROG} < ${.CURDIR}/sample.config)" = "Logging/timeFormat: utc" ] + if [ -x /usr/sbin/paxctl ]; then /usr/sbin/paxctl +a ${.OBJDIR}/${PROG}; fi + ulimit -v unlimited && ${T1} + ulimit -v unlimited && ${T2} .else - [ "$$(${.OBJDIR}/${PROG} < ${.CURDIR}/sample.config)" = "Logging/timeFormat: utc" ] + ${T1} + ${T2} .endif .include diff --git a/example/parse_config.c b/example/parse_config.c index ebceef1..1a10cbd 100644 --- a/example/parse_config.c +++ b/example/parse_config.c @@ -15,11 +15,78 @@ */ #include +#include +#include #include +#include + #include "yajl/yajl_tree.h" -static unsigned char fileData[65536]; +/* context storage for memory debugging routines */ +typedef struct +{ + bool do_printfs; + unsigned int numFrees; + unsigned int numMallocs; + /* XXX: we really need a hash table here with per-allocation + * information to find any missing free() calls */ +} yajlTestMemoryContext; + +/* cast void * into context */ +#define TEST_CTX(vptr) ((yajlTestMemoryContext *) (vptr)) + +static void +yajlTestFree(void *ctx, + void *ptr) +{ + assert(ptr != NULL); + TEST_CTX(ctx)->numFrees++; + if (TEST_CTX(ctx)->do_printfs) { + fprintf(stderr, "yfree: %p\n", ptr); + } + free(ptr); +} + +static void * +yajlTestMalloc(void *ctx, + size_t sz) +{ + void *rv = NULL; + + assert(sz != 0); + TEST_CTX(ctx)->numMallocs++; + rv = malloc(sz); + assert(rv != NULL); + if (TEST_CTX(ctx)->do_printfs) { + fprintf(stderr, "yalloc: %p of %ju\n", rv, sz); + } + return rv; +} + +static void * +yajlTestRealloc(void *ctx, + void *ptr, + size_t sz) +{ + void *rv = NULL; + + if (ptr == NULL) { + assert(sz != 0); + TEST_CTX(ctx)->numMallocs++; + } else if (sz == 0) { + TEST_CTX(ctx)->numFrees++; + } + rv = realloc(ptr, sz); + assert(rv != NULL); + if (TEST_CTX(ctx)->do_printfs) { + fprintf(stderr, "yrealloc: %p -> %p of %ju\n", ptr, rv, sz); + } + return rv; +} + + +static unsigned char fileData[65536]; /* xxx: allocate to size of file, error if stdin can't be stat()ed? */ int main(void) @@ -28,35 +95,49 @@ main(void) yajl_val node; char errbuf[1024]; - /* NUL plug the buffers */ - fileData[0] = '\0'; - errbuf[0] = '\0'; + /* memory allocation debugging: allocate a structure which holds + * allocation routines */ + yajl_alloc_funcs allocFuncs = { + yajlTestMalloc, + yajlTestRealloc, + yajlTestFree, + (void *) NULL + }; + + /* memory allocation debugging: allocate a structure which collects + * statistics */ + yajlTestMemoryContext memCtx; + + memCtx.do_printfs = false; /* xxx set from a command option */ + memCtx.numMallocs = 0; + memCtx.numFrees = 0; + + allocFuncs.ctx = (void *) &memCtx; + yajl_tree_parse_afs = &allocFuncs; /* read the entire config file */ rd = fread((void *) fileData, (size_t) 1, sizeof(fileData) - 1, stdin); /* file read error handling */ - if (rd == 0 && !feof(stdin)) { - fprintf(stderr, "error encountered on file read\n"); - return 1; - } else if (rd >= sizeof(fileData) - 1) { + if ((rd == 0 && !feof(stdin)) || ferror(stdin)) { + perror("error encountered on file read"); + exit(1); + } else if (!feof(stdin)) { fprintf(stderr, "config file too big\n"); - return 1; + exit(1); } + fileData[rd] = '\0'; /* we have the whole config file in memory. let's parse it ... */ node = yajl_tree_parse((const char *) fileData, errbuf, sizeof(errbuf)); /* parse error handling */ if (node == NULL) { - fprintf(stderr, "parse_error: "); - if (strlen(errbuf)) { - fprintf(stderr, "%s", errbuf); - } else { - fprintf(stderr, "unknown error"); - } - fprintf(stderr, "\n"); - return 1; + assert(errbuf != NULL); + fprintf(stderr, "tree_parse_error: %s\n", errbuf); + fprintf(stderr, "memory leaks:\t%u\n", memCtx.numMallocs - memCtx.numFrees); + + exit(1); } /* ... and extract a nested value from the config file */ @@ -74,5 +155,7 @@ main(void) yajl_tree_free(node); - return 0; + fprintf(stderr, "memory leaks:\t%u\n", memCtx.numMallocs - memCtx.numFrees); + + exit(memCtx.numMallocs - memCtx.numFrees ? 1 : 0); } diff --git a/src/Makefile b/src/Makefile index a5b24f1..1aa9f12 100644 --- a/src/Makefile +++ b/src/Makefile @@ -166,7 +166,7 @@ install: includes incinstall: .PHONY .endif -.if ${TARGET_OSNAME} == "Linux" +.if defined(TARGET_OSNAME) && ${TARGET_OSNAME} == "Linux" # XXX stupid GNU BinUtils LD changed its command line syntax recently, # apparently without concern for backward compatability. # diff --git a/src/yajl/yajl_tree.h b/src/yajl/yajl_tree.h index ede2d04..9ab1206 100644 --- a/src/yajl/yajl_tree.h +++ b/src/yajl/yajl_tree.h @@ -37,6 +37,9 @@ extern "C" { #endif +/*+ an optional hook to allow use of custom yajl_alloc_funcs with yajl_tree_parse() +*/ +extern yajl_alloc_funcs *yajl_tree_parse_afs; + /*+ possible data types that a yajl_val_s can hold +*/ typedef enum { yajl_t_string = 1, diff --git a/src/yajl_parser.c b/src/yajl_parser.c index bbb9388..90c93cf 100644 --- a/src/yajl_parser.c +++ b/src/yajl_parser.c @@ -158,27 +158,34 @@ yajl_status yajl_do_finish(yajl_handle hand) { yajl_status stat; - stat = yajl_do_parse(hand,(const unsigned char *) " ",(size_t) 1); + size_t offset = hand->bytesConsumed; - if (stat != yajl_status_ok) return stat; + stat = yajl_do_parse(hand, (const unsigned char *) " ", (size_t) 1); + hand->bytesConsumed = offset; - switch(yajl_bs_current(hand->stateStack)) - { - case yajl_state_parse_error: - case yajl_state_lexical_error: - return yajl_status_error; - case yajl_state_got_value: - case yajl_state_parse_complete: - return yajl_status_ok; - default: - if (!(hand->flags & yajl_allow_partial_values)) - { - yajl_bs_set(hand->stateStack, yajl_state_parse_error); - hand->parseError = "premature EOF"; - return yajl_status_error; - } - return yajl_status_ok; + if (stat != yajl_status_ok) { + return stat; + } + + switch (yajl_bs_current(hand->stateStack)) { + case yajl_state_parse_error: + case yajl_state_lexical_error: + return yajl_status_error; + + case yajl_state_got_value: + case yajl_state_parse_complete: + return yajl_status_ok; + + default: + break; + } + if (!(hand->flags & yajl_allow_partial_values)) { + yajl_bs_set(hand->stateStack, yajl_state_parse_error); + hand->parseError = "premature EOF"; + + return yajl_status_error; } + return yajl_status_ok; } yajl_status diff --git a/src/yajl_tree.c b/src/yajl_tree.c index 3b0dd1e..5672752 100644 --- a/src/yajl_tree.c +++ b/src/yajl_tree.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -46,6 +47,8 @@ #define STATUS_CONTINUE 1 #define STATUS_ABORT 0 +yajl_alloc_funcs *yajl_tree_parse_afs = NULL; + struct stack_elem_s; typedef struct stack_elem_s stack_elem_t; struct stack_elem_s @@ -74,7 +77,7 @@ static yajl_val value_alloc (yajl_type type) { yajl_val v; - v = malloc (sizeof (*v)); + v = YA_MALLOC(yajl_tree_parse_afs, sizeof(*v)); if (v == NULL) return (NULL); memset (v, 0, sizeof (*v)); v->type = type; @@ -82,31 +85,37 @@ static yajl_val value_alloc (yajl_type type) return (v); } +/* note: value_free() is actually just yajl_tree_free() */ + static void yajl_object_free (yajl_val v) { size_t i; - if (!YAJL_IS_OBJECT(v)) return; + assert(YAJL_IS_OBJECT(v)); for (i = 0; i < v->u.object.len; i++) { /* __UNCONST() */ - free((void *)(uintmax_t)(const void *) v->u.object.keys[i]); + YA_FREE(yajl_tree_parse_afs, (void *)(uintmax_t)(const void *) v->u.object.keys[i]); v->u.object.keys[i] = NULL; yajl_tree_free (v->u.object.values[i]); v->u.object.values[i] = NULL; } - free((void*) v->u.object.keys); - free(v->u.object.values); - free(v); + if (v->u.object.keys != NULL) { + YA_FREE(yajl_tree_parse_afs, (void*) v->u.object.keys); + } + if (v->u.object.values != NULL) { + YA_FREE(yajl_tree_parse_afs, v->u.object.values); + } + YA_FREE(yajl_tree_parse_afs, v); } static void yajl_array_free (yajl_val v) { size_t i; - if (!YAJL_IS_ARRAY(v)) return; + assert(YAJL_IS_ARRAY(v)); for (i = 0; i < v->u.array.len; i++) { @@ -114,8 +123,8 @@ static void yajl_array_free (yajl_val v) v->u.array.values[i] = NULL; } - free(v->u.array.values); - free(v); + YA_FREE(yajl_tree_parse_afs, v->u.array.values); + YA_FREE(yajl_tree_parse_afs, v); } /* @@ -129,7 +138,7 @@ static int context_push(context_t *ctx, yajl_val v) { stack_elem_t *stack; - stack = malloc (sizeof (*stack)); + stack = YA_MALLOC(yajl_tree_parse_afs, sizeof(*stack)); if (stack == NULL) RETURN_ERROR (ctx, ENOMEM, "Out of memory"); memset (stack, 0, sizeof (*stack)); @@ -159,7 +168,10 @@ static yajl_val context_pop(context_t *ctx) v = stack->value; - free (stack); + if (stack->key != NULL) { + YA_FREE(yajl_tree_parse_afs, stack->key); + } + YA_FREE(yajl_tree_parse_afs, stack); return (v); } @@ -179,12 +191,16 @@ static int object_add_keyval(context_t *ctx, /* We're assuring that "obj" is an object in "context_add_value". */ assert(YAJL_IS_OBJECT(obj)); - tmpk = realloc((void *) obj->u.object.keys, sizeof(*(obj->u.object.keys)) * (obj->u.object.len + 1)); + tmpk = YA_REALLOC(yajl_tree_parse_afs, + (void *) obj->u.object.keys, + sizeof(*(obj->u.object.keys)) * (obj->u.object.len + 1)); if (tmpk == NULL) RETURN_ERROR(ctx, ENOMEM, "Out of memory"); obj->u.object.keys = tmpk; - tmpv = realloc(obj->u.object.values, sizeof (*obj->u.object.values) * (obj->u.object.len + 1)); + tmpv = YA_REALLOC(yajl_tree_parse_afs, + obj->u.object.values, + sizeof (*obj->u.object.values) * (obj->u.object.len + 1)); if (tmpv == NULL) RETURN_ERROR(ctx, ENOMEM, "Out of memory"); obj->u.object.values = tmpv; @@ -210,8 +226,9 @@ static int array_add_value (context_t *ctx, /* "context_add_value" will only call us with array values. */ assert(YAJL_IS_ARRAY(array)); - tmp = realloc(array->u.array.values, - sizeof(*(array->u.array.values)) * (array->u.array.len + 1)); + tmp = YA_REALLOC(yajl_tree_parse_afs, + array->u.array.values, + sizeof(*(array->u.array.values)) * (array->u.array.len + 1)); if (tmp == NULL) RETURN_ERROR(ctx, ENOMEM, "Out of memory"); array->u.array.values = tmp; @@ -259,7 +276,7 @@ static int context_add_value (context_t *ctx, yajl_val v) ctx->stack->key = v->u.string; v->u.string = NULL; - free(v); + YA_FREE(yajl_tree_parse_afs, v); return (0); } else /* if (ctx->key != NULL) */ @@ -292,10 +309,10 @@ static int handle_string (void *ctx, if (v == NULL) RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory"); - v->u.string = malloc (string_length + 1); + v->u.string = YA_MALLOC(yajl_tree_parse_afs, string_length + 1); if (v->u.string == NULL) { - free (v); + YA_FREE(yajl_tree_parse_afs, v); RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory"); } memcpy(v->u.string, string, string_length); @@ -310,13 +327,12 @@ static int handle_number (void *ctx, const char *string, size_t string_length) char *endptr; v = value_alloc(yajl_t_number); - if (v == NULL) + if (v == NULL) { RETURN_ERROR((context_t *) ctx, STATUS_ABORT, "Out of memory"); - - v->u.number.r = malloc(string_length + 1); - if (v->u.number.r == NULL) - { - free(v); + } + v->u.number.r = YA_MALLOC(yajl_tree_parse_afs, string_length + 1); + if (v->u.number.r == NULL) { + YA_FREE(yajl_tree_parse_afs, v); RETURN_ERROR((context_t *) ctx, STATUS_ABORT, "Out of memory"); } memcpy(v->u.number.r, string, string_length); @@ -463,8 +479,8 @@ yajl_val yajl_tree_parse (const char *input, /*+ Pointer to a null-terminated yajl_handle handle; yajl_status status; - char * internal_err_str; context_t ctx = { NULL, NULL, NULL, 0 }; + bool undo_afs = false; ctx.errbuf = error_buffer; ctx.errbuf_size = error_buffer_size; @@ -472,29 +488,52 @@ yajl_val yajl_tree_parse (const char *input, /*+ Pointer to a null-terminated if (error_buffer != NULL) memset (error_buffer, 0, error_buffer_size); - handle = yajl_alloc (&callbacks, NULL, &ctx); + handle = yajl_alloc(&callbacks, yajl_tree_parse_afs, &ctx); + if (yajl_tree_parse_afs == NULL) { + undo_afs = true; + yajl_tree_parse_afs = &handle->alloc; + } yajl_config(handle, yajl_allow_comments, 1); status = yajl_parse(handle, (const unsigned char *) input, strlen (input)); + if (status == yajl_status_ok) { + status = yajl_complete_parse(handle); + } if (status != yajl_status_ok) { if (error_buffer != NULL && error_buffer_size > 0) { - internal_err_str = (char *) yajl_get_error(handle, 1, - (const unsigned char *) input, - strlen(input)); - snprintf(error_buffer, error_buffer_size, "%s", internal_err_str); - YA_FREE(&(handle->alloc), internal_err_str); + char *ies; + + ies = (char *) yajl_get_error(handle, 1, + (const unsigned char *) input, + strlen(input)); + snprintf(error_buffer, error_buffer_size, "%s", ies); + YA_FREE(&(handle->alloc), ies); } while (ctx.stack) { yajl_tree_free(context_pop(&ctx)); } - yajl_free (handle); + if (ctx.root) { + yajl_tree_free(ctx.root); + } + yajl_free(handle); + if (undo_afs) { + yajl_tree_parse_afs = NULL; + } return NULL; } - yajl_complete_parse (handle); - yajl_free (handle); + if (ctx.root == NULL) { + if (error_buffer != NULL && error_buffer_size > 0) { + snprintf(error_buffer, error_buffer_size, "parse OK, but nothing to return"); + } + } + yajl_free(handle); + if (undo_afs) { + yajl_tree_parse_afs = NULL; + } + return (ctx.root); } @@ -544,24 +583,24 @@ void yajl_tree_free (yajl_val v) /*+ Pointer to a JSON value returned by if (YAJL_IS_STRING(v)) { - free(v->u.string); - free(v); + YA_FREE(yajl_tree_parse_afs, v->u.string); + YA_FREE(yajl_tree_parse_afs, v); } else if (YAJL_IS_NUMBER(v)) { - free(v->u.number.r); - free(v); + YA_FREE(yajl_tree_parse_afs, v->u.number.r); + YA_FREE(yajl_tree_parse_afs, v); } - else if (YAJL_GET_OBJECT(v)) + else if (YAJL_IS_OBJECT(v)) { yajl_object_free(v); } - else if (YAJL_GET_ARRAY(v)) + else if (YAJL_IS_ARRAY(v)) { yajl_array_free(v); } else /* if (yajl_t_true or yajl_t_false or yajl_t_null) */ { - free(v); + YA_FREE(yajl_tree_parse_afs, v); } }