Minimize FreeSpace nesting

ofrak_core/CHANGELOG.md

@@ -38,6 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Pass `usedforsecurity=False` to non-cryptographic `hashlib` calls to prevent failures when Python links against FIPS OpenSSL ([#744](https://github.com/redballoonsecurity/ofrak/pull/744))
 - Fix `chdir` in UEFI components causing failing tests. ([#747](https://github.com/redballoonsecurity/ofrak/pull/747))
 - Fix bug in `ISO9660Unpacker` for files in root directory. ([#750](https://github.com/redballoonsecurity/ofrak/pull/750))
+- `RemoveFreeSpaceModifier` now creates remaining free space as siblings instead of children. ([#754](https://github.com/redballoonsecurity/ofrak/pull/754))
 - Fix binwalk and entropy file descriptor leak. ([#749](https://github.com/redballoonsecurity/ofrak/pull/749))
 
 ## [3.3.0](https://github.com/redballoonsecurity/ofrak/compare/ofrak-v3.2.0...ofrak-v3.3.0) - 2025-10-03

ofrak_core/src/ofrak/core/free_space.py

@@ -367,10 +367,10 @@ async def analyze(self, resource: Resource, config: ComponentConfig = None) -> A
 class RemoveFreeSpaceModifier(Modifier[FreeSpaceAllocation]):
     """
     After allocating some space from an `Allocatable`, fix up its descendants to make sure the
-    allocated space will not be allocated again. Remove FreeSpace tags from resources which
-    overlap with an allocated range. If part of one of these resources is not within an
-    allocated range, create a child tagged as FreeSpace to reflect that part of it is still
-    available as free space.
+    allocated space will not be allocated again. Deletes FreeSpace resources that overlap with
+    an allocated range. If part of an original resource is not within an allocated range,
+    create new sibling FreeSpace resources under the original resource's parent to reflect
+    that part of it is still available as free space.
     """
 
     targets = (Allocatable,)
@@ -421,37 +421,38 @@ async def modify(self, resource: Resource, config: FreeSpaceAllocation) -> None:
                     )
 
         for fs in wholly_allocated_resources:
-            fs.resource.remove_tag(FreeSpace)
-            fs.resource.remove_tag(RuntimeFreeSpace)
-            fs.resource.remove_tag(AnyFreeSpace)
+            await fs.resource.delete()
 
         for fs, allocated_ranges in partially_allocated_resources.values():
             remaining_free_space_ranges = remove_subranges([fs.vaddr_range()], allocated_ranges)
+            parent = await fs.resource.get_parent()
             if fs.resource.has_tag(FreeSpace):
+                fs_offset_in_parent = (await fs.resource.get_data_range_within_parent()).start
                 for remaining_range in remaining_free_space_ranges:
                     remaining_data_range = Range.from_size(
-                        fs.get_offset_in_self(remaining_range.start), remaining_range.length()
+                        fs_offset_in_parent + fs.get_offset_in_self(remaining_range.start),
+                        remaining_range.length(),
                     )
-                    await fs.resource.create_child_from_view(
+                    await parent.create_child_from_view(
                         FreeSpace(
                             remaining_range.start,
                             remaining_range.length(),
                             fs.permissions,
                         ),
                         data_range=remaining_data_range,
                     )
-                fs.resource.remove_tag(FreeSpace)
+                await fs.resource.delete()
             elif fs.resource.has_tag(RuntimeFreeSpace):
                 for remaining_range in remaining_free_space_ranges:
-                    await fs.resource.create_child_from_view(
+                    await parent.create_child_from_view(
                         RuntimeFreeSpace(
                             remaining_range.start,
                             remaining_range.length(),
                             fs.permissions,
                         ),
                         data_range=None,
                     )
-                fs.resource.remove_tag(RuntimeFreeSpace)
+                await fs.resource.delete()
             else:
                 raise TypeError(f"Got AnyFreeSpace {fs} without FreeSpace or RuntimeFreeSpace tags")
 

ofrak_core/tests/components/free_space_components_test/test_allocation_modifier.py

@@ -71,7 +71,7 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
 
 FREE_SPACE_ALLOCATION_MODIFIER_TEST_CASES = [
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from an entire resource",
+        "allocation deletes a wholly-allocated FreeSpace resource",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -87,13 +87,12 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             [
                 (MemoryRegion(0x0, 0x40), None),
                 (MemoryRegion(0x40, 0x40), None),
-                (MemoryRegion(0x80, 0x40), None),
                 (MemoryRegion(0xC0, 0x40), None),
             ],
         ),
     ),
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from multiple entire resources",
+        "allocation deletes multiple wholly-allocated FreeSpace resources",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -109,15 +108,11 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             MemoryRegion(0x0, 0x100),
             [
                 (MemoryRegion(0x0, 0x40), None),
-                (MemoryRegion(0x40, 0x40), None),
-                (MemoryRegion(0x80, 0x40), None),
-                (MemoryRegion(0xC0, 0x40), None),
-                (MemoryRegion(0xD00, 0x40), None),
             ],
         ),
     ),
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from resource, creates FreeSpace resource child on right side",
+        "allocation creates sibling FreeSpace resource on right side",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -133,16 +128,13 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             [
                 (MemoryRegion(0x0, 0x40), None),
                 (MemoryRegion(0x40, 0x40), None),
-                (
-                    MemoryRegion(0x80, 0x40),
-                    [(FreeSpace(0xA0, 0x20, MemoryPermissions.RX), None)],
-                ),
+                (FreeSpace(0xA0, 0x20, MemoryPermissions.RX), None),
                 (MemoryRegion(0xC0, 0x40), None),
             ],
         ),
     ),
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from resource, creates FreeSpace resource child on left side",
+        "allocation creates sibling FreeSpace resource on left side",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -158,16 +150,13 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             [
                 (MemoryRegion(0x0, 0x40), None),
                 (MemoryRegion(0x40, 0x40), None),
-                (
-                    MemoryRegion(0x80, 0x40),
-                    [(FreeSpace(0x80, 0x20, MemoryPermissions.RX), None)],
-                ),
+                (FreeSpace(0x80, 0x20, MemoryPermissions.RX), None),
                 (MemoryRegion(0xC0, 0x40), None),
             ],
         ),
     ),
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from resource, creates FreeSpace resource child on left and right",
+        "allocation creates sibling FreeSpace resources on left and right",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -183,19 +172,14 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             [
                 (MemoryRegion(0x0, 0x40), None),
                 (MemoryRegion(0x40, 0x40), None),
-                (
-                    MemoryRegion(0x80, 0x40),
-                    [
-                        (FreeSpace(0x80, 0x10, MemoryPermissions.RX), None),
-                        (FreeSpace(0xB0, 0x10, MemoryPermissions.RX), None),
-                    ],
-                ),
+                (FreeSpace(0x80, 0x10, MemoryPermissions.RX), None),
+                (FreeSpace(0xB0, 0x10, MemoryPermissions.RX), None),
                 (MemoryRegion(0xC0, 0x40), None),
             ],
         ),
     ),
     FreeSpaceAllocationModifierTestCase(
-        "allocation removes tag from resource, creates FreeSpace resource child in middle",
+        "allocation creates sibling FreeSpace resource in middle",
         (
             MemoryRegion(0x0, 0x100),
             [
@@ -214,12 +198,7 @@ async def validate_tree(actual_mem_region: MemoryRegion, expected_node: FreeSpac
             [
                 (MemoryRegion(0x0, 0x40), None),
                 (MemoryRegion(0x40, 0x40), None),
-                (
-                    MemoryRegion(0x80, 0x40),
-                    [
-                        (FreeSpace(0x90, 0x20, MemoryPermissions.RX), None),
-                    ],
-                ),
+                (FreeSpace(0x90, 0x20, MemoryPermissions.RX), None),
                 (MemoryRegion(0xC0, 0x40), None),
             ],
         ),
@@ -248,3 +227,44 @@ async def test_free_space_analyzer(
         await allocatable_r.view_as(MemoryRegion),
         test_case.resulting_tree_structure,
     )
+
+
+async def test_repeated_partial_allocations_stay_flat(ofrak_context: OFRAKContext):
+    """
+    Regression test: N sequential partial allocations against a single FreeSpace must
+    produce N flat siblings under the Allocatable root, not a depth-N chain. The chained
+    form caused 30%-ish segfaults during deletion of injected pixel firmware due to C-stack
+    overflow in the recursive Resource.delete().
+    """
+    initial_tree: FreeSpaceTreeType = (
+        MemoryRegion(0x0, 0x1000),
+        [(FreeSpace(0x0, 0x1000, MemoryPermissions.RX), None)],
+    )
+    root_r = await inflate_tree(initial_tree, ofrak_context)
+
+    n_allocations = 50
+    chunk_size = 0x10
+    for i in range(n_allocations):
+        start = i * chunk_size
+        await root_r.run(
+            RemoveFreeSpaceModifier,
+            FreeSpaceAllocation(MemoryPermissions.RX, [Range(start, start + chunk_size)]),
+        )
+
+    root_region = await root_r.view_as(MemoryRegion)
+    direct_children = list(
+        await root_region.resource.get_children_as_view(
+            MemoryRegion, r_sort=ResourceSort(MemoryRegion.VirtualAddress)
+        )
+    )
+    for child in direct_children:
+        grandchildren = list(await child.resource.get_children())
+        assert grandchildren == [], (
+            f"Expected flat tree under Allocatable, but {child} has {len(grandchildren)} "
+            f"descendant(s). RemoveFreeSpaceModifier should create siblings, not children."
+        )
+
+    free_spaces = [c for c in direct_children if c.resource.has_tag(FreeSpace)]
+    assert len(free_spaces) == 1, f"Expected 1 surviving FreeSpace, found {len(free_spaces)}"
+    assert free_spaces[0].virtual_address == n_allocations * chunk_size
+    assert free_spaces[0].size == 0x1000 - n_allocations * chunk_size

ofrak_core/version.py

@@ -1 +1 @@
-VERSION = "3.4.0rc14"
+VERSION = "3.4.0rc15"