From cf23604413610e881d3edad9594b61c1b2630a4e Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Fri, 26 Dec 2025 19:19:51 -0800 Subject: [PATCH 1/6] atf-c: tp_main.c: avoid use-after-free in params_fini(..) Set the `m_tcname` parameter to NULL after calling free to ensure that a wild pointer isn't used after free. Signed-off-by: Enji Cooper --- atf-c/detail/tp_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/atf-c/detail/tp_main.c b/atf-c/detail/tp_main.c index 8236d6af..e32da4db 100644 --- a/atf-c/detail/tp_main.c +++ b/atf-c/detail/tp_main.c @@ -198,8 +198,10 @@ params_fini(struct params *p) atf_map_fini(&p->m_config); atf_fs_path_fini(&p->m_resfile); atf_fs_path_fini(&p->m_srcdir); - if (p->m_tcname != NULL) + if (p->m_tcname != NULL) { free(p->m_tcname); + p->m_tcname = NULL; + } } static From 32f63fd142c54c7b2d6734aee5857013578d3a57 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Fri, 26 Dec 2025 19:24:02 -0800 Subject: [PATCH 2/6] atf_process_exec_list: unconditionally free argv2 Prior to this change, exiting the function early would leak argv2. Free it unconditionally to avoid leaking the memory. Signed-off-by: Enji Cooper --- atf-c/detail/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atf-c/detail/process.c b/atf-c/detail/process.c index 2bc8294c..165fc114 100644 --- a/atf-c/detail/process.c +++ b/atf-c/detail/process.c @@ -659,7 +659,7 @@ atf_process_exec_list(atf_process_status_t *s, err = atf_process_exec_array(s, prog, argv2, outsb, errsb, prehook); - free(argv2); out: + free(argv2); return err; } From c43361c5ae7ad19feff8c399d1be2671e0f429d8 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Fri, 26 Dec 2025 19:50:40 -0800 Subject: [PATCH 3/6] `process_params(..)`: call `process_fini(..)` when exiting on error The code prior to this change would exit the function in select scenarios after allocating memory. Call `process_fini(..)` when exiting on error to ensure that all memory allocated as part of the function has been properly cleaned up. Signed-off-by: Enji Cooper --- atf-c/detail/tp_main.c | 41 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 27 deletions(-) diff --git a/atf-c/detail/tp_main.c b/atf-c/detail/tp_main.c index e32da4db..bf124cfb 100644 --- a/atf-c/detail/tp_main.c +++ b/atf-c/detail/tp_main.c @@ -292,17 +292,13 @@ static atf_error_t handle_tcarg(const char *tcarg, char **tcname, enum tc_part *tcpart) { - atf_error_t err; - - err = atf_no_error(); + char *delim; *tcname = strdup(tcarg); - if (*tcname == NULL) { - err = atf_no_memory_error(); - goto out; - } + if (*tcname == NULL) + return atf_no_memory_error(); - char *delim = strchr(*tcname, ':'); + delim = strchr(*tcname, ':'); if (delim != NULL) { *delim = '\0'; @@ -312,13 +308,11 @@ handle_tcarg(const char *tcarg, char **tcname, enum tc_part *tcpart) } else if (strcmp(delim, "cleanup") == 0) { *tcpart = CLEANUP; } else { - err = usage_error("Invalid test case part `%s'", delim); - goto out; + return usage_error("Invalid test case part `%s'", delim); } } -out: - return err; + return atf_no_error(); } static @@ -329,9 +323,7 @@ process_params(int argc, char **argv, struct params *p) int ch; int old_opterr; - err = params_init(p, argv[0]); - if (atf_is_error(err)) - goto out; + err = atf_no_error(); old_opterr = opterr; opterr = 0; @@ -389,10 +381,6 @@ process_params(int argc, char **argv, struct params *p) } } - if (atf_is_error(err)) - params_fini(p); - -out: return err; } @@ -486,12 +474,8 @@ run_tc(const atf_tp_t *tp, struct params *p, int *exitcode) { atf_error_t err; - err = atf_no_error(); - - if (!atf_tp_has_tc(tp, p->m_tcname)) { - err = usage_error("Unknown test case `%s'", p->m_tcname); - goto out; - } + if (!atf_tp_has_tc(tp, p->m_tcname)) + return usage_error("Unknown test case `%s'", p->m_tcname); if (!atf_env_has("__RUNNING_INSIDE_ATF_RUN") || strcmp(atf_env_get( "__RUNNING_INSIDE_ATF_RUN"), "internal-yes-value") != 0) @@ -531,7 +515,6 @@ run_tc(const atf_tp_t *tp, struct params *p, int *exitcode) } INV(!atf_is_error(err)); -out: return err; } @@ -546,10 +529,14 @@ controlled_main(int argc, char **argv, atf_tp_t tp; char **raw_config; - err = process_params(argc, argv, &p); + err = params_init(&p, argv[0]); if (atf_is_error(err)) goto out; + err = process_params(argc, argv, &p); + if (atf_is_error(err)) + goto out_p; + err = handle_srcdir(&p); if (atf_is_error(err)) goto out_p; From bca0a21156dd097bd3b0e38818ad345a0694d1fd Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Sat, 27 Dec 2025 01:00:37 -0800 Subject: [PATCH 4/6] Address scan-build reported issues Split off memory initialization and management from other resource initialization so the memory can be allocated and freed once at a high-level instead of dealing with the complexity that the code had previously. NULL out memory after it's freed to avoid double-free situations. Initialize several variables, e.g., file descriptors, to bogus values to avoid issues with code being called twice in error and accidentally closing valid file descriptors. Signed-off-by: Enji Cooper --- atf-c/detail/process.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/atf-c/detail/process.c b/atf-c/detail/process.c index 165fc114..c19e27e4 100644 --- a/atf-c/detail/process.c +++ b/atf-c/detail/process.c @@ -67,6 +67,8 @@ stream_prepare_init(stream_prepare_t *sp, const atf_process_stream_t *sb) const int type = atf_process_stream_type(sb); sp->m_sb = sb; + sp->m_pipefds[0] = -1; + sp->m_pipefds[1] = -1; sp->m_pipefds_ok = false; if (type == atf_process_stream_type_capture) { @@ -239,14 +241,12 @@ atf_process_status_coredump(const atf_process_status_t *s) * --------------------------------------------------------------------- */ static -atf_error_t +void atf_process_child_init(atf_process_child_t *c) { c->m_pid = 0; c->m_stdout = -1; c->m_stderr = -1; - - return atf_no_error(); } static @@ -381,25 +381,17 @@ parent_connect(const stream_prepare_t *sp, int *fd) } static -atf_error_t +void do_parent(atf_process_child_t *c, const pid_t pid, const stream_prepare_t *outsp, const stream_prepare_t *errsp) { - atf_error_t err; - - err = atf_process_child_init(c); - if (atf_is_error(err)) - goto out; c->m_pid = pid; parent_connect(outsp, &c->m_stdout); parent_connect(errsp, &c->m_stderr); - -out: - return err; } static @@ -475,7 +467,7 @@ fork_with_streams(atf_process_child_t *c, abort(); err = atf_no_error(); } else { - err = do_parent(c, pid, &outsp, &errsp); + do_parent(c, pid, &outsp, &errsp); if (atf_is_error(err)) goto err_errpipe; } @@ -522,6 +514,8 @@ atf_process_fork(atf_process_child_t *c, atf_process_stream_t inherit_outsb, inherit_errsb; const atf_process_stream_t *real_outsb, *real_errsb; + atf_process_child_init(c); + real_outsb = NULL; /* Shut up GCC warning. */ err = init_stream_w_default(outsb, &inherit_outsb, &real_outsb); if (atf_is_error(err)) From a3dd72ccfb9914e0ab891489e99e18dc88e5b360 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Sat, 27 Dec 2025 01:07:00 -0800 Subject: [PATCH 5/6] atf_fs_mk*temp: prevent memory leaks Refactor code to exit early in unpreventable scenarios and explicitly free in all scenarios when exiting the functions in the long paths. This avoids having to deal with some of the complexity around freeing temporary buffer space. Replace a malloc(3)+strcpy(3) use with strdup(3) while here to reduce complexity in `copy_contents(..)`. Signed-off-by: Enji Cooper --- atf-c/detail/fs.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/atf-c/detail/fs.c b/atf-c/detail/fs.c index 5ff7648c..2a584428 100644 --- a/atf-c/detail/fs.c +++ b/atf-c/detail/fs.c @@ -175,16 +175,12 @@ atf_error_t copy_contents(const atf_fs_path_t *p, char **buf) { atf_error_t err; - char *str; - str = (char *)malloc(atf_dynstr_length(&p->m_data) + 1); - if (str == NULL) + *buf = strdup(atf_fs_path_cstring(p)); + if (*buf == NULL) err = atf_no_memory_error(); - else { - strcpy(str, atf_dynstr_cstring(&p->m_data)); - *buf = str; + else err = atf_no_error(); - } return err; } @@ -795,10 +791,8 @@ atf_fs_mkdtemp(atf_fs_path_t *p) atf_error_t err; char *buf; - if (!check_umask(S_IRWXU, S_IRWXU)) { - err = invalid_umask_error(p, atf_fs_stat_dir_type, current_umask()); - goto out; - } + if (!check_umask(S_IRWXU, S_IRWXU)) + return invalid_umask_error(p, atf_fs_stat_dir_type, current_umask()); err = copy_contents(p, &buf); if (atf_is_error(err)) @@ -806,14 +800,13 @@ atf_fs_mkdtemp(atf_fs_path_t *p) err = do_mkdtemp(buf); if (atf_is_error(err)) - goto out_buf; + goto out; replace_contents(p, buf); INV(!atf_is_error(err)); -out_buf: - free(buf); out: + free(buf); return err; } @@ -824,10 +817,8 @@ atf_fs_mkstemp(atf_fs_path_t *p, int *fdout) char *buf; int fd; - if (!check_umask(S_IRWXU, S_IRWXU)) { - err = invalid_umask_error(p, atf_fs_stat_reg_type, current_umask()); - goto out; - } + if (!check_umask(S_IRWXU, S_IRWXU)) + return invalid_umask_error(p, atf_fs_stat_reg_type, current_umask()); err = copy_contents(p, &buf); if (atf_is_error(err)) @@ -835,15 +826,14 @@ atf_fs_mkstemp(atf_fs_path_t *p, int *fdout) err = do_mkstemp(buf, &fd); if (atf_is_error(err)) - goto out_buf; + goto out; replace_contents(p, buf); *fdout = fd; INV(!atf_is_error(err)); -out_buf: - free(buf); out: + free(buf); return err; } From bdbc70d809fa9c32b254d056e4df2ea819721cf7 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Tue, 10 Dec 2024 10:13:13 -0800 Subject: [PATCH 6/6] Be more defensive in do_test(..) Shuffle around assertions with `nlines` to the appropriate locations and initialize `lines[]` to an array of `NULL` pointers to avoid reading unintialized memory. Signed-off-by: Enji Cooper --- atf-c/detail/sanity_test.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/atf-c/detail/sanity_test.c b/atf-c/detail/sanity_test.c index a986c32a..7d4e3905 100644 --- a/atf-c/detail/sanity_test.c +++ b/atf-c/detail/sanity_test.c @@ -85,6 +85,8 @@ do_test_child(void *v) exit(EXIT_SUCCESS); } +#define MAX_LINES 3 + static void do_test(enum type t, bool cond) @@ -92,7 +94,7 @@ do_test(enum type t, bool cond) atf_process_child_t child; atf_process_status_t status; int nlines; - char *lines[3]; + char *lines[MAX_LINES] = { 0 }; { atf_process_stream_t outsb, errsb; @@ -106,16 +108,17 @@ do_test(enum type t, bool cond) } nlines = 0; - while (nlines < 3 && (lines[nlines] = + while (nlines < MAX_LINES && (lines[nlines] = atf_utils_readline(atf_process_child_stderr(&child))) != NULL) nlines++; - ATF_REQUIRE(nlines == 0 || nlines == 3); RE(atf_process_child_wait(&child, &status)); if (!cond) { + ATF_REQUIRE(nlines == MAX_LINES); ATF_REQUIRE(atf_process_status_signaled(&status)); ATF_REQUIRE(atf_process_status_termsig(&status) == SIGABRT); } else { + ATF_REQUIRE(nlines == 0); ATF_REQUIRE(atf_process_status_exited(&status)); ATF_REQUIRE(atf_process_status_exitstatus(&status) == EXIT_SUCCESS); }