This is an automated email from the ASF dual-hosted git repository. reiabreu pushed a commit to branch worker-launcher-config-value-arrays in repository https://gitbox.apache.org/repos/asf/storm.git
commit ac255189ac753ee5a3071ef9cebf4285b8dd372b Author: Rui Abreu <[email protected]> AuthorDate: Mon Aug 24 10:37:24 2026 +0100 Return independently owned value arrays from get_values get_values returned the array from extract_values_delim directly, whose entries point into the single buffer that strtok_r tokenized in place. free_values then freed values[0] as if it were that buffer, which only holds when the first token starts at the buffer: a value with a leading delimiter (e.g. ",a") left free_values calling free() on an interior pointer, and a value made up only of delimiters produced an array that was never NULL-terminated. get_values now copies the tokens into an independently owned, NULL-terminated array and frees the parsed buffer itself; free_values frees each element and the array (and no longer dereferences a NULL argument before checking it); extract_values_delim NULL-terminates the zero-token case. Adds test_get_values_degenerate covering leading, trailing, and delimiter-only values. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- .../native/worker-launcher/impl/configuration.c | 45 +++++++++++++---- .../worker-launcher/test/test-worker-launcher.c | 58 ++++++++++++++++++++++ 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/storm-core/src/native/worker-launcher/impl/configuration.c b/storm-core/src/native/worker-launcher/impl/configuration.c index 985e9312e..0d63f1e4d 100644 --- a/storm-core/src/native/worker-launcher/impl/configuration.c +++ b/storm-core/src/native/worker-launcher/impl/configuration.c @@ -295,7 +295,31 @@ char * get_value(const char* key) { */ char ** get_values(const char * key) { char *value = get_value(key); - return extract_values_delim(value, ","); + // extract_values_delim tokenizes value in place and returns pointers into it. + // Copy the tokens into an independently owned array and release the backing + // buffer here, so a leading or trailing delimiter (e.g. ",a") never leaves + // free_values with an interior pointer to free, and free_values can release + // each element without assuming anything about the original buffer. + char **tokens = extract_values_delim(value, ","); + if (tokens == NULL) { + free(value); + return NULL; + } + int count = 0; + while (tokens[count] != NULL) { + count++; + } + char **out = (char **) malloc(sizeof(char *) * (count + 1)); + if (out != NULL) { + int i; + for (i = 0; i < count; i++) { + out[i] = strdup(tokens[i]); + } + out[count] = NULL; + } + free(tokens); + free(value); + return out; } /** @@ -326,20 +350,23 @@ char ** extract_values_delim(char *value, const char *delim) { tempTok = strtok_r(NULL, delim, &tempstr); } } - if (size > 0) { + // Terminate whenever an array was allocated, including the zero-token case + // (e.g. a value consisting only of delimiters). Callers walk the array until + // the first NULL, so an unterminated array would be read past its contents. + if (toPass != NULL) { toPass[size] = NULL; } return toPass; } -// free an entry set of values +// free a NULL-terminated array of values and the array itself void free_values(char** values) { - if (*values != NULL) { - free(*values); - *values = NULL; + if (values == NULL) { + return; } - if (values != NULL) { - free(values); - values = NULL; + char** v; + for (v = values; *v != NULL; v++) { + free(*v); } + free(values); } diff --git a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c index 4226d527d..a963c9c03 100644 --- a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c +++ b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c @@ -263,6 +263,63 @@ void test_signal_container_group() { } } +// get_values must return an independently owned, NULL-terminated array for any +// value, including ones with leading, trailing, or only delimiters, so that +// free_values can release it without freeing an interior pointer of the parsed +// buffer. Run in a child (via run_test_in_child) so the temporary config that +// read_config installs does not leak into later tests. +void test_get_values_degenerate() { + const char* cfg = TEST_ROOT "/get-values.cfg"; + FILE* f = fopen(cfg, "w"); + if (f == NULL) { + printf("FAIL: could not write %s\n", cfg); + exit(1); + } + fprintf(f, "test.values.normal=a,b,c\n"); + fprintf(f, "test.values.leading=,a,b\n"); + fprintf(f, "test.values.trailing=a,b,\n"); + fprintf(f, "test.values.only.delims=,,\n"); + fclose(f); + read_config(cfg); + + char** v = get_values("test.values.normal"); + if (v == NULL || v[0] == NULL || strcmp(v[0], "a") != 0 + || v[1] == NULL || strcmp(v[1], "b") != 0 + || v[2] == NULL || strcmp(v[2], "c") != 0 || v[3] != NULL) { + printf("FAIL: get_values did not return [a,b,c] for a normal value\n"); + exit(1); + } + free_values(v); + + // A leading delimiter makes the first token an interior pointer of the parsed + // buffer; get_values must still yield [a,b] and free_values must not choke. + v = get_values("test.values.leading"); + if (v == NULL || v[0] == NULL || strcmp(v[0], "a") != 0 + || v[1] == NULL || strcmp(v[1], "b") != 0 || v[2] != NULL) { + printf("FAIL: get_values did not return [a,b] for a leading-delimiter value\n"); + exit(1); + } + free_values(v); + + v = get_values("test.values.trailing"); + if (v == NULL || v[0] == NULL || strcmp(v[0], "a") != 0 + || v[1] == NULL || strcmp(v[1], "b") != 0 || v[2] != NULL) { + printf("FAIL: get_values did not return [a,b] for a trailing-delimiter value\n"); + exit(1); + } + free_values(v); + + // Only delimiters: an empty but NULL-terminated array, not an unterminated one. + v = get_values("test.values.only.delims"); + if (v == NULL || v[0] != NULL) { + printf("FAIL: get_values did not return an empty terminated array for a delimiter-only value\n"); + exit(1); + } + free_values(v); + + printf("get_values degenerate-value handling OK\n"); +} + int main(int argc, char **argv) { LOGFILE = stdout; ERRORFILE = stderr; @@ -314,6 +371,7 @@ int main(int argc, char **argv) { // when they change user they don't give up our privs run_test_in_child("test_signal_container", test_signal_container); run_test_in_child("test_signal_container_group", test_signal_container_group); + run_test_in_child("test_get_values_degenerate", test_get_values_degenerate); seteuid(0); run("rm -fr " TEST_ROOT);
