From 8eb420263537cc8b9f2d0e340482f8f3de2414bf Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Sat, 18 Jul 2020 00:08:58 -0500 Subject: [PATCH] Security: memory leaks in execute functions. Use f_array_length, not f_string_length, for traversing an array. The variable "i" is being used when it should be "j". Add numerous missing deallocation calls. Deallocate immediately instead of after a waitpid(). Avoid redeclaring the "i" variable. --- level_2/fll_execute/c/execute.c | 62 ++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/level_2/fll_execute/c/execute.c b/level_2/fll_execute/c/execute.c index 496682c..7f8a846 100644 --- a/level_2/fll_execute/c/execute.c +++ b/level_2/fll_execute/c/execute.c @@ -481,14 +481,14 @@ extern "C" { fixed_arguments[0] = program_name; f_status status = F_none; - f_string_length i = 0; + f_array_length i = 0; for (; i < arguments.used; i++) { f_macro_string_new(status, fixed_arguments[i + 1], arguments.array[i].used + 1); if (F_status_is_error(status)) { - for (f_string_length j = 0; j < i; j++) { - f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[j].used + 1); + for (f_array_length j = 0; j < i; j++) { + f_macro_string_delete_simple(fixed_arguments[j + 1], arguments.array[j].used + 1); } // for return status; @@ -506,7 +506,7 @@ extern "C" { process_id = fork(); if (process_id < 0) { - for (f_string_length i = 0; i < arguments.used; i++) { + for (i = 0; i < arguments.used; i++) { f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); } // for @@ -517,17 +517,21 @@ extern "C" { if (process_id == 0) { execvp(program_name, fixed_arguments); + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + // according to manpages, calling _exit() is safer and should be called here instead of exit() _exit(-1); } - // have the parent wait for the child process to finish - waitpid(process_id, result, 0); - - for (f_string_length i = 0; i < arguments.used; i++) { + for (i = 0; i < arguments.used; i++) { f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); } // for + // have the parent wait for the child process to finish + waitpid(process_id, result, 0); + if (result != 0 && *result != 0) return F_status_set_error(F_failure); return F_none; @@ -551,14 +555,14 @@ extern "C" { fixed_arguments[0] = program_name; f_status status = F_none; - f_string_length i = 0; + f_array_length i = 0; for (; i < arguments.used; i++) { f_macro_string_new(status, fixed_arguments[i + 1], arguments.array[i].used + 1); if (F_status_is_error(status)) { - for (f_string_length j = 0; j < i; j++) { - f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[j].used + 1); + for (f_array_length j = 0; j < i; j++) { + f_macro_string_delete_simple(fixed_arguments[j + 1], arguments.array[j].used + 1); } // for return status; @@ -587,6 +591,10 @@ extern "C" { } if (F_status_is_error(status)) { + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + f_macro_string_dynamic_delete_simple(path); f_macro_string_dynamics_delete_simple(paths); return status; @@ -594,6 +602,10 @@ extern "C" { f_macro_string_dynamic_delete(status, path); if (F_status_is_error(status)) { + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + f_macro_string_dynamics_delete_simple(paths); return status; } @@ -633,12 +645,20 @@ extern "C" { } if (F_status_is_error(status)) { + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + f_macro_string_dynamics_delete_simple(paths); return status; } } // for if (found == 0) { + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + f_macro_string_dynamics_delete_simple(paths); return F_status_set_error(F_file_found_not); } @@ -649,6 +669,10 @@ extern "C" { f_macro_string_dynamics_delete(status, paths); if (F_status_is_error(status)) { + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + return status; } @@ -657,7 +681,7 @@ extern "C" { process_id = fork(); if (process_id < 0) { - for (f_string_length i = 0; i < arguments.used; i++) { + for (i = 0; i < arguments.used; i++) { f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); } // for @@ -668,23 +692,27 @@ extern "C" { if (process_id == 0) { clearenv(); - for (f_array_length i = 0; i < names.used; i++) { + for (i = 0; i < names.used; i++) { f_environment_set_dynamic(names.array[i], values.array[i], F_true); } execvp(program_path, fixed_arguments); + for (i = 0; i < arguments.used; i++) { + f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); + } // for + // according to manpages, calling _exit() is safer and should be called here instead of exit() _exit(-1); } - // have the parent wait for the child process to finish - waitpid(process_id, result, 0); - - for (f_string_length i = 0; i < arguments.used; i++) { + for (i = 0; i < arguments.used; i++) { f_macro_string_delete_simple(fixed_arguments[i + 1], arguments.array[i].used + 1); } // for + // have the parent wait for the child process to finish + waitpid(process_id, result, 0); + if (result != 0 && *result != 0) return F_status_set_error(F_failure); return F_none; -- 1.8.3.1