[ros-diffs] [reactos] 01/02: [NTOS:OB] Clarify and fix the usage of the Obp*DirectoryLock*() and ObpReleaseLookupContextObject() functions.

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

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

commit 4c63ed5a7af6338cc63f1395d9895b22c0827a47
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Sat Sep 25 00:03:56 2021 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Sat Sep 25 00:47:43 2021 +0200

    [NTOS:OB] Clarify and fix the usage of the Obp*DirectoryLock*() and
ObpReleaseLookupContextObject() functions.
    
    - Disentangle the usage of ObpAcquireDirectoryLockExclusive() when it's
      used only for accessing a directory structure, or as part of a lookup
      operation.
    
      The Obp*DirectoryLock*() -- both shared and exclusive -- functions
      are only for locking an OB directory, for reading or writing its
      structure members.
    
      When performing lookup operations (insertions/deletions of entries
      within a directory), use a ObpAcquireLookupContextLock() function that
      exclusively locks the directory and saves extra lock state, that can
      be used by ObpReleaseLookupContextObject() for cleanup.
    
    - Add documentation for these functions.
---
 ntoskrnl/include/internal/ob_x.h | 96 ++++++++++++++++++++++++++++++++++------
 ntoskrnl/ob/obdir.c              |  2 +-
 ntoskrnl/ob/obinit.c             |  6 +--
 ntoskrnl/ob/oblife.c             |  6 +--
 ntoskrnl/ob/obname.c             | 12 +++--
 5 files changed, 93 insertions(+), 29 deletions(-)

diff --git a/ntoskrnl/include/internal/ob_x.h b/ntoskrnl/include/internal/ob_x.h
index 587b82ebc61..0594c08b8f8 100644
--- a/ntoskrnl/include/internal/ob_x.h
+++ b/ntoskrnl/include/internal/ob_x.h
@@ -169,15 +169,26 @@ ObpDereferenceNameInfo(IN POBJECT_HEADER_NAME_INFO HeaderNameInfo)
     }
 }
 
+/**
+ * @brief
+ * Locks a directory for shared access.
+ * Used for reading members of the directory object.
+ *
+ * @param[in] Directory
+ * The directory to lock.
+ *
+ * @param[in] Context
+ * The lookup lock context.
+ */
 FORCEINLINE
 VOID
 ObpAcquireDirectoryLockShared(IN POBJECT_DIRECTORY Directory,
-                               IN POBP_LOOKUP_CONTEXT Context)
+                              IN POBP_LOOKUP_CONTEXT Context)
 {
-    /* It's not, set lock flag */
+    /* Update lock flag */
     Context->LockStateSignature = OBP_LOCK_STATE_PRE_ACQUISITION_SHARED;
 
-    /* Lock it */
+    /* Acquire an shared directory lock */
     KeEnterCriticalRegion();
     ExAcquirePushLockShared(&Directory->Lock);
 
@@ -185,10 +196,21 @@ ObpAcquireDirectoryLockShared(IN POBJECT_DIRECTORY Directory,
     Context->LockStateSignature = OBP_LOCK_STATE_POST_ACQUISITION_SHARED;
 }
 
+/**
+ * @brief
+ * Locks a directory for exclusive access.
+ * Used for writing/reading members of the directory object.
+ *
+ * @param[in] Directory
+ * The directory to lock.
+ *
+ * @param[in] Context
+ * The lookup lock context.
+ */
 FORCEINLINE
 VOID
 ObpAcquireDirectoryLockExclusive(IN POBJECT_DIRECTORY Directory,
-                                  IN POBP_LOOKUP_CONTEXT Context)
+                                 IN POBP_LOOKUP_CONTEXT Context)
 {
     /* Update lock flag */
     Context->LockStateSignature = OBP_LOCK_STATE_PRE_ACQUISITION_EXCLUSIVE;
@@ -197,18 +219,24 @@ ObpAcquireDirectoryLockExclusive(IN POBJECT_DIRECTORY Directory,
     KeEnterCriticalRegion();
     ExAcquirePushLockExclusive(&Directory->Lock);
 
-    /* Set the directory */
-    Context->Directory = Directory;
-
-    /* Update lock settings */
+    /* Update lock flag */
     Context->LockStateSignature = OBP_LOCK_STATE_POST_ACQUISITION_EXCLUSIVE;
-    Context->DirectoryLocked = TRUE;
 }
 
+/**
+ * @brief
+ * Unlocks a previously shared or exclusively locked directory.
+ *
+ * @param[in] Directory
+ * The directory to unlock.
+ *
+ * @param[in] Context
+ * The lookup lock context.
+ */
 FORCEINLINE
 VOID
 ObpReleaseDirectoryLock(IN POBJECT_DIRECTORY Directory,
-                         IN POBP_LOOKUP_CONTEXT Context)
+                        IN POBP_LOOKUP_CONTEXT Context)
 {
     /* Release the lock */
     ExReleasePushLock(&Directory->Lock);
@@ -216,6 +244,15 @@ ObpReleaseDirectoryLock(IN POBJECT_DIRECTORY Directory,
     KeLeaveCriticalRegion();
 }
 
+/**
+ * @brief
+ * Initializes a new object directory lookup context.
+ * Used for lookup operations (insertions/deletions) in a directory.
+ * Employed in conjunction with the directory locking functions.
+ *
+ * @param[in] Context
+ * The new lookup context to initialize.
+ */
 FORCEINLINE
 VOID
 ObpInitializeLookupContext(IN POBP_LOOKUP_CONTEXT Context)
@@ -227,6 +264,29 @@ ObpInitializeLookupContext(IN POBP_LOOKUP_CONTEXT Context)
     Context->LockStateSignature = OBP_LOCK_STATE_INITIALIZED;
 }
 
+/**
+ * @brief
+ * Locks an object directory lookup context for performing
+ * lookup operations (insertions/deletions) in a directory.
+ * The directory is locked for exclusive access.
+ *
+ * @param[in] Context
+ * The lookup context to lock.
+ *
+ * @param[in] Directory
+ * The directory on which the lookup context applies.
+ */
+FORCEINLINE
+VOID
+ObpAcquireLookupContextLock(IN POBP_LOOKUP_CONTEXT Context,
+                            IN POBJECT_DIRECTORY Directory)
+{
+    /* Acquire an exclusive directory lock and save its lock state */
+    ObpAcquireDirectoryLockExclusive(Directory, Context);
+    Context->Directory = Directory;
+    Context->DirectoryLocked = TRUE;
+}
+
 FORCEINLINE
 VOID
 ObpReleaseLookupContextObject(IN POBP_LOOKUP_CONTEXT Context)
@@ -234,14 +294,14 @@ ObpReleaseLookupContextObject(IN POBP_LOOKUP_CONTEXT Context)
     POBJECT_HEADER ObjectHeader;
     POBJECT_HEADER_NAME_INFO HeaderNameInfo;
 
-    /* Check if we had found an object */
+    /* Check if we had an object */
     if (Context->Object)
     {
         /* Get the object name information */
         ObjectHeader = OBJECT_TO_OBJECT_HEADER(Context->Object);
         HeaderNameInfo = OBJECT_HEADER_TO_NAME_INFO(ObjectHeader);
 
-        /* release the name information */
+        /* Release the name information */
         ObpDereferenceNameInfo(HeaderNameInfo);
 
         /* Dereference the object */
@@ -250,6 +310,14 @@ ObpReleaseLookupContextObject(IN POBP_LOOKUP_CONTEXT Context)
     }
 }
 
+/**
+ * @brief
+ * Releases an initialized object directory lookup context.
+ * Unlocks it if necessary, and dereferences the underlying object.
+ *
+ * @param[in] Context
+ * The lookup context to release.
+ */
 FORCEINLINE
 VOID
 ObpReleaseLookupContext(IN POBP_LOOKUP_CONTEXT Context)
@@ -257,13 +325,13 @@ ObpReleaseLookupContext(IN POBP_LOOKUP_CONTEXT Context)
     /* Check if we came back with the directory locked */
     if (Context->DirectoryLocked)
     {
-        /* Release the lock */
+        /* Release the directory lock */
         ObpReleaseDirectoryLock(Context->Directory, Context);
         Context->Directory = NULL;
         Context->DirectoryLocked = FALSE;
     }
 
-    /* Clear the context  */
+    /* Clear the context */
     ObpReleaseLookupContextObject(Context);
 }
 
diff --git a/ntoskrnl/ob/obdir.c b/ntoskrnl/ob/obdir.c
index 1c7b457c726..cb7699ac840 100644
--- a/ntoskrnl/ob/obdir.c
+++ b/ntoskrnl/ob/obdir.c
@@ -573,7 +573,7 @@ NtQueryDirectoryObject(IN HANDLE DirectoryHandle,
         return Status;
     }
 
-    /* Lock directory in shared mode */
+    /* Lock the directory in shared mode */
     ObpAcquireDirectoryLockShared(Directory, &LookupContext);
 
     /* Start at position 0 */
diff --git a/ntoskrnl/ob/obinit.c b/ntoskrnl/ob/obinit.c
index 0b03779d820..8c61db54756 100644
--- a/ntoskrnl/ob/obinit.c
+++ b/ntoskrnl/ob/obinit.c
@@ -386,11 +386,9 @@ ObPostPhase0:
     Status = NtClose(Handle);
     if (!NT_SUCCESS(Status)) return FALSE;
 
-    /* Initialize lookup context */
+    /* Initialize the lookup context and lock it */
     ObpInitializeLookupContext(&Context);
-
-    /* Lock it */
-    ObpAcquireDirectoryLockExclusive(ObpTypeDirectoryObject, &Context);
+    ObpAcquireLookupContextLock(&Context, ObpTypeDirectoryObject);
 
     /* Loop the object types */
     ListHead = &ObpTypeObjectType->TypeList;
diff --git a/ntoskrnl/ob/oblife.c b/ntoskrnl/ob/oblife.c
index 5fc68b0d6b5..a01f5373bca 100644
--- a/ntoskrnl/ob/oblife.c
+++ b/ntoskrnl/ob/oblife.c
@@ -1093,8 +1093,8 @@ ObCreateObjectType(IN PUNICODE_STRING TypeName,
     /* Check if we've already created the directory of types */
     if (ObpTypeDirectoryObject)
     {
-        /* Acquire the directory lock */
-        ObpAcquireDirectoryLockExclusive(ObpTypeDirectoryObject, &Context);
+        /* Lock the lookup context */
+        ObpAcquireLookupContextLock(&Context, ObpTypeDirectoryObject);
 
         /* Do the lookup */
         if (ObpLookupEntryDirectory(ObpTypeDirectoryObject,
@@ -1853,7 +1853,7 @@ NtSetInformationObject(IN HANDLE ObjectHandle,
                     OBP_LOOKUP_CONTEXT LookupContext;
                     ObpInitializeLookupContext(&LookupContext);
 
-                    /* Set its session ID */
+                    /* Set the directory session ID */
                     ObpAcquireDirectoryLockExclusive(Directory, &LookupContext);
                     Directory->SessionId = PsGetCurrentProcessSessionId();
                     ObpReleaseDirectoryLock(Directory, &LookupContext);
diff --git a/ntoskrnl/ob/obname.c b/ntoskrnl/ob/obname.c
index 586561cd9b0..667815d41dc 100644
--- a/ntoskrnl/ob/obname.c
+++ b/ntoskrnl/ob/obname.c
@@ -321,11 +321,9 @@ ObpDeleteNameCheck(IN PVOID Object)
          (ObjectNameInfo->Directory) &&
          !(ObjectHeader->Flags & OB_FLAG_PERMANENT))
     {
-        /* Setup a lookup context */
+        /* Setup a lookup context and lock it */
         ObpInitializeLookupContext(&Context);
-
-        /* Lock the directory */
-        ObpAcquireDirectoryLockExclusive(ObjectNameInfo->Directory, &Context);
+        ObpAcquireLookupContextLock(&Context, ObjectNameInfo->Directory);
 
         /* Do the lookup */
         Object = ObpLookupEntryDirectory(ObjectNameInfo->Directory,
@@ -352,7 +350,7 @@ ObpDeleteNameCheck(IN PVOID Object)
                     ObpDeleteSymbolicLinkName(Object);
                 }
 
-                /* Check if the kernel exclusive is set */
+                /* Check if the kernel exclusive flag is set */
                 ObjectNameInfo = OBJECT_HEADER_TO_NAME_INFO(ObjectHeader);
                 if ((ObjectNameInfo) &&
                     (ObjectNameInfo->QueryReferences & OB_FLAG_KERNEL_EXCLUSIVE))
@@ -843,8 +841,8 @@ ParseFromRoot:
                 /* Check if we are inserting an object */
                 if (InsertObject)
                 {
-                    /* Lock the directory */
-                    ObpAcquireDirectoryLockExclusive(Directory, LookupContext);
+                    /* Lock the lookup context */
+                    ObpAcquireLookupContextLock(LookupContext, Directory);
                 }
             }