Fix access violation issue in libgpiod v1 driver

src/System.Device.Gpio.Tests/GpioControllerTestBase.cs

@@ -595,6 +595,34 @@ void Callback(object sender, PinValueChangedEventArgs pinValueChangedEventArgs)
         }
     }
 
+    [Fact]
+    public void UsingPinAfterDriverDisposedCausesException()
+    {
+        var controller = new GpioController(PinNumberingScheme.Logical, GetTestDriver());
+        var pin6 = controller.OpenPin(InputPin, PinMode.Input);
+        controller.Dispose();
+        bool correctExceptionSeen = false;
+        try
+        {
+            pin6.Read();
+        }
+        catch (Exception x) when (x is InvalidOperationException || x is ObjectDisposedException)
+        {
+            correctExceptionSeen = true;
+        }
+
+        Assert.True(correctExceptionSeen);
+    }
+
+    [Fact]
+    public void UsingControllerAfterDisposeCausesException()
+    {
+        var controller = new GpioController(PinNumberingScheme.Logical, GetTestDriver());
+        var pin6 = controller.OpenPin(InputPin, PinMode.Input);
+        controller.Dispose();
+        Assert.Throws<ObjectDisposedException>(() => controller.OpenPin(InputPin, PinMode.Input));
+    }
+
     protected abstract GpioDriver GetTestDriver();
     protected abstract PinNumberingScheme GetTestNumberingScheme();
 }

src/System.Device.Gpio.Tests/LibGpiodV1DriverTests.cs

@@ -72,4 +72,36 @@ static void PinChanged(object sender, PinValueChangedEventArgs args)
             gc.UnregisterCallbackForPinValueChangedEvent(InputPin, PinChanged);
         }
     }
+
+    /// <summary>
+    /// Ensure leaking instances of the driver doesn't cause a segfault
+    /// Regression test for https://github.com/dotnet/iot/issues/1849
+    /// </summary>
+    [Fact]
+    public void LeakingDriverDoesNotCrash()
+    {
+        GpioController controller1 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+        controller1.OpenPin(10, PinMode.Output);
+        GpioController controller2 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+        controller2.OpenPin(11, PinMode.Output);
+        GpioController controller3 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+        controller3.OpenPin(12, PinMode.Output);
+        GpioController controller4 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+        controller4.OpenPin(13, PinMode.Output);
+        GpioController controller5 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+        controller5.OpenPin(14, PinMode.Output);
+
+        for (int i = 0; i < 10; i++)
+        {
+            GC.Collect();
+            GpioController controller6 = new GpioController(PinNumberingScheme.Logical, new LibGpiodDriver());
+            controller6.OpenPin(15, PinMode.Output);
+            controller6.ClosePin(15);
+            controller6.Dispose();
+            GC.Collect();
+            Thread.Sleep(20);
+        }
+
+        GC.WaitForPendingFinalizers();
+    }
 }

src/System.Device.Gpio/CompatibilitySuppressions.xml

@@ -13,6 +13,12 @@
     <Left>lib/netstandard2.0/System.Device.Gpio.dll</Left>
     <Right>lib/net6.0-windows10.0.17763/System.Device.Gpio.dll</Right>
   </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:System.Device.Gpio.GpioDriver.Finalize</Target>
+    <Left>lib/netstandard2.0/System.Device.Gpio.dll</Left>
+    <Right>lib/net6.0-windows10.0.17763/System.Device.Gpio.dll</Right>
+  </Suppression>
   <Suppression>
     <DiagnosticId>CP0002</DiagnosticId>
     <Target>M:System.Device.Gpio.PinEventTypes.#ctor</Target>

src/System.Device.Gpio/Interop/Unix/libgpiod/V1/Interop.libgpiod.cs

@@ -81,7 +81,7 @@ static LibgpiodV1()
         /// <param name="offset">The offset of the GPIO line</param>
         /// <returns>Handle to the GPIO line or <see langword="null" /> if an error occurred.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern SafeLineHandle gpiod_chip_get_line(SafeChipHandle chip, int offset);
+        internal static extern IntPtr gpiod_chip_get_line(SafeChipHandle chip, int offset);
 
         /// <summary>
         /// Reserve a single line, set the direction to input.
@@ -90,7 +90,7 @@ static LibgpiodV1()
         /// <param name="consumer">Name of the consumer.</param>
         /// <returns>0 if the line was properly reserved, -1 on failure.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_request_input(SafeLineHandle line, string consumer);
+        internal static extern int gpiod_line_request_input(IntPtr line, string consumer);
 
         /// <summary>
         /// Reserve a single line, set the direction to input with flags
@@ -100,7 +100,7 @@ static LibgpiodV1()
         /// <param name="flags">Additional request flags.</param>
         /// <returns>0 if the line was properly reserved, -1 on failure.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_request_input_flags(SafeLineHandle line, string consumer, int flags);
+        internal static extern int gpiod_line_request_input_flags(IntPtr line, string consumer, int flags);
 
         /// <summary>
         /// Reserve a single line, set the direction to output.
@@ -110,7 +110,7 @@ static LibgpiodV1()
         /// <param name="default_val">Initial value of the line</param>
         /// <returns>0 if the line was properly reserved, -1 on failure.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_request_output(SafeLineHandle line, string consumer, int default_val);
+        internal static extern int gpiod_line_request_output(IntPtr line, string consumer, int default_val);
 
         /// <summary>
         /// Set the value of a single GPIO line.
@@ -119,23 +119,23 @@ static LibgpiodV1()
         /// <param name="value">New value.</param>
         /// <returns>0 if the operation succeeds. In case of an error this routine returns -1 and sets the last error number.</returns>
         [DllImport(LibgpiodLibrary)]
-        internal static extern int gpiod_line_set_value(SafeLineHandle line, int value);
+        internal static extern int gpiod_line_set_value(IntPtr line, int value);
 
         /// <summary>
         /// Read current value of a single GPIO line.
         /// </summary>
         /// <param name="line">GPIO line handle</param>
         /// <returns>0 or 1 if the operation succeeds. On error this routine returns -1 and sets the last error number.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_get_value(SafeLineHandle line);
+        internal static extern int gpiod_line_get_value(IntPtr line);
 
         /// <summary>
         /// Check if line is no used (not set as Input or Output, not listening events).
         /// </summary>
         /// <param name="line">GPIO line handle</param>
         /// <returns>false if pin is used as Input/Output or Listening an event, true if it is free</returns>
         [DllImport(LibgpiodLibrary)]
-        internal static extern bool gpiod_line_is_free(SafeLineHandle line);
+        internal static extern bool gpiod_line_is_free(IntPtr line);
 
         /// <summary>
         /// Release a previously reserved line.
@@ -153,15 +153,15 @@ static LibgpiodV1()
         /// <param name="lineHandle">GPIO line handle</param>
         /// <returns>1 for input, 2 for output</returns>
         [DllImport(LibgpiodLibrary)]
-        internal static extern int gpiod_line_direction(SafeLineHandle lineHandle);
+        internal static extern int gpiod_line_direction(IntPtr lineHandle);
 
         /// <summary>
         /// Read the GPIO line bias setting.
         /// </summary>
         /// <param name="lineHandle">GPIO line handle</param>
         /// <returns>GPIOD_LINE_BIAS_PULL_UP (3), GPIOD_LINE_BIAS_PULL_DOWN (4), GPIOD_LINE_BIAS_DISABLE (2) or GPIOD_LINE_BIAS_UNKNOWN (1). </returns>
         [DllImport(LibgpiodLibrary)]
-        internal static extern int gpiod_line_bias(SafeLineHandle lineHandle);
+        internal static extern int gpiod_line_bias(IntPtr lineHandle);
 
         /// <summary>
         /// Request all event type notifications on a single line.
@@ -170,7 +170,7 @@ static LibgpiodV1()
         /// <param name="consumer">Name of the consumer.</param>
         /// <returns>0 the operation succeeds, -1 on failure.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_request_both_edges_events(SafeLineHandle line, string consumer);
+        internal static extern int gpiod_line_request_both_edges_events(IntPtr line, string consumer);
 
         /// <summary>
         /// Wait for an event on a single line.
@@ -179,7 +179,7 @@ static LibgpiodV1()
         /// <param name="timeout">The TimeSpec to wait for before timing out</param>
         /// <returns>0 if wait timed out, -1 if an error occurred, 1 if an event occurred.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern WaitEventResult gpiod_line_event_wait(SafeLineHandle line, ref TimeSpec timeout);
+        internal static extern WaitEventResult gpiod_line_event_wait(IntPtr line, ref TimeSpec timeout);
 
         /// <summary>
         /// Read the last event from the GPIO line.
@@ -188,7 +188,7 @@ static LibgpiodV1()
         /// <param name="gpioEvent">Reference to the gpio event that was detected</param>
         /// <returns>1 if rising edge event occurred, 2 on falling edge, -1 on error.</returns>
         [DllImport(LibgpiodLibrary, SetLastError = true)]
-        internal static extern int gpiod_line_event_read(SafeLineHandle line, ref GpioLineEvent gpioEvent);
+        internal static extern int gpiod_line_event_read(IntPtr line, ref GpioLineEvent gpioEvent);
 
         /// <summary>
         /// Open a gpiochip by number.

src/System.Device.Gpio/Interop/Unix/libgpiod/V1/SafeLineHandle.cs

@@ -7,31 +7,53 @@
 namespace System.Device.Gpio.Libgpiod.V1;
 
 /// <summary>
-/// Pointer to a pin.
+/// Pointer to a pin (Not a real SafeLineHandle, because we need to align its finalization with the owning object)
 /// </summary>
-internal class SafeLineHandle : SafeHandle
+internal sealed class SafeLineHandle : IDisposable
 {
-    public PinMode PinMode { get; set; }
-
-    public SafeLineHandle()
-        : base(IntPtr.Zero, true)
+    private IntPtr _handle;
+    public SafeLineHandle(IntPtr handle)
     {
+        _handle = handle;
+        PinMode = PinMode.Input;
     }
 
-    protected override bool ReleaseHandle()
+    public PinMode PinMode { get; set; }
+
+    public IntPtr Handle
     {
-        // Contrary to intuition, this does not invalidate the handle (see comment on declaration)
-        LibgpiodV1.gpiod_line_release(handle);
-        return true;
+        get
+        {
+            if (_handle == IntPtr.Zero)
+            {
+                throw new ObjectDisposedException(nameof(SafeLineHandle));
+            }
+
+            return _handle;
+        }
+        set
+        {
+            _handle = value;
+        }
     }
 
     /// <summary>
-    /// Release the lock on the line handle. <see cref="LibgpiodV1.gpiod_line_release"/>
+    /// Release the lock on the line handle. <see cref="Interop.LibgpiodV1.gpiod_line_release"/>
     /// </summary>
     public void ReleaseLock()
     {
-        ReleaseHandle();
+        // Contrary to intuition, this does not invalidate the handle (see comment on declaration)
+        Interop.LibgpiodV1.gpiod_line_release(_handle);
     }
 
-    public override bool IsInvalid => handle == IntPtr.Zero || handle == LibgpiodV1.InvalidHandleValue;
+    public bool IsInvalid => _handle == IntPtr.Zero || _handle == Interop.LibgpiodV1.InvalidHandleValue;
+
+    public void Dispose()
+    {
+        if (_handle != IntPtr.Zero)
+        {
+            Interop.LibgpiodV1.gpiod_line_release(_handle);
+            _handle = IntPtr.Zero;
+        }
+    }
 }

src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/LibGpiodDriver.cs

@@ -144,7 +144,7 @@ public override ComponentInformation QueryComponentInformation()
     /// <inheritdoc/>
     protected override void Dispose(bool disposing)
     {
-        _driver.Dispose();
+        _driver?.Dispose();
         base.Dispose(disposing);
     }
 }

src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1Driver.cs

@@ -116,10 +116,10 @@ private LibGpiodV1DriverEventHandler PopulateEventHandler(int pinNumber)
         {
             _pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle);
 
-            if (pinHandle is null || (pinHandle is object && !LibgpiodV1.gpiod_line_is_free(pinHandle)))
+            if (pinHandle is null || (pinHandle is object && !LibgpiodV1.gpiod_line_is_free(pinHandle.Handle)))
             {
                 pinHandle?.Dispose();
-                pinHandle = LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber);
+                pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber));
                 _pinNumberToSafeLineHandle[pinNumber] = pinHandle;
             }
 
@@ -185,13 +185,13 @@ protected internal override void OpenPin(int pinNumber)
                 return;
             }
 
-            SafeLineHandle pinHandle = LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber);
+            SafeLineHandle pinHandle = new SafeLineHandle(LibgpiodV1.gpiod_chip_get_line(_chip, pinNumber));
             if (pinHandle == null)
             {
                 throw ExceptionHelper.GetIOException(ExceptionResource.OpenPinError, Marshal.GetLastWin32Error());
             }
 
-            int mode = LibgpiodV1.gpiod_line_direction(pinHandle);
+            int mode = LibgpiodV1.gpiod_line_direction(pinHandle.Handle);
             if (mode == 1)
             {
                 pinHandle.PinMode = PinMode.Input;
@@ -203,7 +203,7 @@ protected internal override void OpenPin(int pinNumber)
 
             if (s_isLibgpiodVersion1_5orHigher && pinHandle.PinMode == PinMode.Input)
             {
-                int bias = LibgpiodV1.gpiod_line_bias(pinHandle);
+                int bias = LibgpiodV1.gpiod_line_bias(pinHandle.Handle);
                 if (bias == (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN)
                 {
                     pinHandle.PinMode = PinMode.InputPullDown;
@@ -227,7 +227,7 @@ protected internal override PinValue Read(int pinNumber)
     {
         if (_pinNumberToSafeLineHandle.TryGetValue(pinNumber, out SafeLineHandle? pinHandle))
         {
-            int result = LibgpiodV1.gpiod_line_get_value(pinHandle);
+            int result = LibgpiodV1.gpiod_line_get_value(pinHandle.Handle);
             if (result == -1)
             {
                 throw ExceptionHelper.GetIOException(ExceptionResource.ReadPinError, Marshal.GetLastWin32Error(), pinNumber);
@@ -272,12 +272,12 @@ protected internal override void SetPinMode(int pinNumber, PinMode mode)
             pinHandle.ReleaseLock();
             int requestResult = mode switch
             {
-                PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle, s_consumerName),
-                PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName,
+                PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle.Handle, s_consumerName),
+                PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName,
                         (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN),
-                PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName,
+                PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName,
                         (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP),
-                PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle, s_consumerName, 0),
+                PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle.Handle, s_consumerName, 0),
                 _ => -1,
             };
 
@@ -304,12 +304,12 @@ protected internal override void SetPinMode(int pinNumber, PinMode mode, PinValu
             pinHandle.ReleaseLock();
             int requestResult = mode switch
             {
-                PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle, s_consumerName),
-                PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName,
+                PinMode.Input => LibgpiodV1.gpiod_line_request_input(pinHandle.Handle, s_consumerName),
+                PinMode.InputPullDown => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName,
                     (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_DOWN),
-                PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle, s_consumerName,
+                PinMode.InputPullUp => LibgpiodV1.gpiod_line_request_input_flags(pinHandle.Handle, s_consumerName,
                     (int)RequestFlag.GPIOD_LINE_REQUEST_FLAG_BIAS_PULL_UP),
-                PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle, s_consumerName, initialValue == PinValue.High ? 1 : 0),
+                PinMode.Output => LibgpiodV1.gpiod_line_request_output(pinHandle.Handle, s_consumerName, initialValue == PinValue.High ? 1 : 0),
                 _ => -1,
             };
 
@@ -384,7 +384,7 @@ protected internal override void Write(int pinNumber, PinValue value)
                 pin: pinNumber);
         }
 
-        LibgpiodV1.gpiod_line_set_value(pinHandle, (value == PinValue.High) ? 1 : 0);
+        LibgpiodV1.gpiod_line_set_value(pinHandle.Handle, (value == PinValue.High) ? 1 : 0);
         _pinValue[pinNumber] = value;
     }
 

src/System.Device.Gpio/System/Device/Gpio/Drivers/Libgpiod/V1/LibGpiodV1DriverEventHandler.cs

@@ -36,7 +36,7 @@ public LibGpiodV1DriverEventHandler(int pinNumber, SafeLineHandle safeLineHandle
 
     private void SubscribeForEvent(SafeLineHandle pinHandle)
     {
-        int eventSuccess = LibgpiodV1.gpiod_line_request_both_edges_events(pinHandle, s_consumerName);
+        int eventSuccess = LibgpiodV1.gpiod_line_request_both_edges_events(pinHandle.Handle, s_consumerName);
 
         if (eventSuccess < 0)
         {
@@ -57,7 +57,7 @@ private Task InitializeEventDetectionTask(CancellationToken token, SafeLineHandl
                     TvNsec = new IntPtr(50_000_000)
                 };
 
-                WaitEventResult waitResult = LibgpiodV1.gpiod_line_event_wait(pinHandle, ref timeout);
+                WaitEventResult waitResult = LibgpiodV1.gpiod_line_event_wait(pinHandle.Handle, ref timeout);
                 if (waitResult == WaitEventResult.Error)
                 {
                     var errorCode = Marshal.GetLastWin32Error();
@@ -73,7 +73,7 @@ private Task InitializeEventDetectionTask(CancellationToken token, SafeLineHandl
                 if (waitResult == WaitEventResult.EventOccured)
                 {
                     GpioLineEvent eventResult = new GpioLineEvent();
-                    int checkForEvent = LibgpiodV1.gpiod_line_event_read(pinHandle, ref eventResult);
+                    int checkForEvent = LibgpiodV1.gpiod_line_event_read(pinHandle.Handle, ref eventResult);
                     if (checkForEvent == -1)
                     {
                         throw ExceptionHelper.GetIOException(ExceptionResource.EventReadError, Marshal.GetLastWin32Error());

src/System.Device.Gpio/System/Device/Gpio/GpioController.cs

@@ -233,6 +233,11 @@ public virtual PinMode GetPinMode(int pinNumber)
     /// <returns>The status if the pin is open or closed.</returns>
     public bool IsPinOpen(int pinNumber)
     {
+        if (_driver == null)
+        {
+            throw new ObjectDisposedException(nameof(GpioController));
+        }
+
         return _openPins.ContainsKey(pinNumber);
     }