From 6ab6c29313bccc0d7f6a109876508de2267e5dbb Mon Sep 17 00:00:00 2001 From: Samar Sunkaria Date: Thu, 30 May 2024 22:14:09 +0200 Subject: [PATCH] Remove multiple calls to free when successively calling jq_reset. `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. --- src/execute.c | 2 ++ src/jq_test.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/src/execute.c b/src/execute.c index 3d2ae0e089..cf255e49ec 100644 --- a/src/execute.c +++ b/src/execute.c @@ -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(); diff --git a/src/jq_test.c b/src/jq_test.c index 2e574642f6..dd6920a420 100644 --- a/src/jq_test.c +++ b/src/jq_test.c @@ -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 @@ -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 @@ -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