Fix memory leaks in PE format (#4727)

* Fix freeing of relocation array when no relocations are present

* Reimplement canary checking for PE for speed + getting rid of memory leak when calling imports()

librz/bin/format/pe/pe.h

@@ -191,6 +191,7 @@ struct PE_(rz_bin_pe_obj_t) {
 	Sdb *kv;
 	RzCMS *cms;
 	RzSpcIndirectDataContent *spcinfo;
+	bool has_canary;
 	char *authentihash;
 	bool is_authhash_valid;
 	bool is_signed;
@@ -244,6 +245,7 @@ int PE_(rz_bin_pe_is_stripped_local_syms)(RzBinPEObj *bin);
 int PE_(rz_bin_pe_is_stripped_debug)(RzBinPEObj *bin);
 int PE_(bin_pe_get_claimed_checksum)(RzBinPEObj *bin);
 int PE_(bin_pe_get_actual_checksum)(RzBinPEObj *bin);
+bool PE_(rz_bin_pe_has_canary)(const RzBinPEObj *bin);
 struct rz_bin_pe_addr_t *PE_(check_unknow)(RzBinPEObj *bin);
 struct rz_bin_pe_addr_t *PE_(check_msvcseh)(RzBinPEObj *bin);
 struct rz_bin_pe_addr_t *PE_(check_mingw)(RzBinPEObj *bin);

librz/bin/format/pe/pe_info.c

@@ -21,6 +21,10 @@ static inline int is_arm(RzBinPEObj *bin) {
 	return 0;
 }
 
+bool PE_(rz_bin_pe_has_canary)(const RzBinPEObj *bin) {
+	return bin->has_canary;
+}
+
 // TODO: make it const! like in elf
 char *PE_(rz_bin_pe_get_machine)(RzBinPEObj *bin) {
 	char *machine = NULL;

librz/bin/format/pe/pe_relocs.c

@@ -61,7 +61,7 @@ int PE_(bin_pe_init_relocs)(RZ_NONNULL RzBinPEObj *bin) {
 	}
 
 	if (PE_(rz_bin_pe_is_stripped_relocs)(bin) || !get_relocs_from_data_dir(bin, ret)) {
-		rz_vector_free(bin->relocs);
+		rz_vector_free(ret);
 		bin->relocs = NULL;
 		return false;
 	}

librz/bin/p/bin_pe.inc

@@ -429,14 +429,17 @@ static RzPVector /*<RzBinImport *>*/ *imports(RzBinFile *bf) {
 	if (!bf || !bf->o || !bf->o->bin_obj) {
 		return NULL;
 	}
+	RzBinPEObj *bin = (RzBinPEObj *)bf->o->bin_obj;
+	bin->has_canary = false;
 	RzPVector *ret = rz_pvector_new((RzListFree)rz_bin_import_free);
 	if (!ret) {
 		return NULL;
 	}
 
-	if (!(imports = PE_(rz_bin_pe_get_imports)(bf->o->bin_obj))) {
+	if (!(imports = PE_(rz_bin_pe_get_imports)(bin))) {
 		return ret;
 	}
+	bool has_canary = false;
 	for (i = 0; !imports[i].last; i++) {
 		if (!(ptr = RZ_NEW0(RzBinImport))) {
 			break;
@@ -451,7 +454,15 @@ static RzPVector /*<RzBinImport *>*/ *imports(RzBinFile *bf) {
 		// index for the import. There is no point in exposing it.
 		// ptr->hint = imports[i].hint;
 		rz_pvector_push(ret, ptr);
+
+		// __security_init_cookie is a function imported from msvcrt.dll (libc) that when called
+		// initiliazes the stack canary. So if the function is imported, we can
+		// conclude the binary uses stack canary.
+		if (!has_canary && !strcmp(ptr->name, "__security_init_cookie")) {
+			has_canary = true;
+		}
 	}
+	bin->has_canary = has_canary;
 	free(imports);
 	return ret;
 }
@@ -637,24 +648,6 @@ err:
 	return NULL;
 }
 
-static int has_canary(RzBinFile *bf) {
-	void **it;
-	const RzPVector *imports_vec = imports(bf);
-	RzBinImport *imp;
-	if (imports_vec) {
-		rz_pvector_foreach (imports_vec, it) {
-			imp = *it;
-			// __security_init_cookie is a function imported from msvcrt.dll (libc) that when called
-			// initiliazes the stack canary. So if the function is imported, we can
-			// assume the binary uses stack canary.
-			if (!strcmp(imp->name, "__security_init_cookie")) {
-				return true;
-			}
-		}
-	}
-	return false;
-}
-
 static inline bool haschr(const struct PE_(rz_bin_pe_obj_t) * bin, ut16 dllCharacteristic) {
 	return bin->nt_headers->optional_header.DllCharacteristics & dllCharacteristic;
 }
@@ -703,15 +696,15 @@ static RzBinInfo *info(RzBinFile *bf) {
 	ret->bits = PE_(rz_bin_pe_get_bits)(bf->o->bin_obj);
 	ret->big_endian = PE_(rz_bin_pe_is_big_endian)(bf->o->bin_obj);
 	ret->dbg_info = 0;
-	ret->has_canary = has_canary(bf);
+	ret->has_canary = PE_(rz_bin_pe_has_canary)(bf->o->bin_obj);
 	ret->has_nx = haschr(bin, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT);
 	ret->has_pi = haschr(bin, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE);
 	ret->claimed_checksum = rz_str_newf("0x%08x", claimed_checksum);
 	ret->actual_checksum = rz_str_newf("0x%08x", actual_checksum);
 	ret->pe_overlay = pe_overlay > 0;
 	ret->signature = bin ? bin->is_signed : false;
 	Sdb *db = sdb_ns(bf->sdb, "pe", true);
-	sdb_bool_set(db, "canary", has_canary(bf));
+	sdb_bool_set(db, "canary", ret->has_canary);
 	sdb_bool_set(db, "highva", haschr(bin, IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA));
 	sdb_bool_set(db, "aslr", haschr(bin, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE));
 	sdb_bool_set(db, "forceintegrity", haschr(bin, IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY));