From 2a1735b162437bb35a112014092a834c2496c3e3 Mon Sep 17 00:00:00 2001
From: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
Date: Fri, 18 Sep 2020 16:01:58 -0700
Subject: [PATCH] Fix Windows AccessViolationException in FileSystemWatcher
 when monitoring network share for changes (#42419)

* Fix FileSystemWatcher crash due to malformed data coming from Windows.

* Address PR suggestions.

* Bring back assert for numBytes.

Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>
---
 .../Kernel32/Interop.ReadDirectoryChangesW.cs |  9 +----
 .../src/System/IO/FileSystemWatcher.Win32.cs  | 37 +++++++++++++++----
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs
index 3790ca3c10ef7..95cf5d06facd7 100644
--- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs
+++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs
@@ -28,13 +28,8 @@ internal struct FILE_NOTIFY_INFORMATION
             internal FileAction Action;
 
             // Note that the file name is not null terminated
-            private uint FileNameLength;
-            private char _FileName;
-
-            internal unsafe ReadOnlySpan<char> FileName
-            {
-                get { fixed (char* c = &_FileName) { return new ReadOnlySpan<char>(c, (int)FileNameLength / sizeof(char)); } }
-            }
+            internal readonly uint FileNameLength;
+            internal readonly char FileName;
         }
 
         internal enum FileAction : uint
diff --git a/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs b/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs
index 26e2ee6308553..e2c3756f084fe 100644
--- a/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs
+++ b/src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs
@@ -235,7 +235,7 @@ private void ReadDirectoryChangesCallback(uint errorCode, uint numBytes, AsyncRe
                 }
                 else
                 {
-                    ParseEventBufferAndNotifyForEach(state.Buffer);
+                    ParseEventBufferAndNotifyForEach(state.Buffer, numBytes);
                 }
             }
             finally
@@ -245,13 +245,18 @@ private void ReadDirectoryChangesCallback(uint errorCode, uint numBytes, AsyncRe
             }
         }
 
-        private unsafe void ParseEventBufferAndNotifyForEach(byte[] buffer)
+        private unsafe void ParseEventBufferAndNotifyForEach(byte[] buffer, uint numBytes)
         {
             Debug.Assert(buffer != null);
             Debug.Assert(buffer.Length > 0);
+            Debug.Assert(numBytes <= buffer.Length);
+
+            numBytes = Math.Min(numBytes, (uint)buffer.Length);
 
             fixed (byte* b = buffer)
             {
+                byte* pBufferLimit = b + numBytes;
+
                 Interop.Kernel32.FILE_NOTIFY_INFORMATION* info = (Interop.Kernel32.FILE_NOTIFY_INFORMATION*)b;
 
                 ReadOnlySpan<char> oldName = ReadOnlySpan<char>.Empty;
@@ -259,6 +264,22 @@ private unsafe void ParseEventBufferAndNotifyForEach(byte[] buffer)
                 // Parse each event from the buffer and notify appropriate delegates
                 do
                 {
+                    // Validate the data we received in case it's corrupted. This can happen
+                    // if we are watching files over the network with a poor connection.
+
+                    // Verify the info object is within the expected bounds
+                    if (info < b || (((byte*)info) + sizeof(Interop.Kernel32.FILE_NOTIFY_INFORMATION)) > pBufferLimit)
+                    {
+                        break;
+                    }
+
+                    // Verify the file path is within the bounds
+                    byte* pFileEnd = ((byte*)&(info->FileName)) + info->FileNameLength;
+                    if (pFileEnd < &(info->FileName) || pFileEnd > pBufferLimit)
+                    {
+                        break;
+                    }
+
                     // A slightly convoluted piece of code follows.  Here's what's happening:
                     //
                     // We wish to collapse the poorly done rename notifications from the
@@ -284,17 +305,19 @@ private unsafe void ParseEventBufferAndNotifyForEach(byte[] buffer)
                     //
                     // (Phew!)
 
+                    ReadOnlySpan<char> fileName = new ReadOnlySpan<char>(&(info->FileName), (int)info->FileNameLength / sizeof(char));
+
                     switch (info->Action)
                     {
                         case Interop.Kernel32.FileAction.FILE_ACTION_RENAMED_OLD_NAME:
                             // Action is renamed from, save the name of the file
-                            oldName = info->FileName;
+                            oldName = fileName;
                             break;
                         case Interop.Kernel32.FileAction.FILE_ACTION_RENAMED_NEW_NAME:
                             // oldName may be empty if we didn't receive FILE_ACTION_RENAMED_OLD_NAME first
                             NotifyRenameEventArgs(
                                 WatcherChangeTypes.Renamed,
-                                info->FileName,
+                                fileName,
                                 oldName);
                             oldName = ReadOnlySpan<char>.Empty;
                             break;
@@ -309,13 +332,13 @@ private unsafe void ParseEventBufferAndNotifyForEach(byte[] buffer)
                             switch (info->Action)
                             {
                                 case Interop.Kernel32.FileAction.FILE_ACTION_ADDED:
-                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Created, info->FileName);
+                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Created, fileName);
                                     break;
                                 case Interop.Kernel32.FileAction.FILE_ACTION_REMOVED:
-                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Deleted, info->FileName);
+                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Deleted, fileName);
                                     break;
                                 case Interop.Kernel32.FileAction.FILE_ACTION_MODIFIED:
-                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Changed, info->FileName);
+                                    NotifyFileSystemEventArgs(WatcherChangeTypes.Changed, fileName);
                                     break;
                                 default:
                                     Debug.Fail($"Unknown FileSystemEvent action type!  Value: {info->Action}");