Bugfix ~AcmStreamHeader finalizer exception

[speige]

Fix application crash caused by unallocated GCHandle..Free in ~AcmStreamHeader finalizer. Occurs if corrupted data bytes cause exception during constructor.

[nightblade9]

Interesting. Do you know how to reproduce the crash that this fixes?

[speige]

corruptFile.dmp
new Mp3FileReader(File.ReadAllBytes("corruptFile.dmp"))
The file is not a valid mp3.
DestBuffer = new byte[destBufferLength]; // destBufferLength is a negative value, throwing exception in AcmStreamHeader constructor before hDestBuffer has been allocated.
Exception during a ~finalizer will always crash a .net app with no possibility of try/catch because it's done on a separate GC thread outside user code.

[nightblade9]

Oh, that's very cool. I'll add an automated test to cover it. Thank you for your contribution!

[nightblade9]

I'm not sure how to write a test for this. Both with and without your changes, I see the same OverflowException appear. Below is my test code.

[Test]
        public void Constructor_DoesNotThrow_IfFileIsInvalidOrCorrupted()
        {
            // Arrange
            var bytes = File.ReadAllBytes(Path.Join("TestData", "corruptFile.dmp"));
            var stream = new MemoryStream(bytes);

            // Act/Assert
            Assert.DoesNotThrow(() => new Mp3FileReader(stream));
        }

[speige]

The overflow is not the issue. The ~AcmStreamHeader function calling .Free on an unallocated GCHandle is the issue. Same issue would happen if calling Dispose directly. Tricky thing is you can't call those because you don't have an object if the constructor fails. The exception happens in the GC thread instead. I think theoretically you could add a global unhandled exception handler to see the failure, then you could force GC to run. Not sure if unit tests allow async, but maybe you could have the unit test pause for a few seconds to let the background threads run & use a shared static bool so the unhandled global exception could let the unit test assert after the pause. It might be interesting to search the code base for any other .Free() that isn't wrapped in .IsAllocated, because any others could also throw an exception. Kind of a bug in GCHandle honestly, a common approach is to make a SafeGCHandle wrapper so you don't need to always check it everywhere. It's only especially bad if it's done in a ~Finalizer() method because you can't fix that with a try/catch.