Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42419

carlossanlop · 2020-09-18T00:49:42Z

This issue was reported for 3.1 twice:

w3wp.exe crash in FileSystemWatcher after 2.2 -> 3.1 migration (c0000005 Access Violation) #40412 - w3wp.exe crash in FileSystemWatcher after 2.2 -> 3.1 migration (c0000005 Access Violation)
FileSystemWatcher crash UNALIGNED_STACK_POINTER #42108 - FileSystemWatcher crash UNALIGNED_STACK_POINTER

When monitoring a network share with FileSystemWatcher, if there are network issues that could cause the data to arrive malformed, then after casting the byte array to a FILE_NOTIFY_INFORMATION, attempts to access the struct fields can throw an AV.

The issue was also reported internally by one of the customers. Our CSS partner was able to reproduce it locally (I couldn't).

I provided him a private binary of System.IO.FileSystem.Watcher.dll with the changes in this fix, and he confirmed the AV no longer happens.

We need this change to be backported to 5.0 and 3.1.

Here is a small repro code, which was executed along with clumsy, which can alter the network quality.

Code

using System;
using System.IO;

namespace FileWatcherConsole
{
  class Program
  {
      static void Main()
      {
          try
          {
              var w = new Watcher();
              w.InitializeFilewatcher();
          }
          catch (Exception ex)
          {
              Console.WriteLine(ex.Message);
              Console.WriteLine(ex.StackTrace);
          }
      }
  }

  class Watcher
  {
      const string WatchPath = @"\\remote\location\goes\here";
      FileSystemWatcher _watcher = new FileSystemWatcher();

      public void InitializeFilewatcher()
      {
          _watcher = new FileSystemWatcher(WatchPath);
          _watcher.InternalBufferSize = 65536;
          _watcher.NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName;
          _watcher.Changed += new FileSystemEventHandler(FileChanged);
          _watcher.Created += new FileSystemEventHandler(FileCreated);
          _watcher.Renamed += new RenamedEventHandler(FileRenamed);
          _watcher.Deleted += new FileSystemEventHandler(FileDeleted);
          _watcher.Error += Watcher_Error;
          _watcher.EnableRaisingEvents = true;
      }

      private void FileRenamed(object sender, RenamedEventArgs e) => Console.WriteLine($"Renamed: {e.OldName} -> {e.Name}");

      private void FileCreated(object sender, FileSystemEventArgs e) => Console.WriteLine($"Created: {e.Name} - {e.FullPath}");

      private void FileChanged(object sender, FileSystemEventArgs e) => Console.WriteLine($"Changed: {e.Name} - {e.FullPath}");

      private void FileDeleted(object sender, FileSystemEventArgs e) => Console.WriteLine($"Deleted: {e.Name} - {e.FullPath}");

      private void Watcher_Error(object sender, ErrorEventArgs e)
      {
          Exception ex = e.GetException();
          Console.WriteLine(ex.Message);
          Console.WriteLine(ex.StackTrace);
      }
  }
}

carlossanlop · 2020-09-18T00:57:32Z

/backport to release/5.0-rc2

github-actions · 2020-09-18T00:58:31Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/260302385

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

carlossanlop

@jkotas @ericstj @danmosemsft I would like to point out that we sent a private DLL to the customer with the changes up to this point, and they got back to us last night confirming that the issue is gone with the fix.

I address almost all feedback I got, but I would like to emphasize that we are actively trying to backport this change to 5.0 and 3.1, so I would like to minimize the number of changes as much as we can.

The FileSystemWatcher unit tests passed locally.

I'm working on getting these suggestions addressed in the 5.0 and 3.1 backports as well.

cc @jeffhandley @tarekgh

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs

jkotas · 2020-09-18T17:35:05Z

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

        {
            Debug.Assert(buffer != null);
            Debug.Assert(buffer.Length > 0);

+            numBytes = Math.Min(numBytes, (uint)buffer.Length);


Do we expect wrong numBytes to be actually passed in, or are we just doing this defensively?

It is done defensively. it was suggested by @ericstj

We should add assert and comment to make it clear.

Defensively in case the number of bytes provided by Windows does not match the length in the buffer.

We had the assert and removed it to add the check :-) I think it is fine to return back the assert.

I think it is fine to return back the assert.

This whole PR's premise is we can't trust the data format being reported by Windows. We can keep the assert, but we need to have the check to protect against potential AV at runtime.

From the same token, we could be asserting in the break statements added below, since those also represent cases which shouldn't happen. I believe @jkotas suggested the same here: https://github.com/dotnet/runtime/pull/42419/files#r491093990

I brought back the assert and I updated the backport PRs to ensure the CI finishes on time before 4pm.

we could be asserting in the break statements added below

The suggestion makes sense to me. Can I add it those asserts in a separate PR?

Fine with me. Also, please add comments like: "This is defensive check in case the number of bytes provided by Windows is wrong. We have not seen this situation in practice.".

jkotas · 2020-09-18T17:37:09Z

we are actively trying to backport this change to 5.0 and 3.1, so I would like to minimize the number of changes as much as we can.

It is not unusual to do a servicing change that has minimal changes and that is coded in less than ideal way; and do a different cleaner change for the tip.

carlossanlop · 2020-09-18T17:51:18Z

It is not unusual to do a servicing change that has minimal changes and that is coded in less than ideal way; and do a different cleaner change for the tip.

I'd be happy to update the backport PRs with the suggestions provided here, if there's enough time after merging this change.

tarekgh · 2020-09-18T17:53:41Z

I'd be happy to update the backport PRs with the suggestions provided here, if there's enough time after merging this change.

I think the changes here is reasonable for the service release too. I am not seeing high risk with it. I am not sure if there is any more strong feedback need to be addressed here.

carlossanlop · 2020-09-18T19:06:45Z

/backport to release/5.0-rc2

github-actions · 2020-09-18T19:07:50Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/261654138

carlossanlop · 2020-09-18T19:29:17Z

/backport to release/5.0-rc2

github-actions · 2020-09-18T19:30:23Z

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/261682407

jozkee

Just nits, hold-off addressing my comments to avoid restarting CI.

jozkee · 2020-09-18T19:20:12Z

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

+                    // 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)


Suggested change

if (info < b || (((byte*)info) + sizeof(Interop.Kernel32.FILE_NOTIFY_INFORMATION)) > pBufferLimit)

if (info < b || ((byte*)info) + sizeof(Interop.Kernel32.FILE_NOTIFY_INFORMATION) > pBufferLimit)

jozkee · 2020-09-18T19:21:00Z

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

+                    }
+
+                    // Verify the file path is within the bounds
+                    byte* pFileEnd = ((byte*)&(info->FileName)) + info->FileNameLength;


create a local for info->FileName and info->FileNameLength since they are also referenced below.

To improve readability or to improve performance? It won't help to improve performance.

To improve readability. I was thinking that it could help both, but it may be worth for the former still.

I have submitted #42474 that shows how this can be rewritten using Span - to both improve readability and improve security.

@jkotas is the intention of that fix to be the backport for 3.1 and 5.0? In that case, I can close my 3 PRs and I can let you submit the backports as well.

I think it is fine to use your original fix for servicing. It is less a bit less invasive.

Okay great. So the only one I'll close is the one for master.

On a second thought, if we want to preserve consistency, I should also merge the master change and your PR can go on top of that.

Merging your PR to master sounds good to me.

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs

carlossanlop · 2020-09-18T22:16:07Z

CI failures:

OSX x64 release and Browser wasm Release - Unrelated to this change. Timeouts while attempting to build.
```
##[error]The job running on agent Azure Pipelines 52 ran longer than the maximum time of 120 minutes.
```

Fix FileSystemWatcher crash due to malformed data coming from Windows.

6583d95

carlossanlop added the area-System.IO label Sep 18, 2020

carlossanlop requested a review from jozkee September 18, 2020 00:49

carlossanlop self-assigned this Sep 18, 2020

carlossanlop requested a review from tarekgh September 18, 2020 00:55

tarekgh approved these changes Sep 18, 2020

View reviewed changes

github-actions bot mentioned this pull request Sep 18, 2020

[release/5.0-rc2] Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42420

Merged

carlossanlop mentioned this pull request Sep 18, 2020

[release/3.1] Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes dotnet/corefx#42989

Merged

danmoseley reviewed Sep 18, 2020

View reviewed changes

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ReadDirectoryChangesW.cs Outdated Show resolved Hide resolved

ericstj reviewed Sep 18, 2020

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs Show resolved Hide resolved

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs Show resolved Hide resolved

jkotas reviewed Sep 18, 2020

View reviewed changes

src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.Win32.cs Show resolved Hide resolved

carlossanlop commented Sep 18, 2020

View reviewed changes

Address PR suggestions.

9765289

jkotas reviewed Sep 18, 2020

View reviewed changes

Bring back assert for numBytes.

7f3817b

jozkee reviewed Sep 18, 2020

View reviewed changes

jeffhandley linked an issue Sep 18, 2020 that may be closed by this pull request

w3wp.exe crash in FileSystemWatcher after 2.2 -> 3.1 migration (c0000005 Access Violation) #40412

Closed

carlossanlop merged commit 2a1735b into dotnet:master Sep 18, 2020

carlossanlop deleted the FileSystemWatcher branch September 18, 2020 23:02

carlossanlop mentioned this pull request Oct 16, 2020

.Net Core 2.2 console application : System.Private.CoreLib System.ArgumentOutOfRangeException (FileSystemWatcher) #36406

Open

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42419

Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42419

carlossanlop commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

carlossanlop left a comment

jkotas Sep 18, 2020

tarekgh Sep 18, 2020

jkotas Sep 18, 2020

carlossanlop Sep 18, 2020

tarekgh Sep 18, 2020

ericstj Sep 18, 2020

carlossanlop Sep 18, 2020

carlossanlop Sep 18, 2020

jkotas Sep 18, 2020

jkotas commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

tarekgh commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

jozkee left a comment

jozkee Sep 18, 2020

jozkee Sep 18, 2020

jkotas Sep 18, 2020

jozkee Sep 18, 2020 •

edited

Loading

jkotas Sep 18, 2020 •

edited

Loading

carlossanlop Sep 18, 2020

jkotas Sep 18, 2020

carlossanlop Sep 18, 2020

carlossanlop Sep 18, 2020

jkotas Sep 18, 2020

carlossanlop commented Sep 18, 2020

	if (info < b \|\| (((byte*)info) + sizeof(Interop.Kernel32.FILE_NOTIFY_INFORMATION)) > pBufferLimit)
	if (info < b \|\| ((byte*)info) + sizeof(Interop.Kernel32.FILE_NOTIFY_INFORMATION) > pBufferLimit)

Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42419

Fix Windows AccessViolationException in FileSystemWatcher when monitoring network share for changes #42419

Conversation

carlossanlop commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

carlossanlop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

tarekgh commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

carlossanlop commented Sep 18, 2020

github-actions bot commented Sep 18, 2020

jozkee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

jkotas Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop commented Sep 18, 2020

jozkee Sep 18, 2020 •

edited

Loading

jkotas Sep 18, 2020 •

edited

Loading