Skip to content

Commit

Permalink
yajl_gen.c, yajl_parser.c, yajl_tree.c: remove alloc failure checks
Browse files Browse the repository at this point in the history 
- the memory allocators must do their own checks -- checking again in
  the calling code is wasteful and misleading.
  • Loading branch information
@robohack
robohack committed Feb 8, 2024
1 parent b52d4c1 commit 159bb72
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 51 deletions.
3 changes: 0 additions & 3 deletions src/yajl_gen.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ yajl_gen_alloc(const yajl_alloc_funcs *afs)
}

g = (yajl_gen) YA_MALLOC(afs, sizeof(struct yajl_gen_t));
if (!g) {
return NULL;
}
memset((void *) g, 0, sizeof(struct yajl_gen_t));
/* copy in pointers to allocation routines */
g->alloc = *afs;
Expand Down
11 changes: 4 additions & 7 deletions src/yajl_parser.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ yajl_render_error_string(yajl_handle hand, const unsigned char * jsonText,
memneeded += strlen(errorText);
}
str = (unsigned char *) YA_MALLOC(&(hand->alloc), memneeded + 2);
if (!str) return NULL;
str[0] = 0;
strcat((char *) str, errorType);
strcat((char *) str, " error");
Expand Down Expand Up @@ -131,12 +130,10 @@ yajl_render_error_string(yajl_handle hand, const unsigned char * jsonText,
YA_MALLOC(&(hand->alloc), (size_t)(strlen((char *) str) +
strlen((char *) text) +
strlen(arrow) + 1));
if (newStr) {
newStr[0] = 0;
strcat((char *) newStr, (char *) str);
strcat((char *) newStr, (char *) text);
strcat((char *) newStr, arrow);
}
newStr[0] = 0;
strcat((char *) newStr, (char *) str);
strcat((char *) newStr, (char *) text);
strcat((char *) newStr, arrow);
YA_FREE(&(hand->alloc), str);
str = (unsigned char *) newStr;
}
Expand Down
47 changes: 6 additions & 41 deletions src/yajl_tree.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ static yajl_val value_alloc (yajl_type type)
yajl_val v;

v = YA_MALLOC(yajl_tree_parse_afs, sizeof(*v));
if (v == NULL) return (NULL);
memset (v, 0, sizeof (*v));
memset(v, 0, sizeof (*v));
v->type = type;

return (v);
Expand Down Expand Up @@ -138,14 +137,13 @@ static int context_push(context_t *ctx, yajl_val v)
{
stack_elem_t *stack;

assert(v);

stack = YA_MALLOC(yajl_tree_parse_afs, sizeof(*stack));
if (stack == NULL)
RETURN_ERROR (ctx, ENOMEM, "Out of memory");
memset (stack, 0, sizeof (*stack));
memset(stack, 0, sizeof (*stack));

assert ((ctx->stack == NULL)
|| YAJL_IS_OBJECT (v)
|| YAJL_IS_ARRAY (v));
assert(YAJL_IS_OBJECT (v) ||
YAJL_IS_ARRAY (v));

stack->value = v;
stack->next = ctx->stack;
Expand Down Expand Up @@ -194,15 +192,11 @@ static int object_add_keyval(context_t *ctx,
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 = 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;

obj->u.object.keys[obj->u.object.len] = key;
Expand All @@ -229,8 +223,6 @@ static int array_add_value (context_t *ctx,
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;
array->u.array.values[array->u.array.len] = value;
array->u.array.len++;
Expand Down Expand Up @@ -306,15 +298,7 @@ static int handle_string (void *ctx,
yajl_val v;

v = value_alloc (yajl_t_string);
if (v == NULL)
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");

v->u.string = YA_MALLOC(yajl_tree_parse_afs, string_length + 1);
if (v->u.string == NULL)
{
YA_FREE(yajl_tree_parse_afs, v);
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");
}
memcpy(v->u.string, string, string_length);
v->u.string[string_length] = 0;

Expand All @@ -327,14 +311,7 @@ static int handle_number (void *ctx, const char *string, size_t string_length)
char *endptr;

v = value_alloc(yajl_t_number);
if (v == NULL) {
RETURN_ERROR((context_t *) ctx, STATUS_ABORT, "Out of memory");
}
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);
v->u.number.r[string_length] = 0;

Expand All @@ -360,9 +337,6 @@ static int handle_start_map (void *ctx)
yajl_val v;

v = value_alloc(yajl_t_object);
if (v == NULL)
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");

v->u.object.keys = NULL;
v->u.object.values = NULL;
v->u.object.len = 0;
Expand All @@ -386,9 +360,6 @@ static int handle_start_array (void *ctx)
yajl_val v;

v = value_alloc(yajl_t_array);
if (v == NULL)
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");

v->u.array.values = NULL;
v->u.array.len = 0;

Expand All @@ -411,9 +382,6 @@ static int handle_boolean (void *ctx, int boolean_value)
yajl_val v;

v = value_alloc (boolean_value ? yajl_t_true : yajl_t_false);
if (v == NULL)
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");

return ((context_add_value (ctx, v) == 0) ? STATUS_CONTINUE : STATUS_ABORT);
}

Expand All @@ -422,9 +390,6 @@ static int handle_null (void *ctx)
yajl_val v;

v = value_alloc (yajl_t_null);
if (v == NULL)
RETURN_ERROR ((context_t *) ctx, STATUS_ABORT, "Out of memory");

return ((context_add_value (ctx, v) == 0) ? STATUS_CONTINUE : STATUS_ABORT);
}

Expand Down

2 comments on commit 159bb72

@marcstern
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "must do their own check"?

g = (yajl_gen) YA_MALLOC(afs, sizeof(struct yajl_gen_t));
if (!g) { return NULL; }
memset((void *) g, 0, sizeof(struct yajl_gen_t));

In the above code, if the YA_MALLOC cannot find enough memory, what is it supposed to do?
if g is null, the memset will use an invalid pointer and the program will usually crash instead of gracefully handling this (and log a clean error message). How could YA_MALLOC break the flow, know where to log the error, etc.?

@robohack
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the documentation for yajl_alloc_funcs says:

/*+
 *  A structure which can be passed to yajl_*_alloc routines to allow the
 *  client to specify memory allocation functions to be used.
 *
 *  These functions should check for and handle any allocation errors, including
 *  considering requests to (*.realloc)() for zero bytes (sz==0) as an error.
 +*/

See example/parse_config.c where assert() is used to check for allocation failures. Note also that as shown in this example a context pointer can also be included in yajl_alloc_funcs to allow an application specific response, such as jumping to safer cleanup, logging, and exit. Users could also make use of a pool allocator (with the specific pool being used indicated in another context variable) and then on error jump to a cleanup that aborted the operation, released the whole pool, and potentially allowed the application to return to operation.

I will (hopefully soon) be fixing json_verify.c and json_reformat.c to also do error and leak detection as in parse_config.c, and I'll probably change the default allocators to at least use assert() if not directly use abort() (to avoid -DNDEBUG).

Please sign in to comment.