Bug 26127 - SafeHandle types + Delegates + Marshal.GetDelegateForFunctionPointer() == Possible crash
Summary: SafeHandle types + Delegates + Marshal.GetDelegateForFunctionPointer() == Pos...
Status: RESOLVED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: Interop ()
Version: unspecified
Hardware: PC Mac OS
: --- normal
Target Milestone: ---
Assignee: marcos.henrich
URL:
Depends on:
Blocks:
 
Reported: 2015-01-16 17:08 UTC by Jonathan Pryor
Modified: 2015-02-02 04:43 UTC (History)
4 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Jonathan Pryor 2015-01-16 17:08:46 UTC
The function pointer passed to Marshal.GetDelegateForFunctionPointer() appears to be tied to some internal data structures, such that if you create two *incompatible* delegate types around the *same* function pointer, Mono may crash when invoking the delegates. Consider:

Note: App written against OS X and dlopen(2)'s /usr/lib/libc.dylib. Use on other platforms will require fixes.

  using System;
  using System.IO;
  using System.Runtime.InteropServices;
  
  class MyFD : SafeHandle {
  
    public MyFD ()
      : base (new IntPtr (-1), ownsHandle:true)
    {
    }
  
    public override bool IsInvalid {
      get {return base.handle == new IntPtr (-1);}
    }
    
    protected override bool ReleaseHandle ()
    {
      return App.close ((int) handle) == 0;
    }
  }
  
  delegate int IntPtr_Create (string path, int mode);
  delegate IntPtr IntPtr_Write (int fd, byte[] buf, IntPtr bytes);
  delegate MyFD MyFD_Create (string path, int mode);
  delegate IntPtr MyFD_Write (MyFD fd, byte[] buf, IntPtr bytes);
  
  class App {
    [DllImport ("dl")]
    static extern IntPtr dlopen (string path, int mode);
  
    [DllImport ("dl")]
    static extern IntPtr dlsym (IntPtr h, string symbol);
    
    [DllImport ("c")]
    internal static extern int close (int fd);
    
    static void Main (string[] args)
    {
      var libc  = dlopen ("/usr/lib/libc.dylib", 0);
      var creat = dlsym (libc, "creat");
      var write = dlsym (libc, "write");
      int perms = Convert.ToInt32 ("664", 8);
  
      Console.WriteLine ("creat=0x{0}", creat.ToString ("x"));
  
      if (args.Length == 0) {
        Traditional (creat, write, perms);
        Safe (creat, write, perms);      
      } else {
        Safe (creat, write, perms);
        Traditional (creat, write, perms);      
      }
    }
    
    static void Traditional (IntPtr creat, IntPtr write, int perms)
    {  
      var fd_creat = (IntPtr_Create) Marshal.GetDelegateForFunctionPointer (
          creat,
          typeof (IntPtr_Create));
      var fd_write = (IntPtr_Write) Marshal.GetDelegateForFunctionPointer (
          write,
          typeof (IntPtr_Write));
  
      int y = fd_creat ("old.txt", perms);
      fd_write (y, new byte[]{1,2,3}, new IntPtr (3));
      close (y);
    }
    
    static void Safe (IntPtr creat, IntPtr write, int perms)
    {
      var myfd_creat = (MyFD_Create) Marshal.GetDelegateForFunctionPointer (
          creat,
          typeof (MyFD_Create));
      var myfd_write = (MyFD_Write) Marshal.GetDelegateForFunctionPointer (
          write,
          typeof (MyFD_Write));
      
      MyFD x = myfd_creat ("hawtness.txt", perms);
      myfd_write (x, new byte[]{1,2,3}, new IntPtr (3));
      x.Close ();
    }
  }

Run the app without any arguments, and it crashes:

> $ mcs -debug+ shd.cs && mono --debug shd.exe
> creat=0x99d4509a
> 
> Unhandled Exception:
> System.NullReferenceException: Object reference not set to an instance of an object
>   at App.Safe (IntPtr creat, IntPtr write, Int32 perms) [0x0005a] in /Users/jon/tmp/csharp/shd.cs:80 
>   at App.Main (System.String[] args) [0x0005b] in /Users/jon/tmp/csharp/shd.cs:48 

Running the app WITH an argument causes the SafeHandle delegate to be created and invoked before the "traditional" flavor delegates, which allows the app to run without error:

> $ mcs -debug+ shd.cs && mono --debug shd.exe fix
> creat=0x99d4509a
Comment 2 Jonathan Pryor 2015-01-16 17:26:39 UTC
Even though `mono --debug shd.exe fix` doesn't crash, it DOES have an error.

If we modify App.Traditional() to log the return value from close(2):

      int y = fd_creat ("old.txt", perms);
      fd_write (y, new byte[]{1,2,3}, new IntPtr (3));
      int r = close (y);
      Console.WriteLine ("# traditional close({0})={1}", y, r);

We see that the close() call fails:

> $ mcs -debug+ shd.cs && mono --debug shd.exe  fix
> creat=0x99d4509a
> # traditional close(8419456)=-1

Furthermore, we see that we're getting *bizarrely* corrupt data; a file descriptor value of 8419456 is crazypants, and if we don't run the "fix"variant we see that the "normal" Traditional-first+crash scenario results in a file descriptor value of 4, which is much saner.
Comment 3 Jonathan Pryor 2015-01-16 17:32:32 UTC
WORKAROUND: If the delegate types come from different assembles, it works.

  // File: ds.cs:
  using System;

  public delegate int IntPtr_Create (string path, int mode);
  public delegate IntPtr IntPtr_Write (int fd, byte[] buf, IntPtr bytes);

  $ mcs -t:library ds.cs

Then edit Comment #0 to *remove* the declarations for IntPtr_Create and IntPtr_Write, and modify the compile command to reference ds.dll:

> $ mcs -debug+ shd.cs -r:ds.dll && mono --debug shd.exe  fix
> creat=0x99d4509a
> # traditional close(4)=0

We now get a properly sane file descriptor in the "fix" case, and the default case no longer crashes:

> $ mcs -debug+ shd.cs -r:ds.dll && mono --debug shd.exe 
> creat=0x99d4509a
> # traditional close(4)=0
Comment 4 Rodrigo Kumpera 2015-01-16 17:49:57 UTC
Hey Zoltan,

Can I assign it to Marcos? He's going to work on SafeHandles.
Comment 5 Zoltan Varga 2015-01-16 18:42:26 UTC
Sure.
Comment 6 Rodrigo Kumpera 2015-01-16 18:49:58 UTC
Hi Marcos,

Jon just his one of those cases where SafeHandles fail on mono.

Once you're confortable on the subject, could you take a look on this one?
Comment 7 marcos.henrich 2015-01-30 11:27:03 UTC
Hey,

It was not SafeHandle related after all.
The problem was in how mono_marshal_get_native_func_wrapper was using the cache.
Only one wrapper was created per native function.

The pull request for this issue can be found in the link below.
https://github.com/mono/mono/pull/1540
Comment 8 marcos.henrich 2015-02-02 04:43:41 UTC
Fixed in master 3edd4ffe6bf35433a2285b6e65606e45946a8ebe.
https://github.com/mono/mono/commit/3edd4ffe6bf35433a2285b6e65606e45946a8ebe