[ros-diffs] [reactos] 01/01: [FREELDR] fs.c: Fix handling of file handles, (de)referencing, and direct-device access (#7414)

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

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=5d361b602d45917ae966f…

commit 5d361b602d45917ae966ff3ae36a4e6f0bbf0665
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu Sep 26 21:45:49 2024 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Wed Oct 16 22:20:52 2024 +0200

    [FREELDR] fs.c: Fix handling of file handles, (de)referencing, and direct-device
access (#7414)
    
    - The original code was just incrementing the reference counts (RefCounts)
      of the device objects or the device/file handles, without decrementing
      them when closing the handles. This is now fixed.
    
      Notice the following:
    
      * When opening a file on a device (disk), the device's (and its
        handle's) RefCount is incremented, and the file handle's RefCount
        is incremented as well.
    
      * When closing a file, the file handle's RefCount is decremented
        (and the file closed if the RefCount reaches zero), and the file's
        parent device handle is also closed, recursively.
        This has the effect of decrementing the parent device handle's
        RefCount, and the device's own RefCount is decremented as well.
    
      IMPORTANT NOTE: The usefulness of handle-level RefCount is still
      under question, and might be (consistently) removed in the future.
    
    - Fix opening a device (disk) in direct access, when this device is
      already opened. Indeed, we previously allowed direct access only if
      the device was opened as such for the very first time (its RefCount
      = 0 originally); no filesystem mounting was attempted as well.
      Then for any later open-operations on this device (while keeping an
      already-opened handle to it), filesystem access was assumed.
    
      Thus, this problem would show up in two ways:
    
      * Either the device is first opened for direct access, this succeeded
        and no filesystem was mounted. Then, for any other open-operations,
        the filesystem was NOT mounted, and opening files on it would fail.
        Direct accesses would succeed but would create an unnecessary second
        file handle.
    
      * Or, the device is first opened for file-access: a filesystem was
        mounted and file opening would succeed. Any other file opening
        operation would succeed as well (if the file exists). But, a direct
        access open-operation would fail, because now any open-operations on
        the device would be assumed to be a file opening.
    
      This is now correctly fixed. If direct-open is requested, just do it.
      If this is a file opening, we open the device, then try to mount a
      filesystem on it (if not already done), then we try to open the file.
    
      If file opening fails, derereference the device.
    
    - Pass the file path to the filesystem-specific Open() functions without
      truncating the leading path separator, if any. This has to be handled
      by the filesystem routines themselves.
---
 boot/freeldr/freeldr/include/fs.h  |  10 +-
 boot/freeldr/freeldr/lib/fs/ext2.c |   3 +
 boot/freeldr/freeldr/lib/fs/fat.c  |   3 +
 boot/freeldr/freeldr/lib/fs/fs.c   | 256 +++++++++++++++++++++++++++++--------
 boot/freeldr/freeldr/lib/fs/iso.c  |   3 +
 boot/freeldr/freeldr/lib/fs/ntfs.c |   5 +
 boot/freeldr/freeldr/lib/fs/pxe.c  |  12 +-
 7 files changed, 236 insertions(+), 56 deletions(-)

diff --git a/boot/freeldr/freeldr/include/fs.h b/boot/freeldr/freeldr/include/fs.h
index a7acd071112..edc515b8715 100644
--- a/boot/freeldr/freeldr/include/fs.h
+++ b/boot/freeldr/freeldr/include/fs.h
@@ -35,7 +35,11 @@ typedef struct tagDEVVTBL
 #define INVALID_FILE_ID ((ULONG)-1)
 
 ARC_STATUS ArcOpen(CHAR* Path, OPENMODE OpenMode, ULONG* FileId);
-ARC_STATUS ArcClose(ULONG FileId);
+
+ARC_STATUS
+ArcClose(
+    _In_ ULONG FileId);
+
 ARC_STATUS ArcRead(ULONG FileId, VOID* Buffer, ULONG N, ULONG* Count);
 ARC_STATUS ArcSeek(ULONG FileId, LARGE_INTEGER* Position, SEEKMODE SeekMode);
 ARC_STATUS ArcGetFileInformation(ULONG FileId, FILEINFORMATION* Information);
@@ -58,7 +62,7 @@ FsRegisterDevice(
     _In_ const DEVVTBL* FuncTable);
 
 PCWSTR FsGetServiceName(ULONG FileId);
-VOID  FsSetDeviceSpecific(ULONG FileId, VOID* Specific);
-VOID* FsGetDeviceSpecific(ULONG FileId);
+VOID  FsSetDeviceSpecific(ULONG FileId, PVOID Specific);
+PVOID FsGetDeviceSpecific(ULONG FileId);
 ULONG FsGetDeviceId(ULONG FileId);
 VOID  FsInit(VOID);
diff --git a/boot/freeldr/freeldr/lib/fs/ext2.c b/boot/freeldr/freeldr/lib/fs/ext2.c
index fa68cd9d96a..6ff59398f29 100644
--- a/boot/freeldr/freeldr/lib/fs/ext2.c
+++ b/boot/freeldr/freeldr/lib/fs/ext2.c
@@ -216,6 +216,9 @@ BOOLEAN Ext2LookupFile(PEXT2_VOLUME_INFO Volume, PCSTR FileName,
PEXT2_FILE_INFO
 
     RtlZeroMemory(Ext2FileInfo, sizeof(EXT2_FILE_INFO));
 
+    /* Skip leading path separator, if any */
+    if (*FileName == '\\' || *FileName == '/')
+        ++FileName;
     //
     // Figure out how many sub-directories we are nested in
     //
diff --git a/boot/freeldr/freeldr/lib/fs/fat.c b/boot/freeldr/freeldr/lib/fs/fat.c
index 257f1e43c63..a31467b6310 100644
--- a/boot/freeldr/freeldr/lib/fs/fat.c
+++ b/boot/freeldr/freeldr/lib/fs/fat.c
@@ -785,6 +785,9 @@ ARC_STATUS FatLookupFile(PFAT_VOLUME_INFO Volume, PCSTR FileName,
PFAT_FILE_INFO
 
     RtlZeroMemory(FatFileInfoPointer, sizeof(FAT_FILE_INFO));
 
+    /* Skip leading path separator, if any */
+    if (*FileName == '\\' || *FileName == '/')
+        ++FileName;
     //
     // Figure out how many sub-directories we are nested in
     //
diff --git a/boot/freeldr/freeldr/lib/fs/fs.c b/boot/freeldr/freeldr/lib/fs/fs.c
index 4bc9f2863f0..cefdad7249f 100644
--- a/boot/freeldr/freeldr/lib/fs/fs.c
+++ b/boot/freeldr/freeldr/lib/fs/fs.c
@@ -30,24 +30,24 @@ DBG_DEFAULT_CHANNEL(FILESYSTEM);
 #define TAG_DEVICE_NAME 'NDsF'
 #define TAG_DEVICE 'vDsF'
 
-typedef struct tagFILEDATA
-{
-    ULONG DeviceId;
-    ULONG ReferenceCount;
-    const DEVVTBL* FuncTable;
-    const DEVVTBL* FileFuncTable;
-    VOID* Specific;
-} FILEDATA;
-
 typedef struct tagDEVICE
 {
     LIST_ENTRY ListEntry;
-    const DEVVTBL* FuncTable;
+    const DEVVTBL* FuncTable;     ///< Driver function table.
+    const DEVVTBL* FileFuncTable; ///< Used only by mounted storage devices.
     PSTR DeviceName;
-    ULONG DeviceId;
+    ULONG DeviceId; ///< Entry ID in FileData when the device gets referenced.
     ULONG ReferenceCount;
 } DEVICE;
 
+typedef struct tagFILEDATA
+{
+    ULONG DeviceId; ///< Parent device's ID.
+    ULONG ReferenceCount;
+    const DEVVTBL* FuncTable;
+    PVOID Specific;
+} FILEDATA;
+
 static FILEDATA FileData[MAX_FDS];
 static LIST_ENTRY DeviceListHead;
 
@@ -75,6 +75,84 @@ PFS_MOUNT FileSystems[] =
 };
 
 
+/* DEBUGGING HELPERS **********************************************************/
+
+#if DBG && 0
+static VOID
+DumpDeviceList(VOID)
+{
+    PLIST_ENTRY pEntry;
+
+    DbgPrint("\n== Dumping ARC devices ==\n");
+
+    for (pEntry = DeviceListHead.Flink;
+         pEntry != &DeviceListHead;
+         pEntry = pEntry->Flink)
+    {
+        DEVICE* pDevice = CONTAINING_RECORD(pEntry, DEVICE, ListEntry);
+        ULONG DeviceId = pDevice->DeviceId;
+        const DEVVTBL* FuncTable;
+
+        DbgPrint("\n"
+                 "DeviceId %lu\n"
+                 "  RefCount: %lu\n"
+                 "  DeviceName  : '%s'\n",
+                 DeviceId, pDevice->ReferenceCount, pDevice->DeviceName);
+
+        FuncTable = pDevice->FuncTable;
+        DbgPrint("  DevFuncTable: 0x%p (In array: 0x%p) ; Name:
'%S'\n",
+                 FuncTable,
+                 (DeviceId != INVALID_FILE_ID ? FileData[DeviceId].FuncTable : NULL),
+                 (FuncTable && FuncTable->ServiceName)
+                    ? FuncTable->ServiceName : L"n/a");
+
+        FuncTable = pDevice->FileFuncTable;
+        DbgPrint("  FileFuncTable: 0x%p ; Name: '%S'\n",
+                 FuncTable,
+                 (FuncTable && FuncTable->ServiceName)
+                    ? FuncTable->ServiceName : L"n/a");
+    }
+
+    DbgPrint("\n== END Dumping ARC devices ==\n\n");
+}
+
+static VOID
+DumpFileTable(VOID)
+{
+    ULONG i;
+
+    DbgPrint("\n== Dumping ARC files table ==\n");
+
+    for (i = 0; i < _countof(FileData); ++i)
+    {
+        FILEDATA* pFileData = &FileData[i];
+        const DEVVTBL* FuncTable;
+
+        /* Show only occupied slots */
+        if (!pFileData->FuncTable)
+            continue;
+
+        DbgPrint("\n"
+                 "FileId %lu\n"
+                 "  RefCount: %lu\n"
+                 "  ParentDeviceId: %lu\n"
+                 /*"  FileName  : '%s'\n"*/
+                 "  Specific: 0x%p\n",
+                 i, pFileData->ReferenceCount,
+                 pFileData->DeviceId, pFileData->Specific);
+
+        FuncTable = pFileData->FuncTable;
+        DbgPrint("  FuncTable: 0x%p ; Name: '%S'\n",
+                 FuncTable,
+                 (FuncTable && FuncTable->ServiceName)
+                    ? FuncTable->ServiceName : L"n/a");
+    }
+
+    DbgPrint("\n== END Dumping ARC files table ==\n\n");
+}
+#endif // DBG
+
+
 /* ARC FUNCTIONS **************************************************************/
 
 /**
@@ -204,6 +282,7 @@ ARC_STATUS ArcOpen(CHAR* Path, OPENMODE OpenMode, ULONG* FileId)
         }
 
         /* Try to open the device */
+        FileData[DeviceId].ReferenceCount = 0;
         FileData[DeviceId].FuncTable = pDevice->FuncTable;
         Status = pDevice->FuncTable->Open(pDevice->DeviceName, DeviceOpenMode,
&DeviceId);
         if (Status != ESUCCESS)
@@ -211,57 +290,71 @@ ARC_STATUS ArcOpen(CHAR* Path, OPENMODE OpenMode, ULONG* FileId)
             FileData[DeviceId].FuncTable = NULL;
             return Status;
         }
-        else if (!*FileName)
-        {
-            /* Done, caller wanted to open the raw device */
-            *FileId = DeviceId;
-            pDevice->ReferenceCount++;
-            return ESUCCESS;
-        }
-
-        /* Try to detect the file system */
-        for (ULONG fs = 0; fs < _countof(FileSystems); ++fs)
-        {
-            FileData[DeviceId].FileFuncTable = FileSystems[fs](DeviceId);
-            if (FileData[DeviceId].FileFuncTable)
-                break;
-        }
-        if (!FileData[DeviceId].FileFuncTable)
-        {
-            /* Error, unable to detect the file system */
-            pDevice->FuncTable->Close(DeviceId);
-            FileData[DeviceId].FuncTable = NULL;
-            return ENODEV;
-        }
-
         pDevice->DeviceId = DeviceId;
     }
     else
     {
+        /* Reuse the existing entry */
         DeviceId = pDevice->DeviceId;
+        ASSERT(FileData[DeviceId].FuncTable == pDevice->FuncTable);
     }
+
+    /* Done, increase the device reference count */
     pDevice->ReferenceCount++;
+    FileData[DeviceId].ReferenceCount++;
 
-    /* At this point, device is found and opened. Its file id is stored
-     * in DeviceId, and FileData[DeviceId].FileFuncTable contains what
-     * needs to be called to open the file */
+    if (!*FileName)
+    {
+        /* The caller wanted to open the raw device: return its ID */
+        *FileId = DeviceId;
+        return ESUCCESS;
+    }
+
+
+    /*
+     * We are opening a file.
+     */
 
     /* Find some room for the file */
     for (i = 0; ; ++i)
     {
         if (i >= _countof(FileData))
-            return EMFILE;
+        {
+            Status = EMFILE;
+            goto Done;
+        }
         if (!FileData[i].FuncTable)
             break;
     }
 
-    /* Skip leading path separator, if any */
-    if (*FileName == '\\' || *FileName == '/')
-        FileName++;
+    /* The device is accessed using filesystem semantics.
+     * Try to detect the file system if not already done. */
+    if (!pDevice->FileFuncTable && (pDevice->ReferenceCount <= 1))
+    {
+        for (ULONG fs = 0; fs < _countof(FileSystems); ++fs)
+        {
+            pDevice->FileFuncTable = FileSystems[fs](DeviceId);
+            if (pDevice->FileFuncTable)
+                break;
+        }
+    }
+    if (!pDevice->FileFuncTable)
+    {
+        /* Error, unable to detect the file system */
+        Status = ENOENT; // ENXIO;
+        goto Done;
+    }
+
+    /*
+     * At this point, the device is found and opened. Its file ID is stored
+     * in DeviceId, and pDevice->FileFuncTable contains what needs to be
+     * called to manipulate the file.
+     */
 
     /* Open the file */
-    FileData[i].FuncTable = FileData[DeviceId].FileFuncTable;
     FileData[i].DeviceId = DeviceId;
+    FileData[i].ReferenceCount = 0;
+    FileData[i].FuncTable = pDevice->FileFuncTable;
     *FileId = i;
     Status = FileData[i].FuncTable->Open(FileName, OpenMode, FileId);
     if (Status != ESUCCESS)
@@ -271,24 +364,85 @@ ARC_STATUS ArcOpen(CHAR* Path, OPENMODE OpenMode, ULONG* FileId)
         FileData[i].Specific = NULL;
         *FileId = INVALID_FILE_ID;
     }
+    else
+    {
+        /* Reference the file */
+        FileData[i].ReferenceCount++;
+    }
+
+Done:
+    /* If we failed somewhere, dereference the device as well */
+    if (Status != ESUCCESS)
+    {
+        // ArcClose(DeviceId);
+        if (--FileData[DeviceId].ReferenceCount == 0)
+        {
+            (void)FileData[DeviceId].FuncTable->Close(DeviceId);
+            FileData[DeviceId].DeviceId = INVALID_FILE_ID;
+            FileData[DeviceId].FuncTable = NULL;
+            FileData[DeviceId].Specific = NULL;
+        }
+        if (--pDevice->ReferenceCount == 0)
+            pDevice->DeviceId = INVALID_FILE_ID;
+    }
+
     return Status;
 }
 
-ARC_STATUS ArcClose(ULONG FileId)
+static DEVICE*
+FsGetDeviceById(ULONG DeviceId)
 {
-    ARC_STATUS Status;
+    PLIST_ENTRY pEntry;
+
+    for (pEntry = DeviceListHead.Flink;
+         pEntry != &DeviceListHead;
+         pEntry = pEntry->Flink)
+    {
+        DEVICE* pDevice = CONTAINING_RECORD(pEntry, DEVICE, ListEntry);
+        if (pDevice->DeviceId == DeviceId)
+            return pDevice;
+    }
+    return NULL;
+}
+
+ARC_STATUS
+ArcClose(
+    _In_ ULONG FileId)
+{
+    ULONG DeviceId;
+    DEVICE* pDevice;
 
     if (!IS_VALID_FILEID(FileId))
         return EBADF;
 
-    Status = FileData[FileId].FuncTable->Close(FileId);
-    if (Status == ESUCCESS)
+    /* Retrieve the parent device's ID if any, for later */
+    DeviceId = FileData[FileId].DeviceId;
+
+    /* Dereference the file and close it if needed */
+    ASSERT(FileData[FileId].ReferenceCount > 0);
+    if (--FileData[FileId].ReferenceCount == 0)
     {
+        (void)FileData[FileId].FuncTable->Close(FileId);
         FileData[FileId].DeviceId = INVALID_FILE_ID;
         FileData[FileId].FuncTable = NULL;
         FileData[FileId].Specific = NULL;
     }
-    return Status;
+
+    /* Check whether this file actually references a device */
+    pDevice = FsGetDeviceById(FileId);
+    if (pDevice)
+    {
+        /* It does, dereference it */
+        ASSERT(pDevice->ReferenceCount > 0);
+        if (--pDevice->ReferenceCount == 0)
+            pDevice->DeviceId = INVALID_FILE_ID;
+    }
+
+    /* And dereference the parent device too, if there is one */
+    if (IS_VALID_FILEID(DeviceId))
+        ArcClose(DeviceId);
+
+    return ESUCCESS;
 }
 
 ARC_STATUS ArcRead(ULONG FileId, VOID* Buffer, ULONG N, ULONG* Count)
@@ -381,7 +535,7 @@ FsOpenFile(
 /*
  * FsGetNumPathParts()
  * This function parses a path in the form of dir1\dir2\file1.ext
- * and returns the number of parts it has (i.e. 3 - dir1,dir2,file1.ext)
+ * and returns the number of parts it has (i.e. 3 - dir1,dir2,file1.ext).
  */
 ULONG FsGetNumPathParts(PCSTR Path)
 {
@@ -409,7 +563,7 @@ ULONG FsGetNumPathParts(PCSTR Path)
  * FsGetFirstNameFromPath()
  * This function parses a path in the form of dir1\dir2\file1.ext
  * and puts the first name of the path (e.g. "dir1") in buffer
- * compatible with the MSDOS directory structure
+ * compatible with the MSDOS directory structure.
  */
 VOID FsGetFirstNameFromPath(PCHAR Buffer, PCSTR Path)
 {
@@ -468,14 +622,14 @@ PCWSTR FsGetServiceName(ULONG FileId)
     return FileData[FileId].FuncTable->ServiceName;
 }
 
-VOID FsSetDeviceSpecific(ULONG FileId, VOID* Specific)
+VOID FsSetDeviceSpecific(ULONG FileId, PVOID Specific)
 {
     if (!IS_VALID_FILEID(FileId))
         return;
     FileData[FileId].Specific = Specific;
 }
 
-VOID* FsGetDeviceSpecific(ULONG FileId)
+PVOID FsGetDeviceSpecific(ULONG FileId)
 {
     if (!IS_VALID_FILEID(FileId))
         return NULL;
diff --git a/boot/freeldr/freeldr/lib/fs/iso.c b/boot/freeldr/freeldr/lib/fs/iso.c
index 1079d578883..a8314b1ac8a 100644
--- a/boot/freeldr/freeldr/lib/fs/iso.c
+++ b/boot/freeldr/freeldr/lib/fs/iso.c
@@ -182,6 +182,9 @@ static ARC_STATUS IsoLookupFile(PCSTR FileName, ULONG DeviceId,
PISO_FILE_INFO I
     DirectorySector = Pvd->RootDirRecord.ExtentLocationL;
     DirectoryLength = Pvd->RootDirRecord.DataLengthL;
 
+    /* Skip leading path separator, if any */
+    if (*FileName == '\\' || *FileName == '/')
+        ++FileName;
     //
     // Figure out how many sub-directories we are nested in
     //
diff --git a/boot/freeldr/freeldr/lib/fs/ntfs.c b/boot/freeldr/freeldr/lib/fs/ntfs.c
index fb4db082cd8..c8d4a547d58 100644
--- a/boot/freeldr/freeldr/lib/fs/ntfs.c
+++ b/boot/freeldr/freeldr/lib/fs/ntfs.c
@@ -718,6 +718,11 @@ static BOOLEAN NtfsLookupFile(PNTFS_VOLUME_INFO Volume, PCSTR
FileName, PNTFS_MF
     TRACE("NtfsLookupFile() FileName = %s\n", FileName);
 
     CurrentMFTIndex = NTFS_FILE_ROOT;
+
+    /* Skip leading path separator, if any */
+    if (*FileName == '\\' || *FileName == '/')
+        ++FileName;
+
     NumberOfPathParts = FsGetNumPathParts(FileName);
     for (i = 0; i < NumberOfPathParts; i++)
     {
diff --git a/boot/freeldr/freeldr/lib/fs/pxe.c b/boot/freeldr/freeldr/lib/fs/pxe.c
index 66145a3d328..1a1866ed058 100644
--- a/boot/freeldr/freeldr/lib/fs/pxe.c
+++ b/boot/freeldr/freeldr/lib/fs/pxe.c
@@ -155,10 +155,18 @@ static ARC_STATUS PxeOpen(CHAR* Path, OPENMODE OpenMode, ULONG*
FileId)
     if (OpenMode != OpenReadOnly)
         return EACCES;
 
+    /* Skip leading path separator, if any, so as to ensure that
+     * we always lookup the file at the root of the TFTP server's
+     * file space ("virtual root"), even if the server doesn't
+     * support this, and NOT from the root of the file system. */
+    if (*Path == '\\' || *Path == '/')
+        ++Path;
+
     /* Retrieve the path length without NULL terminator */
-    PathLen = (Path ? min(strlen(Path), sizeof(_OpenFileName) - 1) : 0);
+    PathLen = min(strlen(Path), sizeof(_OpenFileName) - 1);
 
-    /* Lowercase the path and always use slashes as separators */
+    /* Lowercase the path and always use slashes as separators,
+     * for supporting TFTP servers on POSIX systems */
     for (i = 0; i < PathLen; i++)
     {
         if (Path[i] == '\\')