From c082ae82ae42bd8c655d7fefb196285584c76892 Mon Sep 17 00:00:00 2001 From: Khairul Azhar Kasmiran Date: Sat, 18 May 2024 19:10:58 +0800 Subject: [PATCH] Fix bin_elf import & symbol leak (#4490) * Fix bin_elf import & symbol leak * Allow bin plugins to set custom relocs free --- librz/bin/bin.c | 12 ++---------- librz/bin/bobj.c | 7 +++++-- librz/bin/format/le/le.c | 7 +++++-- librz/bin/format/le/le.h | 1 + librz/bin/format/ne/ne.c | 2 +- librz/bin/p/bin_bflt.c | 2 +- librz/bin/p/bin_elf.inc | 2 +- librz/bin/p/bin_mz.c | 2 +- librz/bin/p/bin_qnx.c | 2 +- librz/include/rz_bin.h | 1 + 10 files changed, 19 insertions(+), 19 deletions(-) diff --git a/librz/bin/bin.c b/librz/bin/bin.c index 4db820647d..2e19540ff3 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 e4b7b33950..6dab41479e 100644 --- a/librz/bin/bobj.c +++ b/librz/bin/bobj.c @@ -115,6 +115,7 @@ RZ_API RzBinRelocStorage *rz_bin_reloc_storage_new(RZ_OWN RzPVector /*relocs_free = relocs->v.free_user; relocs->v.free = NULL; // ownership of relocs transferred rz_pvector_free(relocs); rz_pvector_sort(&sorter, reloc_cmp, NULL); @@ -132,8 +133,10 @@ RZ_API void rz_bin_reloc_storage_free(RzBinRelocStorage *storage) { if (!storage) { return; } - for (size_t i = 0; i < storage->relocs_count; i++) { - rz_bin_reloc_free(storage->relocs[i]); + if (storage->relocs_free) { + for (size_t i = 0; i < storage->relocs_count; i++) { + storage->relocs_free(storage->relocs[i]); + } } free(storage->relocs); free(storage->target_relocs); diff --git a/librz/bin/format/le/le.c b/librz/bin/format/le/le.c index 70177ec301..86be372d16 100644 --- a/librz/bin/format/le/le.c +++ b/librz/bin/format/le/le.c @@ -1307,6 +1307,7 @@ static bool le_append_fixup(rz_bin_le_obj_t *bin, LE_reloc *reloc, RzList /*le_fixups, tmp); return true; } @@ -1491,7 +1492,7 @@ static bool le_load_fixup_record(rz_bin_le_obj_t *bin, RzList /**/ } static RZ_OWN RzList /**/ *le_load_relocs(rz_bin_le_obj_t *bin) { - RzList *relocs = rz_list_newf((RzListFree)rz_bin_reloc_free); + RzList *relocs = rz_list_newf(NULL); if (!relocs) { return NULL; } @@ -1525,6 +1526,7 @@ static void rz_bin_le_free(rz_bin_le_obj_t *bin) { rz_pvector_free(bin->imports); ht_pp_free(bin->le_import_ht); rz_list_free(bin->le_relocs); + rz_list_free(bin->le_fixups); free(bin); } @@ -1571,6 +1573,7 @@ bool rz_bin_le_load_buffer(RzBinFile *bf, RzBinObject *obj, RzBuffer *buf, Sdb * CHECK(bin->imp_mod_names = le_load_import_mod_names(bin)); CHECK(bin->le_entries = le_load_entries(bin)); err_ctx = ", unable to load and apply relocations."; + CHECK(bin->le_fixups = rz_list_newf(free)); CHECK(bin->le_relocs = le_load_relocs(bin)); CHECK(le_patch_relocs(bin)); @@ -1762,7 +1765,7 @@ RZ_OWN RzPVector /**/ *rz_bin_le_get_virtual_files(RzBinFile RZ_OWN RzPVector /**/ *rz_bin_le_get_relocs(RzBinFile *bf) { rz_bin_le_obj_t *bin = bf->o->bin_obj; RzList /**/ *le_relocs = bin->le_relocs; - RzPVector /**/ *relocs = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free); + RzPVector /**/ *relocs = rz_pvector_new(free); RzBinReloc *reloc = NULL; if (!relocs) { fail_cleanup: diff --git a/librz/bin/format/le/le.h b/librz/bin/format/le/le.h index b0ca3724c4..d79c7d768b 100644 --- a/librz/bin/format/le/le.h +++ b/librz/bin/format/le/le.h @@ -96,6 +96,7 @@ typedef struct rz_bin_le_obj_s { RzPVector /**/ *imports; HtPP /**/ *le_import_ht; RzList /**/ *le_relocs; + RzList /**/ *le_fixups; ut32 reloc_target_map_base; ut32 reloc_targets_count; } rz_bin_le_obj_t; diff --git a/librz/bin/format/ne/ne.c b/librz/bin/format/ne/ne.c index e86146db29..f3994ae0fa 100644 --- a/librz/bin/format/ne/ne.c +++ b/librz/bin/format/ne/ne.c @@ -495,7 +495,7 @@ RzPVector /**/ *rz_bin_ne_get_relocs(rz_bin_ne_obj_t *bin) { } rz_buf_read_at(bin->buf, (ut64)bin->ne_header->ModRefTable + bin->header_offset, (ut8 *)modref, bin->ne_header->ModRefs * sizeof(ut16)); - RzPVector *relocs = rz_pvector_new(free); + RzPVector *relocs = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free); if (!relocs) { free(modref); return NULL; diff --git a/librz/bin/p/bin_bflt.c b/librz/bin/p/bin_bflt.c index 23196c15c6..30ff88e6da 100644 --- a/librz/bin/p/bin_bflt.c +++ b/librz/bin/p/bin_bflt.c @@ -202,7 +202,7 @@ static void convert_relocs(RzBfltObj *bin, RzPVector /**/ *out, Rz static RzPVector /**/ *relocs(RzBinFile *bf) { RzBfltObj *obj = (RzBfltObj *)bf->o->bin_obj; - RzPVector *vec = rz_pvector_new((RzPVectorFree)free); + RzPVector *vec = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free); if (!vec || !obj) { rz_pvector_free(vec); return NULL; diff --git a/librz/bin/p/bin_elf.inc b/librz/bin/p/bin_elf.inc index 6cd4e1fafc..899fd8db49 100644 --- a/librz/bin/p/bin_elf.inc +++ b/librz/bin/p/bin_elf.inc @@ -1858,7 +1858,7 @@ static RzPVector /**/ *relocs(RzBinFile *bf) { patch_relocs(bf, bin); - if (!(ret = rz_pvector_new((RzPVectorFree)free))) { + if (!(ret = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free))) { return NULL; } diff --git a/librz/bin/p/bin_mz.c b/librz/bin/p/bin_mz.c index d59d4ca24b..bc47d8cae4 100644 --- a/librz/bin/p/bin_mz.c +++ b/librz/bin/p/bin_mz.c @@ -233,7 +233,7 @@ static RzPVector /**/ *relocs(RzBinFile *bf) { if (!bf || !bf->o || !bf->o->bin_obj) { return NULL; } - if (!(ret = rz_pvector_new(free))) { + if (!(ret = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free))) { return NULL; } if (!(relocs = rz_bin_mz_get_relocs(bf->o->bin_obj))) { diff --git a/librz/bin/p/bin_qnx.c b/librz/bin/p/bin_qnx.c index 8802ac57b9..ab45699c7a 100644 --- a/librz/bin/p/bin_qnx.c +++ b/librz/bin/p/bin_qnx.c @@ -249,7 +249,7 @@ static RzPVector /**/ *relocs(RzBinFile *bf) { QnxObj *qo = bf->o->bin_obj; RzBinReloc *reloc = NULL; RzListIter *it = NULL; - RzPVector *relocs = rz_pvector_new(free); + RzPVector *relocs = rz_pvector_new((RzPVectorFree)rz_bin_reloc_free); if (!relocs) { return NULL; } diff --git a/librz/include/rz_bin.h b/librz/include/rz_bin.h index a43d5d7efc..f46419e038 100644 --- a/librz/include/rz_bin.h +++ b/librz/include/rz_bin.h @@ -680,6 +680,7 @@ RZ_API ut64 rz_bin_reloc_size(RzBinReloc *reloc); struct rz_bin_reloc_storage_t { RzBinReloc **relocs; ///< all relocs, ordered by their vaddr size_t relocs_count; + RzPVectorFree relocs_free; 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; }; // RzBinRelocStorage