From cfa8107ab99f932752d7e1b15d28b61beba01185 Mon Sep 17 00:00:00 2001 From: "Markus F.X.J. Oberhumer" Date: Wed, 30 Aug 2023 16:41:59 +0200 Subject: [PATCH] src: make sort order deterministic, next try We cannot compare pointers as they may point to qsort-local objects. And we must make sure that cmp(a,b) always agrees with cmp(b,a). --- src/linker.cpp | 1 + src/linker.h | 1 + src/p_mach.cpp | 19 +++++++++++++++++-- src/p_vmlinx.cpp | 12 +++++++++++- src/pefile.cpp | 45 ++++++++++++++++++++++++--------------------- 5 files changed, 54 insertions(+), 24 deletions(-) diff --git a/src/linker.cpp b/src/linker.cpp index 2ab3c24e..580947dc 100644 --- a/src/linker.cpp +++ b/src/linker.cpp @@ -329,6 +329,7 @@ ElfLinker::Section *ElfLinker::addSection(const char *sname, const void *sdata, assert(sname[strlen(sname) - 1] != ':'); assert(findSection(sname, false) == nullptr); Section *sec = new Section(sname, sdata, slen, p2align); + sec->sort_id = nsections; sections[nsections++] = sec; return sec; } diff --git a/src/linker.h b/src/linker.h index 01d3f531..a412ab3f 100644 --- a/src/linker.h +++ b/src/linker.h @@ -119,6 +119,7 @@ struct ElfLinker::Section : private noncopyable { void *input = nullptr; byte *output = nullptr; unsigned size = 0; + unsigned sort_id = 0; // for qsort() upx_uint64_t offset = 0; unsigned p2align = 0; // log2 Section *next = nullptr; diff --git a/src/p_mach.cpp b/src/p_mach.cpp index b0118401..2ebac2dd 100644 --- a/src/p_mach.cpp +++ b/src/p_mach.cpp @@ -562,7 +562,8 @@ PackMachBase::compare_segment_command(void const *const aa, void const *const unsigned const xb = b->cmd - lc_seg; if (xa < xb) return -1; // LC_SEGMENT first if (xa > xb) return 1; - if (0 != xa) return 0; // not LC_SEGMENT +// mfx @jreiser: we do not want to return 0; please check +//// if (0 != xa) return 0; // not LC_SEGMENT // Ascending by .fileoff so that find_SEGMENT_gap works if (a->fileoff < b->fileoff) return -1; @@ -572,10 +573,24 @@ PackMachBase::compare_segment_command(void const *const aa, void const *const if (a->vmaddr < b->vmaddr) return -1; if (a->vmaddr > b->vmaddr) return 1; // Descending by .vmsize +// mfx @jreiser: I think this may violate cmp(a,b) == -cmp(b,a); please check +// if ((a->vmsize != 0) != (b->vmsize != 0)) +// return (a->vmsize != 0) ? -1 : 1; if (a->vmsize) return -1; // 'a' is first if (b->vmsize) return 1; // 'a' is last // What could remain? - return aa < bb ? -1 : 1; // make sort order deterministic/stable +#define CMP(field) \ + if (a->field != b->field) return a->field < b->field ? -1 : 1 + // try to make sort order deterministic and just compare more fields + CMP(vmsize); + CMP(cmdsize); + CMP(filesize); + CMP(maxprot); + CMP(initprot); + CMP(nsects); + CMP(flags); +#undef CMP + return 0; } // At 2013-02-03 part of the source for codesign was diff --git a/src/p_vmlinx.cpp b/src/p_vmlinx.cpp index fab8def2..be5701a2 100644 --- a/src/p_vmlinx.cpp +++ b/src/p_vmlinx.cpp @@ -99,7 +99,17 @@ PackVmlinuxBase::compare_Phdr(void const *aa, void const *bb) if (a->p_paddr < b->p_paddr) return -1; // ascending by .p_paddr if (a->p_paddr > b->p_paddr) return 1; // What could remain? - return aa < bb ? -1 : 1; // make sort order deterministic/stable + // try to make sort order deterministic and just compare more fields +#define CMP(field) \ + if (a->field != b->field) return a->field < b->field ? -1 : 1 + CMP(p_offset); + CMP(p_vaddr); + CMP(p_filesz); + CMP(p_memsz); + CMP(p_flags); + CMP(p_align); +#undef CMP + return 0; } template diff --git a/src/pefile.cpp b/src/pefile.cpp index 20650891..e624db6e 100644 --- a/src/pefile.cpp +++ b/src/pefile.cpp @@ -697,14 +697,15 @@ class PeFile::ImportLinker final : public ElfLinkerAMD64 { infoWarning("empty import: %s", dll); } - static int __acc_cdecl_qsort compare(const void *p1, const void *p2) { - const Section *s1 = *(const Section *const *) p1; - const Section *s2 = *(const Section *const *) p2; - int rc = strcmp(s1->name, s2->name); + static int __acc_cdecl_qsort compare(const void *aa, const void *bb) { + const Section *a = *(const Section *const *) aa; + const Section *b = *(const Section *const *) bb; + int rc = strcmp(a->name, b->name); if (rc != 0) return rc; // What could remain? - return p1 < p2 ? -1 : 1; // make sort order deterministic/stable + assert_noexcept(a->sort_id != b->sort_id); + return a->sort_id < b->sort_id ? -1 : 1; // make sort order deterministic } virtual void alignCode(unsigned len) override { alignWithByte(len, 0); } @@ -873,29 +874,31 @@ unsigned PeFile::processImports0(ord_mask_t ord_mask) // pass 1 unsigned original_position; bool isk32; - static int __acc_cdecl_qsort compare(const void *p1, const void *p2) { - const udll *u1 = *(const udll *const *) p1; - const udll *u2 = *(const udll *const *) p2; - if (u1->isk32 != u2->isk32) - return u1->isk32 ? -1 : 1; - if ((*u1->lookupt != 0) != (*u2->lookupt != 0)) - return (*u1->lookupt != 0) ? -1 : 1; - int rc = strcasecmp(u1->name, u2->name); + static int __acc_cdecl_qsort compare(const void *aa, const void *bb) { + const udll *a = *(const udll *const *) aa; + const udll *b = *(const udll *const *) bb; + if (a->isk32 != b->isk32) + return a->isk32 ? -1 : 1; + if ((*a->lookupt != 0) != (*b->lookupt != 0)) + return (*a->lookupt != 0) ? -1 : 1; + int rc = strcasecmp(a->name, b->name); if (rc != 0) return rc; - if ((u1->ordinal != 0) != (u2->ordinal != 0)) - return (u1->ordinal != 0) ? -1 : 1; - if (u1->shname && u2->shname) { - rc = (int) (upx_safe_strlen(u1->shname) - upx_safe_strlen(u2->shname)); + if ((a->ordinal != 0) != (b->ordinal != 0)) + return (a->ordinal != 0) ? -1 : 1; + if (a->shname && b->shname) { + rc = (int) (upx_safe_strlen(a->shname) - upx_safe_strlen(b->shname)); if (rc != 0) return rc; - rc = strcmp(u1->shname, u2->shname); + rc = strcmp(a->shname, b->shname); if (rc != 0) return rc; - } else if ((u1->shname != nullptr) != (u2->shname != nullptr)) - return (u1->shname != nullptr) ? -1 : 1; + } else if ((a->shname != nullptr) != (b->shname != nullptr)) + return (a->shname != nullptr) ? -1 : 1; // What could remain? - return p1 < p2 ? -1 : 1; // make sort order deterministic/stable + // make sort order deterministic + assert_noexcept(a->original_position != b->original_position); + return a->original_position < b->original_position ? -1 : 1; } };