Windows: AFSInitDirEntry allocated ObjInfoCBs valid
authorJeffrey Altman <jaltman@your-file-system.com>
Fri, 12 Apr 2013 14:58:47 +0000 (10:58 -0400)
committerJeffrey Altman <jaltman@your-file-system.com>
Fri, 26 Apr 2013 13:23:00 +0000 (06:23 -0700)
In AFSInitDirEntry the pattern was to find or allocate an
ObjectInfoCB then destroy it if the DirectoryCB creation fails
for some reason.  The problem with this approach is that once the
VolumeCB ObjectInfoTree.TreeLock is dropped the ObjectInfoCB is findable.
That means that the contents of the ObjectInfoCB must be valid.

This patchset makes three changes.  First, in the case where the
ObjectInfoCB is allocated, the fields of the ObjectInfoCB are populated
from the DirEnumEntry before the TreeLock is dropped.  Second, if the
DirectoryCB allocation fails the ObjectInfoCB is not deleted.  It is
perfectly valid and can be used by a subsequent AFSInitDirEntry call.
Perhaps one that is racing with this thread.   It will eventually be
cleaned up by the AFSPrimaryVolumeWorkerThread.  Finally, when the
ObjectInfoCB reference count is decremented the TreeLock is held shared in
order to prevent races with other threads that might be incrementing it
themselves.

Change-Id: If3091d4fa640bbb614a1a405c3afc910d649aad6
Reviewed-on: http://gerrit.openafs.org/9781
Reviewed-by: Rod Widdowson <rdw@steadingsoftware.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>

src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp

index cdcb70f..3acfa8e 100644 (file)
@@ -1002,7 +1002,6 @@ AFSInitDirEntry( IN AFSObjectInfoCB *ParentObjectInfo,
     NTSTATUS ntStatus = STATUS_SUCCESS;
     ULONG ulEntryLength = 0;
     AFSObjectInfoCB *pObjectInfoCB = NULL;
-    BOOLEAN bAllocatedObjectCB = FALSE;
     ULONGLONG ullIndex = 0;
     AFSNonPagedDirectoryCB *pNonPagedDirEntry = NULL;
     LONG lCount;
@@ -1033,8 +1032,7 @@ AFSInitDirEntry( IN AFSObjectInfoCB *ParentObjectInfo,
                                        ullIndex,
                                        (AFSBTreeEntry **)&pObjectInfoCB);
 
-        if( !NT_SUCCESS( ntStatus) ||
-            pObjectInfoCB == NULL)
+        if( !NT_SUCCESS( ntStatus))
         {
 
             //
@@ -1052,14 +1050,99 @@ AFSInitDirEntry( IN AFSObjectInfoCB *ParentObjectInfo,
                 try_return( ntStatus = STATUS_INSUFFICIENT_RESOURCES);
             }
 
-            bAllocatedObjectCB = TRUE;
-
             AFSDbgTrace(( AFS_SUBSYSTEM_FILE_PROCESSING,
                           AFS_TRACE_LEVEL_VERBOSE,
                           "AFSInitDirEntry initialized object %p Parent Object %p for %wZ\n",
                           pObjectInfoCB,
                           ParentObjectInfo,
                           FileName));
+
+            //
+            // If we allocated the object information cb then set the information
+            //
+
+            pObjectInfoCB->FileId = DirEnumEntry->FileId;
+
+            pObjectInfoCB->TargetFileId = DirEnumEntry->TargetFileId;
+
+            pObjectInfoCB->FileType = DirEnumEntry->FileType;
+
+            pObjectInfoCB->FileAttributes = DirEnumEntry->FileAttributes;
+
+            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_MOUNTPOINT ||
+                pObjectInfoCB->FileType == AFS_FILE_TYPE_DFSLINK)
+            {
+
+                pObjectInfoCB->FileAttributes |= (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT);
+            }
+
+            if (pObjectInfoCB->FileType == AFS_FILE_TYPE_SYMLINK)
+            {
+
+                if ( pObjectInfoCB->FileAttributes == FILE_ATTRIBUTE_NORMAL)
+                {
+
+                    pObjectInfoCB->FileAttributes = FILE_ATTRIBUTE_REPARSE_POINT;
+                }
+                else
+                {
+
+                    pObjectInfoCB->FileAttributes |= FILE_ATTRIBUTE_REPARSE_POINT;
+                }
+            }
+
+            //
+            // Check for the case where we have a filetype of SymLink but both the TargetFid and the
+            // TargetName are empty. In this case set the filetype to zero so we evaluate it later in
+            // the code
+            //
+
+            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_SYMLINK &&
+                pObjectInfoCB->TargetFileId.Vnode == 0 &&
+                pObjectInfoCB->TargetFileId.Unique == 0 &&
+                (TargetName == NULL || TargetName->Length == 0))
+            {
+
+                //
+                // This will ensure we perform a validation on the node
+                //
+
+                pObjectInfoCB->FileType = AFS_FILE_TYPE_UNKNOWN;
+            }
+
+            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_UNKNOWN)
+            {
+
+                SetFlag( pObjectInfoCB->Flags, AFS_OBJECT_FLAGS_NOT_EVALUATED);
+            }
+
+            SetFlag( pObjectInfoCB->Flags, AFS_OBJECT_FLAGS_VERIFY);
+        }
+
+        if ( BooleanFlagOn( pObjectInfoCB->Flags, AFS_OBJECT_FLAGS_VERIFY))
+        {
+
+            pObjectInfoCB->CreationTime = DirEnumEntry->CreationTime;
+
+            pObjectInfoCB->LastAccessTime = DirEnumEntry->LastAccessTime;
+
+            pObjectInfoCB->LastWriteTime = DirEnumEntry->LastWriteTime;
+
+            pObjectInfoCB->ChangeTime = DirEnumEntry->ChangeTime;
+
+            pObjectInfoCB->EndOfFile = DirEnumEntry->EndOfFile;
+
+            pObjectInfoCB->AllocationSize = DirEnumEntry->AllocationSize;
+
+            pObjectInfoCB->EaSize = DirEnumEntry->EaSize;
+
+            pObjectInfoCB->Links = DirEnumEntry->Links;
+
+            pObjectInfoCB->Expiration = DirEnumEntry->Expiration;
+
+            pObjectInfoCB->DataVersion = DirEnumEntry->DataVersion;
+
+            ClearFlag( pObjectInfoCB->Flags, AFS_OBJECT_FLAGS_VERIFY);
         }
 
         //
@@ -1189,96 +1272,6 @@ AFSInitDirEntry( IN AFSObjectInfoCB *ParentObjectInfo,
                            pDirNode->NameInformation.TargetName.Length);
         }
 
-        //
-        // If we allocated the object information cb then update the information
-        //
-
-        if( bAllocatedObjectCB)
-        {
-
-            //
-            // Populate the rest of the data
-            //
-
-            pObjectInfoCB->FileId = DirEnumEntry->FileId;
-
-            pObjectInfoCB->TargetFileId = DirEnumEntry->TargetFileId;
-
-            pObjectInfoCB->FileType = DirEnumEntry->FileType;
-
-            pObjectInfoCB->CreationTime = DirEnumEntry->CreationTime;
-
-            pObjectInfoCB->LastAccessTime = DirEnumEntry->LastAccessTime;
-
-            pObjectInfoCB->LastWriteTime = DirEnumEntry->LastWriteTime;
-
-            pObjectInfoCB->ChangeTime = DirEnumEntry->ChangeTime;
-
-            pObjectInfoCB->EndOfFile = DirEnumEntry->EndOfFile;
-
-            pObjectInfoCB->AllocationSize = DirEnumEntry->AllocationSize;
-
-            pObjectInfoCB->FileAttributes = DirEnumEntry->FileAttributes;
-
-            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_MOUNTPOINT ||
-                pObjectInfoCB->FileType == AFS_FILE_TYPE_DFSLINK)
-            {
-
-                pObjectInfoCB->FileAttributes |= (FILE_ATTRIBUTE_DIRECTORY | FILE_ATTRIBUTE_REPARSE_POINT);
-            }
-
-            if (pObjectInfoCB->FileType == AFS_FILE_TYPE_SYMLINK)
-            {
-
-                if ( pObjectInfoCB->FileAttributes == FILE_ATTRIBUTE_NORMAL)
-                {
-
-                    pObjectInfoCB->FileAttributes = FILE_ATTRIBUTE_REPARSE_POINT;
-                }
-                else
-                {
-
-                    pObjectInfoCB->FileAttributes |= FILE_ATTRIBUTE_REPARSE_POINT;
-                }
-            }
-
-            pObjectInfoCB->EaSize = DirEnumEntry->EaSize;
-
-            //
-            // Check for the case where we have a filetype of SymLink but both the TargetFid and the
-            // TargetName are empty. In this case set the filetype to zero so we evaluate it later in
-            // the code
-            //
-
-            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_SYMLINK &&
-                pObjectInfoCB->TargetFileId.Vnode == 0 &&
-                pObjectInfoCB->TargetFileId.Unique == 0 &&
-                pDirNode->NameInformation.TargetName.Length == 0)
-            {
-
-                //
-                // This will ensure we perform a validation on the node
-                //
-
-                pObjectInfoCB->FileType = AFS_FILE_TYPE_UNKNOWN;
-            }
-
-            if( pObjectInfoCB->FileType == AFS_FILE_TYPE_UNKNOWN)
-            {
-
-                SetFlag( pObjectInfoCB->Flags, AFS_OBJECT_FLAGS_NOT_EVALUATED);
-            }
-        }
-
-        //
-        // Object specific information
-        //
-
-        pObjectInfoCB->Links = DirEnumEntry->Links;
-
-        pObjectInfoCB->Expiration = DirEnumEntry->Expiration;
-
-        pObjectInfoCB->DataVersion = DirEnumEntry->DataVersion;
 
 try_exit:
 
@@ -1313,6 +1306,9 @@ try_exit:
             if( pObjectInfoCB != NULL)
             {
 
+                AFSAcquireShared( pObjectInfoCB->VolumeCB->ObjectInfoTree.TreeLock,
+                                  TRUE);
+
                 lCount = AFSObjectInfoDecrement( pObjectInfoCB,
                                                  AFS_OBJECT_REFERENCE_DIRENTRY);
 
@@ -1322,12 +1318,7 @@ try_exit:
                               pObjectInfoCB,
                               lCount));
 
-                if( bAllocatedObjectCB &&
-                    lCount == 0)
-                {
-
-                    AFSDeleteObjectInfo( &pObjectInfoCB);
-                }
+                AFSReleaseResource( pObjectInfoCB->VolumeCB->ObjectInfoTree.TreeLock);
             }
         }
     }