From 8ace435328717c0dc8caf92d7bbd6ed6ffd6ad6a Mon Sep 17 00:00:00 2001 From: Giovanni <561184+wargio@users.noreply.github.com> Date: Sat, 11 Sep 2021 09:30:57 +0200 Subject: [PATCH] Fix coverity bugs (#1654) * CID 316614: Use after free (USE_AFTER_FREE) * CID 316535: Use after free (USE_AFTER_FREE) * CID 316652: Use after free (USE_AFTER_FREE) * CID 316504: Unsigned compared against 0 (NO_EFFECT) * CID 316439: Resource leak (RESOURCE_LEAK) * CID 356127: Dereference after null check (name can never be NULL) * CID 320881: Resource leak (RESOURCE_LEAK) * CID 320885: Missing break in switch (MISSING_BREAK) * CID 316679: Resource leak (RESOURCE_LEAK) * CID 316574: Explicit null dereferenced (FORWARD_NULL) * CID 356105: Resource leak (RESOURCE_LEAK) * Fixed uninitialized building issue --- binrz/rz-test/rz-test.c | 5 ++++- librz/asm/asm.c | 1 + librz/bin/format/java/class_bin.c | 2 +- librz/core/disasm.c | 2 +- librz/main/rz-hash.c | 3 +++ librz/main/rz-run.c | 1 + librz/main/rz-sign.c | 2 +- librz/util/strpool.c | 4 ++-- librz/util/subprocess.c | 21 +++++++++++++++++---- 9 files changed, 31 insertions(+), 10 deletions(-) diff --git a/binrz/rz-test/rz-test.c b/binrz/rz-test/rz-test.c index c25385f445..1b26ddc495 100644 --- a/binrz/rz-test/rz-test.c +++ b/binrz/rz-test/rz-test.c @@ -344,6 +344,7 @@ int rz_test_main(int argc, const char **argv) { int i; for (i = opt.ind; i < argc; i++) { const char *arg = argv[i]; + char *alloc_arg = NULL; if (*arg == '@') { arg++; eprintf("Category: %s\n", arg); @@ -370,7 +371,7 @@ int rz_test_main(int argc, const char **argv) { } else if (!strcmp(arg, "cmds")) { arg = "db"; } else { - arg = rz_str_newf("db/%s", arg + 1); + arg = alloc_arg = rz_str_newf("db/%s", arg + 1); } } char *tf = rz_file_abspath_rel(cwd, arg); @@ -378,9 +379,11 @@ int rz_test_main(int argc, const char **argv) { eprintf("Failed to load tests from \"%s\"\n", tf); rz_test_test_database_free(state.db); free(tf); + free(alloc_arg); ret = -1; goto beach; } + RZ_FREE(alloc_arg); free(tf); } } else { diff --git a/librz/asm/asm.c b/librz/asm/asm.c index 9a304d4e2d..f0d7478558 100644 --- a/librz/asm/asm.c +++ b/librz/asm/asm.c @@ -1019,6 +1019,7 @@ RZ_API RzAsmCode *rz_asm_massemble(RzAsm *a, const char *assembly) { if (op.buf_inc && rz_buf_size(op.buf_inc) > 1) { char *inc = rz_buf_to_string(op.buf_inc); rz_buf_free(op.buf_inc); + op.buf_inc = NULL; if (inc) { ret += rz_hex_str2bin(inc, acode->bytes + idx + ret); free(inc); diff --git a/librz/bin/format/java/class_bin.c b/librz/bin/format/java/class_bin.c index 49a556a11e..cd6182ddfa 100644 --- a/librz/bin/format/java/class_bin.c +++ b/librz/bin/format/java/class_bin.c @@ -1040,7 +1040,7 @@ RZ_API RZ_OWN RzList *rz_bin_java_class_strings(RZ_NONNULL RzBinJavaClass *bin) } static char *add_class_name_to_name(char *name, char *classname) { - if (classname && name) { + if (classname) { return rz_str_newf("%s.%s", classname, name); } return strdup(name); diff --git a/librz/core/disasm.c b/librz/core/disasm.c index 9c2b009b05..0b0356b2dd 100644 --- a/librz/core/disasm.c +++ b/librz/core/disasm.c @@ -5804,7 +5804,7 @@ toro: if (ds->immtrim) { free(ds->opstr); ds->opstr = strdup(rz_asm_op_get_asm(&ds->asmop)); - rz_parse_immtrim(ds->opstr); + ds->opstr = rz_parse_immtrim(ds->opstr); } } if (ds->asm_instr) { diff --git a/librz/main/rz-hash.c b/librz/main/rz-hash.c index b4391742e8..7c4ffb7a08 100644 --- a/librz/main/rz-hash.c +++ b/librz/main/rz-hash.c @@ -260,9 +260,11 @@ static bool rz_hash_parse_hexadecimal(const char *option, const char *hexadecima if (hexlen < 1 || !hexadecimal) { RZ_LOG_ERROR("rz-hash: error, option %s is empty.\n", option); + free(sstdin); return false; } else if (hexlen & 1) { RZ_LOG_ERROR("rz-hash: error, option %s is not a valid hexadecimal (len is not pair: %d).\n", option, hexlen); + free(sstdin); return false; } *buffer = NULL; @@ -1206,6 +1208,7 @@ RZ_API int rz_main_rz_hash(int argc, const char **argv) { goto rz_main_rz_hash_end; case RZ_HASH_OP_HELP: result = 0; + /* fall-thru */ default: rz_hash_show_help(false); goto rz_main_rz_hash_end; diff --git a/librz/main/rz-run.c b/librz/main/rz-run.c index bb0aecf6e2..e0c5f6349b 100644 --- a/librz/main/rz-run.c +++ b/librz/main/rz-run.c @@ -73,6 +73,7 @@ RZ_API int rz_main_rz_run(int argc, const char **argv) { ret = rz_run_config_env(p); if (ret) { printf("error while configuring the environment.\n"); + rz_run_free(p); return 1; } ret = rz_run_start(p); diff --git a/librz/main/rz-sign.c b/librz/main/rz-sign.c index 67f9774d16..ea2b5a426c 100644 --- a/librz/main/rz-sign.c +++ b/librz/main/rz-sign.c @@ -53,7 +53,7 @@ static void find_functions(RzCore *core, size_t count) { switch (count) { case 0: cmd = "aa"; break; case 1: cmd = "aaa"; break; - case 2: cmd = "aaaa"; break; + default: cmd = "aaaa"; break; } rz_core_cmd0(core, cmd); } diff --git a/librz/util/strpool.c b/librz/util/strpool.c index f0ea4e44ee..34244bc951 100644 --- a/librz/util/strpool.c +++ b/librz/util/strpool.c @@ -48,7 +48,7 @@ RZ_API char *rz_strpool_alloc(RzStrpool *p, int l) { ret = realloc(p->str, p->size); if (!ret) { eprintf("Realloc failed!\n"); - free(p->str); + RZ_FREE(p->str); return NULL; } p->str = ret; @@ -92,7 +92,7 @@ RZ_API int rz_strpool_fit(RzStrpool *p) { s = realloc(p->str, p->len); if (!s) { eprintf("Realloc failed!\n"); - free(p->str); + RZ_FREE(p->str); return false; } p->str = s; diff --git a/librz/util/subprocess.c b/librz/util/subprocess.c index 5f619967d9..31ecac5f41 100644 --- a/librz/util/subprocess.c +++ b/librz/util/subprocess.c @@ -630,6 +630,9 @@ static char **create_child_env(const char *envvars[], const char *envvals[], siz char **ep; size_t new_env_size = env_size, size = 0; size_t *positions = RZ_NEWS(size_t, env_size); + if (!positions) { + return NULL; + } for (size_t i = 0; i < env_size; i++) { positions[i] = SIZE_MAX; } @@ -652,6 +655,10 @@ static char **create_child_env(const char *envvars[], const char *envvals[], siz } char **new_env = RZ_NEWS(char *, size + new_env_size + 1); + if (!new_env) { + free(positions); + return NULL; + } for (size_t i = 0; i < size; i++) { new_env[i] = strdup(environ[i]); } @@ -676,6 +683,9 @@ static char **create_child_env(const char *envvars[], const char *envvals[], siz } static void destroy_child_env(char **child_env) { + if (!child_env) { + return; + } char **ep; for (ep = child_env; *ep; ep++) { free(*ep); @@ -684,6 +694,8 @@ static void destroy_child_env(char **child_env) { } RZ_API RzSubprocess *rz_subprocess_start_opt(RzSubprocessOpt *opt) { + RzSubprocess *proc = NULL; + char **child_env = NULL; char **argv = calloc(opt->args_size + 2, sizeof(char *)); if (!argv) { return NULL; @@ -694,7 +706,7 @@ RZ_API RzSubprocess *rz_subprocess_start_opt(RzSubprocessOpt *opt) { } // done by calloc: argv[args_size + 1] = NULL; subprocess_lock(); - RzSubprocess *proc = RZ_NEW0(RzSubprocess); + proc = RZ_NEW0(RzSubprocess); if (!proc) { goto error; } @@ -756,7 +768,7 @@ RZ_API RzSubprocess *rz_subprocess_start_opt(RzSubprocessOpt *opt) { // Let's create the environment for the child in the parent, with malloc, // because we can't use functions that lock after fork - char **child_env = create_child_env(opt->envvars, opt->envvals, opt->env_size); + child_env = create_child_env(opt->envvars, opt->envvals, opt->env_size); proc->pid = rz_sys_fork(); if (proc->pid == -1) { @@ -838,6 +850,7 @@ error: if (stdin_pipe[1] != -1) { rz_sys_pipe_close(stdin_pipe[1]); } + destroy_child_env(child_env); subprocess_unlock(); return NULL; } @@ -934,14 +947,14 @@ static RzSubprocessWaitReason subprocess_wait(RzSubprocess *proc, ut64 timeout_m if (stdout_enabled && FD_ISSET(proc->stdout_fd, &rfds)) { timedout = false; size_t r = read_to_strbuf(&proc->out, proc->stdout_fd, &stdout_eof, n_bytes); - if (r >= 0 && n_bytes) { + if (r > 0 && n_bytes) { n_bytes -= r; } } if (stderr_enabled && FD_ISSET(proc->stderr_fd, &rfds)) { timedout = false; size_t r = read_to_strbuf(&proc->err, proc->stderr_fd, &stderr_eof, n_bytes); - if (r >= 0 && n_bytes) { + if (r > 0 && n_bytes) { n_bytes -= r; } }