Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mat from managed array unpinned while submats exist #1689

Closed
Austin-Kearns opened this issue Aug 8, 2024 · 0 comments · Fixed by #1691
Closed

Mat from managed array unpinned while submats exist #1689

Austin-Kearns opened this issue Aug 8, 2024 · 0 comments · Fixed by #1691

Comments

@Austin-Kearns
Copy link

Summary of your issue

Managed arrays are unpinned when the original Mat is disposed or finalized. Captured submat data pointers are not updated on subsequent GC compaction, causing AccessViolationExceptions. Mats created by the following constructors are affected:

public Mat(int rows, int cols, MatType type, Array data, long step = 0) { ... }

public Mat(IEnumerable<int> sizes, MatType type, Array data, IEnumerable<long>? steps = null) { ... }

This is caused by the GCHandle DataHandle lifetime management of the DisposableObject parent class and affects all Mat instances which are a submat of the original Mat once the original is disposed or finalized. Both problematic constructors call the DisposableObject.AllocGCHandle method which sets the DataHandle property of the parent DisposableObject class. However, upon either disposal or finalization the DisposeUnmanaged() method is called, which runs the following code to dispose of the GCHandle:

protected virtual void DisposeUnmanaged()
{
    if (DataHandle.IsAllocated)
    {
        DataHandle.Free();
    }
    ...
}

Thus, the source array is unpinned even when submats with references to the current array location are present. The GC may then move the array, causing incorrect memory access or an AccessViolationException when using the captured submatrices.

Environment

All environments

What did you do when you faced the problem?

We are currently using NET8 [UnsafeAccessor] to query the DataHandle.IsAllocated property and create a copy whenever we see this, but this isn't a valid solution. The array GCHandle needs a proper lifetime management and reference counting implementation. To avoid affecting other classes descendant from DisposableObject, we could remove the GCHandle management by the base class and manage it in the Mat class itself. Since this only affects submatrices, a sample solution could be:

  1. Manage the reference count somehow. Done in a helper class below.
/// <summary>
/// Pins an array and unpins it when all references are released.
/// </summary>
internal class ArrayPinningLifetime : IDisposable
{
    private GCHandle _handle;
    private int _refCount;

    public ArrayPinningLifetime(Array array)
    {
        _handle = GCHandle.Alloc(array, GCHandleType.Pinned);
    }

    public IntPtr Data => _handle.IsAllocated ? _handle.AddrOfPinnedObject() : throw new ObjectDisposedException(nameof(ArrayPinningLifetime));

    public ArrayPinningLifetime Ref()
    {
        Interlocked.Increment(ref _refCount);
        return this;
    }

    public void Dispose()
    {
        if (Interlocked.Decrement(ref _refCount) != 0 || !_handle.IsAllocated)
        {
            return;
        }

        _handle.Free();
        GC.SuppressFinalize(this);
    }

    ~ArrayPinningLifetime()
    {
        if (_handle.IsAllocated)
        {
            _handle.Free();
        }
    }
}
  1. Update the Mat class to contain the reference counting logic and maintain a reference for all submatrices.
public class Mat
{
    ...
    private ArrayPinningLifetime? _pinLifetime;

    public Mat SubMat(int rowStart, int rowEnd, int colStart, int colEnd)
    {
        ThrowIfDisposed();
        NativeMethods.HandleException(
            NativeMethods.core_Mat_subMat1(ptr, rowStart, rowEnd, colStart, colEnd, out var ret));
        GC.KeepAlive(this);
        var retVal = new Mat(ret);
        // Keep the array pinned as long as the Mat is alive
        retVal._pinLifetime = _pinLifetime?.Ref();
        return retVal;
    }

    protected override void DisposeManaged() => _pinLifetime.Dispose();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant