From 3d0e7df7d1b347a1d1f3f868f0e255328c40a433 Mon Sep 17 00:00:00 2001 From: Khairul Azhar Kasmiran Date: Mon, 13 May 2024 20:26:13 +0800 Subject: [PATCH] Fix bin_elf import & symbol leak --- librz/bin/bin.c | 12 ++---------- librz/bin/bobj.c | 13 ++++++++++--- librz/bin/bobj_process_reloc.c | 2 +- librz/bin/p/bin_pe.inc | 3 ++- librz/include/rz_bin.h | 3 ++- test/integration/test_bin.c | 2 +- 6 files changed, 18 insertions(+), 17 deletions(-) diff --git a/librz/bin/bin.c b/librz/bin/bin.c index 4db820647d5..2e19540ff37 100644 --- a/librz/bin/bin.c +++ b/librz/bin/bin.c @@ -203,16 +203,8 @@ RZ_API void rz_bin_reloc_free(RZ_NULLABLE RzBinReloc *reloc) { if (!reloc) { return; } - /** - * TODO: leak in bin_elf, but it will cause double free in bin_pe if free here, - * Because in the bin_elf implementation RzBinObject->imports and RzBinObject->relocs->imports - * are two pieces of data, but they are linked to each other in bin_pe - * TODO: also, double free in bin_coff if free here, - * Because in the bin_coff implementation imports and symbols are shared - * between relocs - */ - // rz_bin_import_free(reloc->import); - // rz_bin_symbol_free(reloc->symbol); + rz_bin_import_free(reloc->import); + rz_bin_symbol_free(reloc->symbol); free(reloc); } diff --git a/librz/bin/bobj.c b/librz/bin/bobj.c index e4b7b339501..5d86fc665d4 100644 --- a/librz/bin/bobj.c +++ b/librz/bin/bobj.c @@ -95,7 +95,7 @@ static int reloc_target_cmp(const void *a, const void *b, void *user) { #undef CMP_CHECK -RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /**/ *relocs) { +RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /**/ *relocs, RzBinPlugin *plugin) { RzBinRelocStorage *ret = RZ_NEW0(RzBinRelocStorage); if (!ret) { return NULL; @@ -125,6 +125,9 @@ RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /*target_relocs_count = rz_pvector_len(&target_sorter); ret->target_relocs = (RzBinReloc **)rz_pvector_flush(&target_sorter); rz_pvector_fini(&target_sorter); + if (plugin && !strcmp(plugin->name, "coff")) { + ret->sym_imp_shared = true; + } return ret; } @@ -133,7 +136,11 @@ RZ_API void rz_bin_reloc_storage_free(RzBinRelocStorage *storage) { return; } for (size_t i = 0; i < storage->relocs_count; i++) { - rz_bin_reloc_free(storage->relocs[i]); + if (storage->sym_imp_shared) { + free(storage->relocs[i]); // Not freeing symbol and import + } else { + rz_bin_reloc_free(storage->relocs[i]); + } } free(storage->relocs); free(storage->target_relocs); @@ -554,7 +561,7 @@ RZ_API RzBinRelocStorage *rz_bin_object_patch_relocs(RzBinFile *bf, RzBinObject } rz_bin_reloc_storage_free(o->relocs); REBASE_PADDR(o, tmp, RzBinReloc); - o->relocs = rz_bin_reloc_storage_new(tmp); + o->relocs = rz_bin_reloc_storage_new(tmp, o->plugin); bf->rbin->is_reloc_patched = true; } return o->relocs; diff --git a/librz/bin/bobj_process_reloc.c b/librz/bin/bobj_process_reloc.c index 459a21b0fa6..b93e1cd574f 100644 --- a/librz/bin/bobj_process_reloc.c +++ b/librz/bin/bobj_process_reloc.c @@ -47,7 +47,7 @@ RZ_IPI void rz_bin_set_and_process_relocs(RzBinFile *bf, RzBinObject *o, const R process_handle_reloc(element, o, demangler, flags, imp_cb, sym_cb); } - o->relocs = rz_bin_reloc_storage_new(relocs); + o->relocs = rz_bin_reloc_storage_new(relocs, plugin); } RZ_IPI void rz_bin_demangle_relocs_with_flags(RzBinObject *o, const RzDemanglerPlugin *demangler, RzDemanglerFlag flags) { diff --git a/librz/bin/p/bin_pe.inc b/librz/bin/p/bin_pe.inc index d6ff371cb45..d5fb4a73917 100644 --- a/librz/bin/p/bin_pe.inc +++ b/librz/bin/p/bin_pe.inc @@ -431,7 +431,8 @@ static RzPVector /**/ *imports(RzBinFile *bf) { if (!bf || !bf->o || !bf->o->bin_obj) { return NULL; } - RzPVector *ret = rz_pvector_new((RzListFree)rz_bin_import_free); + // rz_bin core will free imports + RzPVector *ret = rz_pvector_new(NULL); if (!ret) { return NULL; } diff --git a/librz/include/rz_bin.h b/librz/include/rz_bin.h index a43d5d7efca..43a9ba8ca26 100644 --- a/librz/include/rz_bin.h +++ b/librz/include/rz_bin.h @@ -682,9 +682,10 @@ struct rz_bin_reloc_storage_t { size_t relocs_count; RzBinReloc **target_relocs; ///< all relocs that have a valid target_vaddr, ordered by their target_vaddr. size is target_relocs_count! size_t target_relocs_count; + bool sym_imp_shared; // plugin frees reloc symbols and imports }; // RzBinRelocStorage -RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /**/ *relocs); +RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /**/ *relocs, RzBinPlugin *plugin); RZ_API void rz_bin_reloc_storage_free(RzBinRelocStorage *storage); RZ_API RzBinReloc *rz_bin_reloc_storage_get_reloc_in(RzBinRelocStorage *storage, ut64 vaddr, ut64 size); diff --git a/test/integration/test_bin.c b/test/integration/test_bin.c index ed998808a4f..5c234f00ffd 100644 --- a/test/integration/test_bin.c +++ b/test/integration/test_bin.c @@ -117,7 +117,7 @@ bool test_rz_bin_reloc_storage(void) { mu_assert_notnull(rz, "reloc"); RzBinReloc *r3 = add_reloc(l, 0x1003, 0x1006, 0x200c); mu_assert_notnull(r3, "reloc"); - RzBinRelocStorage *relocs = rz_bin_reloc_storage_new(l); + RzBinRelocStorage *relocs = rz_bin_reloc_storage_new(l, NULL); RzBinReloc *r = rz_bin_reloc_storage_get_reloc_in(relocs, 0xfff, 1); mu_assert_null(r, "reloc in");