Skip to content

Commit

Permalink
Remove multiple calls to free when successively calling jq_reset.
Browse files Browse the repository at this point in the history
`jq_reset` calls `jv_free` on the `exit_code` and the `error_message` stored on the jq state.
However, it doesn't replace the actual instance of those members. This means that subsequent
calls to `jq_reset` will call `jv_free` again on those members, which in turn may call `free`
on the same pointer multiple times. Freeing the same pointer multiple times is undefined
behavior and can cause heap corruption, which is how I spotted this issue.

In practice, this issue only occurs when using a program that may `halt_error`, because that
is when the `exit_code` and `error_message` are set to values other than `jv_invalid`.
Subsequent attempts to call `jq_start` (which calls `jq_reset` internally) after hitting a
`halt_error` can cause you to run into this issue.

The changes simply reset the `exit_code` and the `error_message` to `jv_invalid` (the initial
value set in `jq_init`) after they are freed.
  • Loading branch information
Sameesunkaria committed May 31, 2024
1 parent 250475b commit 6ab6c29
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/execute.c
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,9 @@ static void jq_reset(jq_state *jq) {

jq->halted = 0;
jv_free(jq->exit_code);
jq->exit_code = jv_invalid();
jv_free(jq->error_message);
jq->error_message = jv_invalid();
if (jv_get_kind(jq->path) != JV_KIND_INVALID)
jv_free(jq->path);
jq->path = jv_null();
Expand Down
62 changes: 62 additions & 0 deletions src/jq_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

static void jv_test();
static void run_jq_tests(jv, int, FILE *, int, int);
static void run_jq_start_state_tests();
#ifdef HAVE_PTHREAD
static void run_jq_pthread_tests();
#endif
Expand Down Expand Up @@ -37,6 +38,7 @@ int jq_testsuite(jv libdirs, int verbose, int argc, char* argv[]) {
}
}
run_jq_tests(libdirs, verbose, testdata, skip, take);
run_jq_start_state_tests();
#ifdef HAVE_PTHREAD
run_jq_pthread_tests();
#endif
Expand Down Expand Up @@ -251,6 +253,66 @@ static void run_jq_tests(jv lib_dirs, int verbose, FILE *testdata, int skip, int
}


static int test_start_state(jq_state *jq, char *prog) {
int pass = 1;
jv message = jq_get_error_message(jq);
if (jv_is_valid(message)) {
printf("*** Expected error_message to be invalid after jq_start: %s\n", prog);
pass = 0;
}
jv_free(message);

jv exit_code = jq_get_exit_code(jq);
if (jv_is_valid(exit_code)) {
printf("*** Expected exit_code to be invalid after jq_start: %s\n", prog);
pass = 0;
}
jv_free(exit_code);

if (jq_halted(jq)) {
printf("*** Expected jq to not be halted after jq_start: %s\n", prog);
pass = 0;
}

return pass;
}

// Test jq_state is reset after subsequent calls to jq_start.
static void test_jq_start_resets_state(char *prog, const char *input) {
printf("Test jq_state: %s\n", prog);
jq_state *jq = jq_init();
assert(jq);

int compiled = jq_compile(jq, prog);
assert(compiled);

// First call to jq_start. Run until completion.
jv parsed_input = jv_parse(input);
assert(jv_is_valid(parsed_input));
jq_start(jq, parsed_input, 0);
assert(test_start_state(jq, prog));
while (1) {
jv result = jq_next(jq);
int valid = jv_is_valid(result);
jv_free(result);
if (!valid) { break; }
}

// Second call to jq_start.
jv parsed_input2 = jv_parse(input);
assert(jv_is_valid(parsed_input2));
jq_start(jq, parsed_input2, 0);
assert(test_start_state(jq, prog));

jq_teardown(&jq);
}

static void run_jq_start_state_tests() {
test_jq_start_resets_state(".[]", "[1,2,3]");
test_jq_start_resets_state(".[] | if .%2 == 0 then halt_error else . end", "[1,2,3]");
}


/// pthread regression test
#ifdef HAVE_PTHREAD
#define NUMBER_OF_THREADS 3
Expand Down

0 comments on commit 6ab6c29

Please sign in to comment.