Fix crashing bug in finalizer

[wangzq]

CabUnpacker and CabPacker uses a SafeHandle that will callback to C# method when releasing handle. If we don't release the handle in their finalizers, since finalizers from different classes run in nondeterministic orders, this will cause intermittent crash.

Reproducing code:

  private async void button1_Click(object sender, EventArgs e)
  {
     button1.Enabled = false;
     var file = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "small.cab");
     var tasks = Enumerable.Range(0, 1000).Select(_ =>
     {
        return Task.Run(() => TestCab(file));
     });
     await Task.WhenAll(tasks);
     button1.Enabled = true;
  }

  static void TestCab(string file)
  {
     var cab = new CabInfo(file);
     foreach (var fi in cab.GetFiles())
     {
        var sr = new BinaryReader(fi.OpenRead());
        sr.ReadByte();
     }
  }

WiX v3.x pull requests

Active development has now moved to WiX v4.

• Pull requests for new features are extremely unlikely to be accepted for WiX v3.x.
• Pull requests for important bug fixes might be accepted.

Pull requests for WiX v3.x must be approved before they can be reviewed or accepted.

• If an issue doesn't exist for the problem, create one.
• Provide lots of detail so someone just reading the issue can get a good understanding of the problem.
• If possible, attend the biweekly (fortnightly) online meeting for discussion. Meetings are announced on the wix-devs mailing list.

[dnfclas]

All CLA requirements met.

[rseanhall]

I just want to point out that the finalizer should never be called if things are disposed properly. If this test code is supposed to closely resemble production code, then it's a bug in your code that the Stream returned from fi.OpenRead() isn't getting disposed.

That being said, SafeHandles are managed resources and should not be touched while running inside of a finalizer thread. If any Dispose method is getting called with true in a finalizer thread, then that is the root cause and needs to be fixed. This pull request is not the right solution.

[wangzq]

Yes I agree ideally that we should call Dispose proactively. However, I also argue that finalizer is exactly designed to release any resources the user forget to release earlier, the only difference should be the timing not crashing the process. If the handle here is only a normal win32 handle, yes using SafeHandle makes perfect sense and the current code is definitely correct, in which case if the user forgets to call Dispose, the only impact is that the win32 handle will be released later when finalizer runs. Unfortunately the handle's releasing code will call back to a C# method to free memory, which makes this not exactly an unmanaged-only resource. In that sense, using a SafeHandle here is not entirely ideal.

The bottom line is, while the user's code has bug of not disposing eagerly, crashing the process doesn't seem to be an ideal outcome.

[rseanhall]

SafeHandle is a managed class and implements IDisposable. It has its own finalizer that will run. The current code is correct - it should not call the Dispose method on the SafeHandle if it is currently running on the finalizer thread.

Please create an issue and include the crash memory dump. I don't see how the current code could cause a crash inside of the finalizer thread.

[wangzq]

I agree with you that usually we should not need to call SafeHandle.Dispose in finalizer. However, in this case, when we call Destroy from SafeHandle.ReleaseHandle, it will need to callback to a managed method to free memory, this managed method is on another class. Since finalizers from different classes run in nondeterministic order, when SafeHandler's finalizer runs, the managed object that has the callback method may already be gone.

To reproduce, create a winform application and add a button, then paste the code I included. Then find a small cab file and put it in the same folder as the winform application exe, you should be able to reproduce the crashing.

[rseanhall]

Then the solution is to make the target managed method static. As you said, finalizers are run in non-deterministic order. The SafeHandle could have been finalized already before the code in the CabUnpack runs. Calling the Dispose method on that finalized object would be bad.

[wangzq]

Well, since we are checking if the handle is null or not before calling its Dispose method, the scenario you mentioned would not occur. If it does, the repro would have crashed too after making the change I proposed. That said, I agree making the callback static will also work, but I haven't checked if it is feasible to do that -- for example, maybe there are some member variables needed to be used from the callbacks. Please let me know if you prefer me to update my PR to try the static callback method, or you would like to fix this from your side. Thanks!

[rseanhall]

The current method just calls a static method, so that change will be very simple. At this point, it's very unlikely we'll take this in v3. We would need the pull request in https://github.com/wixtoolset/Dtf for wix4, if you don't mind.

[wangzq]

I was trying to fix this by making the callback method static in v4, unfortunately just changing the CabFreeMem into a static method and use it is not enough, I think the CloseStreamHandler may also be used when calling from Finalizer, and this method requires to use other member variables so it is not easy to change into static method.

I want to take a step back and discuss the use of SafeHandle here: is it really the best choice here? The FCI and FDI handles here are using unmanaged resource that need to callback into managed code when releasing, the benefit of using it (to have its built-in implementation of finalizer) is now not only no longer a benefit, but a problem. If we just use a plain handle and do the release ourselves, then since we no longer have two finalizers that may impact each other, we will not have the crashing.

[rseanhall]

I agree that we need to get the native FCI and FDI handle in the same class as those call back methods.

The SafeHandle class is treated differently by the CLR. So my first instinct is to try to move all the callbacks to the NativeMethods.FCI.Handle and NativeMethods.FDI.Handle classes to keep the SafeHandle class's special behavior. If that doesn't work out, then I'd probably make the P/Invoke method return an IntPtr and make CabWorker derive from SafeHandle.

[wangzq]

If you check the example in the doc of SafeHandle, you will see the class using the SafeHandle doesn’t need finalizer. Can we do the same here?