[ros-diffs] [reactos] 04/04: [NTOS:IO] Modify when 'PartitionBuffer' and how 'DriveLayout' are freed in IopCreateArcNamesDisk().

[Hermès Bélusca-Maïto]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=4b4ffa92f564a334ad919…

commit 4b4ffa92f564a334ad919bdda2404561aa1ab10f
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu Jun 10 19:44:59 2021 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Fri Jun 11 02:21:48 2021 +0200

    [NTOS:IO] Modify when 'PartitionBuffer' and how 'DriveLayout' are
freed in IopCreateArcNamesDisk().
    
    - Manage the lifetime of the temporary 'PartitionBuffer' buffer where
      it is locally used only, and free it as soon as possible, just after
      calculating the sector checksum. No need to then free it outside of
      the main for-loop.
    
    - When the 'DriveLayout' buffer is freed, ensure the pointer is NULL-ed
      (and assert this at the top of the main for-loop), since it can also
      be freed at cleanup outside this for-loop, and in this case a NULL
      check is performed.
      This will avoid the scenario of possibly double-freeing a pointer,
      in the case the 'DriveLayout' was previously freed (when e.g. reading
      the sector for checksum calculation failed), then the for-loop goes to
      the next disk and stops early.
---
 ntoskrnl/io/iomgr/arcname.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/ntoskrnl/io/iomgr/arcname.c b/ntoskrnl/io/iomgr/arcname.c
index 3be5930ec69..937b5cfb0c5 100644
--- a/ntoskrnl/io/iomgr/arcname.c
+++ b/ntoskrnl/io/iomgr/arcname.c
@@ -478,6 +478,8 @@ IopCreateArcNamesDisk(IN PLOADER_PARAMETER_BLOCK LoaderBlock,
     /* Start browsing disks */
     for (DiskNumber = 0; DiskNumber < DiskCount; DiskNumber++)
     {
+        ASSERT(DriveLayout == NULL);
+
         /* Check if we have an enabled disk */
         if (lSymbolicLinkList && *lSymbolicLinkList != UNICODE_NULL)
         {
@@ -652,6 +654,7 @@ IopCreateArcNamesDisk(IN PLOADER_PARAMETER_BLOCK LoaderBlock,
                                            &IoStatusBlock);
         if (!Irp)
         {
+            ExFreePoolWithTag(PartitionBuffer, TAG_IO);
             ObDereferenceObject(FileObject);
             Status = STATUS_INSUFFICIENT_RESOURCES;
             goto Cleanup;
@@ -665,20 +668,26 @@ IopCreateArcNamesDisk(IN PLOADER_PARAMETER_BLOCK LoaderBlock,
             KeWaitForSingleObject(&Event, Executive, KernelMode, FALSE, NULL);
             Status = IoStatusBlock.Status;
         }
-        if (!NT_SUCCESS(Status))
+
+        /* If reading succeeded, calculate checksum by adding data */
+        if (NT_SUCCESS(Status))
         {
-            ExFreePool(DriveLayout);
-            ExFreePoolWithTag(PartitionBuffer, TAG_IO);
-            ObDereferenceObject(FileObject);
-            continue;
+            for (i = 0, CheckSum = 0; i < 512 / sizeof(ULONG); i++)
+            {
+                CheckSum += PartitionBuffer[i];
+            }
         }
 
+        /* Release now unnecessary resources */
+        ExFreePoolWithTag(PartitionBuffer, TAG_IO);
         ObDereferenceObject(FileObject);
 
-        /* Calculate checksum by adding data */
-        for (i = 0, CheckSum = 0; i < 512 / sizeof(ULONG) ; i++)
+        /* If we failed, release drive layout before going to next disk */
+        if (!NT_SUCCESS(Status))
         {
-            CheckSum += PartitionBuffer[i];
+            ExFreePool(DriveLayout);
+            DriveLayout = NULL;
+            continue;
         }
 
         /* Browse each ARC disk */
@@ -800,29 +809,22 @@ IopCreateArcNamesDisk(IN PLOADER_PARAMETER_BLOCK LoaderBlock,
             }
         }
 
-        /* Release memory before jumping to next item */
+        /* Finally, release drive layout */
         ExFreePool(DriveLayout);
         DriveLayout = NULL;
-        ExFreePoolWithTag(PartitionBuffer, TAG_IO);
-        PartitionBuffer = NULL;
     }
 
     Status = STATUS_SUCCESS;
 
 Cleanup:
-    if (SymbolicLinkList)
-    {
-        ExFreePool(SymbolicLinkList);
-    }
-
     if (DriveLayout)
     {
         ExFreePool(DriveLayout);
     }
 
-    if (PartitionBuffer)
+    if (SymbolicLinkList)
     {
-        ExFreePoolWithTag(PartitionBuffer, TAG_IO);
+        ExFreePool(SymbolicLinkList);
     }
 
     return Status;