[NTOS:IO] Modify when 'PartitionBuffer' and how 'DriveLayout' are freed in IopCreateA...
authorHermès Bélusca-Maïto <hermes.belusca-maito@reactos.org>
Thu, 10 Jun 2021 17:44:59 +0000 (19:44 +0200)
committerHermès Bélusca-Maïto <hermes.belusca-maito@reactos.org>
Fri, 11 Jun 2021 00:21:48 +0000 (02:21 +0200)
- 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

index 3be5930..937b5cf 100644 (file)
@@ -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;