From 60de925b10576de97e939fbb2f8d6c87fca9afce Mon Sep 17 00:00:00 2001 From: Richard Dzenis Date: Mon, 6 Jan 2025 14:15:56 +0200 Subject: [PATCH] MachO::Binary::extend_section fix sorting order If there are empty sections and non-empty section at the same offset, we want to shift only empty sections. The expected behavior is that we sort sections in ascending order, and then `sections_to_shift` is trimmed, such that the last element becomes `section`. Previously, we were sorting in descending order, while keeping empty sections in the same order. Due to this we were doing trimming incorrectly. Rework macho/test_builder.py::test_extend_section The previous implementation of the test was expecting unsupported behavior from LIEF. LIEF currently does not support arbitrary calls to add_section and extend_section and keeping the layout of the binary correct. Currently it is responsibility of a user to ensure that the final layout of the binary is well-formed. This patch splits test into two cases: 1. Repeated calls to add_section followed by extend_section using different alignment values. 2. Multiple calls to add_section and then multiple calls to extend_section using the same alignment values. --- src/MachO/Binary.cpp | 27 +++++++++---------- tests/macho/test_builder.py | 54 ++++++++++++++++++++----------------- 2 files changed, 43 insertions(+), 38 deletions(-) diff --git a/src/MachO/Binary.cpp b/src/MachO/Binary.cpp index 1c7b5ce49..58618e87a 100644 --- a/src/MachO/Binary.cpp +++ b/src/MachO/Binary.cpp @@ -1259,30 +1259,29 @@ bool Binary::extend_section(Section& section, size_t size) { return false; } - // Select sections that we need to shift. + // Note: if we are extending an empty section, then there may be many zero-sized sections at + // this offset `section.offset()`, and one non-empty section. + + // Select sections that we need to shift to the left. + // Sections that come after the current one are not considered for shifting. + // Note: we compare sections by end_offset to exclude non-empty section that starts at the current offset. sections_cache_t sections_to_shift; for (Section& s : sections()) { - if (s.offset() > section.offset() || s.offset() == 0) { + if (s.offset() == 0 || s.offset() + s.size() > section.offset() + section.size()) { continue; } sections_to_shift.push_back(&s); } assert(!sections_to_shift.empty()); - // Stable-sort by end_offset, preserving original order. + // Stable-sort by offset in ascending order as well as preserving original order. std::stable_sort(sections_to_shift.begin(), sections_to_shift.end(), - [](const Section* a, const Section* b) { - return a->offset() + a->size() > b->offset() + b->size(); - }); + [](const Section* a, const Section* b) { return a->offset() < b->offset(); }); - // If we are extending an empty section, then there may be many sections at this offset, - // we do not want to shift sections which were added after the current one. - // Such that we preserve order of sections in which they were added to the binary. - if (section.size() == 0) { - auto it = std::find(sections_to_shift.begin(), sections_to_shift.end(), §ion); - assert(it != sections_to_shift.end()); - sections_to_shift.erase(std::next(it), sections_to_shift.end()); - } + // We do not want to shift empty sections that were added after the current one. + auto it = std::find(sections_to_shift.begin(), sections_to_shift.end(), §ion); + assert(it != sections_to_shift.end()); + sections_to_shift.erase(std::next(it), sections_to_shift.end()); // Find maximum alignment auto it_maxa = std::max_element(sections_to_shift.begin(), sections_to_shift.end(), diff --git a/tests/macho/test_builder.py b/tests/macho/test_builder.py index ab4b3fad0..4ffd5acd9 100644 --- a/tests/macho/test_builder.py +++ b/tests/macho/test_builder.py @@ -187,40 +187,46 @@ def test_add_section_id(tmp_path): print(stdout) assert re.search(r'uid=', stdout) is not None -def test_extend_section(tmp_path): +def test_extend_section_1(tmp_path): + """ This test calls add_section followed by extend_section repeatedly. + """ bin_path = pathlib.Path(get_sample("MachO/MachO64_x86-64_binary_id.bin")) original = lief.MachO.parse(bin_path.as_posix()).at(0) output = f"{tmp_path}/test_extend_section.bin" text_segment = original.get_segment("__TEXT") - def make_section(name, alignment): - section = lief.MachO.Section(f"__lief_{alignment}") - section.alignment = alignment - return section - - sections = [ - make_section("__lief_8", 8), - make_section("__lief_7", 7), - make_section("__lief_6", 6), - make_section("__lief_5", 5), - make_section("__lief_4", 4), - make_section("__lief_3", 3), - make_section("__lief_2", 2), - make_section("__lief_1", 1), - make_section("__lief_0", 0), - make_section("__lief_00", 0), - ] - sections = [original.add_section(text_segment, s) for s in sections] - checked, err = lief.MachO.check_layout(original) - assert checked, err - - for i, section in enumerate(sections): + for i in range(9, -1, -1): + section = lief.MachO.Section(f"__lief_{i}") + section.alignment = i + section = original.add_section(text_segment, section) assert original.extend_section(section, 1 << section.alignment) - checked, err = lief.MachO.check_layout(original) + original.write(output) + new = lief.MachO.parse(output).at(0) + + checked, err = lief.MachO.check_layout(new) assert checked, err +def test_extend_section_2(tmp_path): + """ This test makes multiple calls to add_section, and then it + extends each added section using extend_section. + """ + bin_path = pathlib.Path(get_sample("MachO/MachO64_x86-64_binary_id.bin")) + original = lief.MachO.parse(bin_path.as_posix()).at(0) + output = f"{tmp_path}/test_extend_section.bin" + + text_segment = original.get_segment("__TEXT") + + sections = [] + for i in range(3): + section = lief.MachO.Section(f"__lief_{i}") + section.alignment = 2 # 2^2 == 4 bytes + sections.append(original.add_section(text_segment, section)) + + for section in sections: + assert original.extend_section(section, 1000) + original.write(output) new = lief.MachO.parse(output).at(0)