From 410ac155c38671d91fdb868a01df84c88928732f Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Fri, 14 Sep 2018 15:20:54 -0400 Subject: [PATCH 01/10] UNI-63112: unit test for the domain reload crash When this test is not ignored, we get a crash. The bug fixes are coming soon (there's three bugs). --- src/embed_tests/Python.EmbeddingTest.csproj | 3 +- src/embed_tests/TestDomainReload.cs | 224 ++++++++++++++++++++ 2 files changed, 226 insertions(+), 1 deletion(-) create mode 100644 src/embed_tests/TestDomainReload.cs diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index 66e8c7165..d16ca4ba4 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -86,6 +86,7 @@ + @@ -122,4 +123,4 @@ - \ No newline at end of file + diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs new file mode 100644 index 000000000..b842d911c --- /dev/null +++ b/src/embed_tests/TestDomainReload.cs @@ -0,0 +1,224 @@ +using System; +using System.CodeDom.Compiler; +using System.Reflection; +using NUnit.Framework; + +namespace Python.EmbeddingTest +{ + class TestDomainReload + { + /// + /// Test that the python runtime can survive a C# domain reload without crashing. + /// + /// At the time this test was written, there was a very annoying + /// seemingly random crash bug when integrating pythonnet into Unity. + /// + /// The repro steps we that David Lassonde, Viktoria Kovecses and + /// Benoit Hudson eventually worked out: + /// 1. Write a HelloWorld.cs script that uses Python.Runtime to access + /// some C# data from python: C# calls python, which calls C#. + /// 2. Execute the script (e.g. make it a MenuItem and click it). + /// 3. Touch HelloWorld.cs on disk, forcing Unity to recompile scripts. + /// 4. Wait several seconds for Unity to be done recompiling and + /// reloading the C# domain. + /// 5. Make python run the gc (e.g. by calling gc.collect()). + /// + /// The reason: + /// A. In step 2, Python.Runtime registers a bunch of new types with + /// their tp_traverse slot pointing to managed code, and allocates + /// some objects of those types. + /// B. In step 4, Unity unloads the C# domain. That frees the managed + /// code. But at the time of the crash investigation, pythonnet + /// leaked the python side of the objects allocated in step 1. + /// C. In step 5, python sees some pythonnet objects in its gc list of + /// potentially-leaked objects. It calls tp_traverse on those objects. + /// But tp_traverse was freed in step 3 => CRASH. + /// + /// This test distills what's going on without needing Unity around (we'd see + /// similar behaviour if we were using pythonnet on a .NET web server that did + /// a hot reload). + /// + [Test] + [Ignore("Test crashes")] + public static void DomainReloadAndGC() + { + // We're set up to run in the directory that includes the bin directory. + System.IO.Directory.SetCurrentDirectory(AppDomain.CurrentDomain.BaseDirectory); + + Assembly pythonRunner1 = BuildAssembly("test1"); + RunAssemblyAndUnload(pythonRunner1, "test1"); + + // This caused a crash because objects allocated in pythonRunner1 + // still existed in memory, but the code to do python GC on those + // objects is gone. + Assembly pythonRunner2 = BuildAssembly("test2"); + RunAssemblyAndUnload(pythonRunner2, "test2"); + } + + // + // The code we'll test. All that really matters is + // using GIL { Python.Exec(pyScript); } + // but the rest is useful for debugging. + // + // What matters in the python code is gc.collect and clr.AddReference. + // + const string TestCode = @" + using Python.Runtime; + using System; + class PythonRunner { + public static void RunPython() { + AppDomain.CurrentDomain.DomainUnload += OnDomainUnload; + string name = AppDomain.CurrentDomain.FriendlyName; + Console.WriteLine($""[{name} in .NET] In PythonRunner.RunPython""); + using (Py.GIL()) { + try { + var pyScript = ""import clr\n"" + + $""print('[{name} in python] imported clr')\n"" + + ""clr.AddReference('System')\n"" + + $""print('[{name} in python] allocated a clr object')\n"" + + ""import gc\n"" + + ""gc.collect()\n"" + + $""print('[{name} in python] collected garbage')\n""; + PythonEngine.Exec(pyScript); + } catch(Exception e) { + Console.WriteLine($""[{name} in .NET] Caught exception: {e}""); + } + } + } + static void OnDomainUnload(object sender, EventArgs e) { + System.Console.WriteLine(string.Format($""[{AppDomain.CurrentDomain.FriendlyName} in .NET] unloading"")); + } + }"; + + + /// + /// Build an assembly out of the source code above. + /// + /// This creates a file .dll in order + /// to support the statement "proxy.theAssembly = assembly" below. + /// That statement needs a file, can't run via memory. + /// + static Assembly BuildAssembly(string assemblyName) + { + var provider = CodeDomProvider.CreateProvider("CSharp"); + + var compilerparams = new CompilerParameters(); + compilerparams.ReferencedAssemblies.Add("Python.Runtime.dll"); + compilerparams.GenerateExecutable = false; + compilerparams.GenerateInMemory = false; + compilerparams.IncludeDebugInformation = false; + compilerparams.OutputAssembly = assemblyName; + + var results = provider.CompileAssemblyFromSource(compilerparams, TestCode); + if (results.Errors.HasErrors) + { + var errors = new System.Text.StringBuilder("Compiler Errors:\n"); + foreach (CompilerError error in results.Errors) + { + errors.AppendFormat("Line {0},{1}\t: {2}\n", + error.Line, error.Column, error.ErrorText); + } + throw new Exception(errors.ToString()); + } + else + { + return results.CompiledAssembly; + } + } + + /// + /// This is a magic incantation required to run code in an application + /// domain other than the current one. + /// + class Proxy : MarshalByRefObject + { + Assembly theAssembly = null; + + public void InitAssembly(string assemblyPath) + { + theAssembly = Assembly.LoadFile(assemblyPath); + } + + public void RunPython() + { + Console.WriteLine("[Proxy] Entering RunPython"); + + // Call into the new assembly. Will execute Python code + var pythonrunner = theAssembly.GetType("PythonRunner"); + var runPythonMethod = pythonrunner.GetMethod("RunPython"); + runPythonMethod.Invoke(null, new object[] { }); + + Console.WriteLine("[Proxy] Leaving RunPython"); + } + } + + /// + /// Create a domain, run the assembly in it (the RunPython function), + /// and unload the domain. + /// + static void RunAssemblyAndUnload(Assembly assembly, string assemblyName) + { + Console.WriteLine($"[Program.Main] === creating domain for assembly {assembly.FullName}"); + + // Create the domain. Make sure to set PrivateBinPath to a relative + // path from the CWD (namely, 'bin'). + // See https://stackoverflow.com/questions/24760543/createinstanceandunwrap-in-another-domain + var currentDomain = AppDomain.CurrentDomain; + var domainsetup = new AppDomainSetup() + { + ApplicationBase = currentDomain.SetupInformation.ApplicationBase, + ConfigurationFile = currentDomain.SetupInformation.ConfigurationFile, + LoaderOptimization = LoaderOptimization.SingleDomain, + PrivateBinPath = "." + }; + var domain = AppDomain.CreateDomain( + $"My Domain {assemblyName}", + currentDomain.Evidence, + domainsetup); + + // Create a Proxy object in the new domain, where we want the + // assembly (and Python .NET) to reside + Type type = typeof(Proxy); + System.IO.Directory.SetCurrentDirectory(AppDomain.CurrentDomain.BaseDirectory); + var theProxy = (Proxy)domain.CreateInstanceAndUnwrap( + type.Assembly.FullName, + type.FullName); + + // From now on use the Proxy to call into the new assembly + theProxy.InitAssembly(assemblyName); + theProxy.RunPython(); + + Console.WriteLine($"[Program.Main] Before Domain Unload on {assembly.FullName}"); + AppDomain.Unload(domain); + Console.WriteLine($"[Program.Main] After Domain Unload on {assembly.FullName}"); + + // Validate that the assembly does not exist anymore + try + { + Console.WriteLine($"[Program.Main] The Proxy object is valid ({theProxy}). Unexpected domain unload behavior"); + } + catch (Exception) + { + Console.WriteLine("[Program.Main] The Proxy object is not valid anymore, domain unload complete."); + } + } + + /// + /// Resolves the assembly. Why doesn't this just work normally? + /// + static Assembly ResolveAssembly(object sender, ResolveEventArgs args) + { + var loadedAssemblies = AppDomain.CurrentDomain.GetAssemblies(); + + foreach (var assembly in loadedAssemblies) + { + if (assembly.FullName == args.Name) + { + return assembly; + } + } + + return null; + } + } +} From c07fff02f4fe30a725e8df1b3aaab3aa2a87ff97 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Fri, 14 Sep 2018 18:08:50 -0400 Subject: [PATCH 02/10] UNI-62864: shutdown on domain reload. Shut down python when the domain that loaded python reloads. --- src/embed_tests/TestDomainReload.cs | 4 ++++ src/runtime/pythonengine.cs | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs index b842d911c..3a91e84ae 100644 --- a/src/embed_tests/TestDomainReload.cs +++ b/src/embed_tests/TestDomainReload.cs @@ -2,6 +2,7 @@ using System.CodeDom.Compiler; using System.Reflection; using NUnit.Framework; +using Python.Runtime; namespace Python.EmbeddingTest { @@ -48,6 +49,9 @@ public static void DomainReloadAndGC() Assembly pythonRunner1 = BuildAssembly("test1"); RunAssemblyAndUnload(pythonRunner1, "test1"); + // Verify that python is not initialized even though we ran it. + Assert.That(Runtime.Runtime.Py_IsInitialized(), Is.Zero); + // This caused a crash because objects allocated in pythonRunner1 // still existed in memory, but the code to do python GC on those // objects is gone. diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs index a23c7ac79..8ab44d041 100644 --- a/src/runtime/pythonengine.cs +++ b/src/runtime/pythonengine.cs @@ -220,9 +220,17 @@ public static void Initialize(IEnumerable args, bool setSysArgv = true) { locals.Dispose(); } + + // Make sure we clean up properly on app domain unload. + AppDomain.CurrentDomain.DomainUnload += OnDomainUnload; } } + static void OnDomainUnload(object _, EventArgs __) + { + Shutdown(); + } + /// /// A helper to perform initialization from the context of an active /// CPython interpreter process - this bootstraps the managed runtime @@ -303,6 +311,8 @@ public static void Shutdown() _pythonPath = IntPtr.Zero; Runtime.Shutdown(); + + AppDomain.CurrentDomain.DomainUnload -= OnDomainUnload; initialized = false; } } From d016b24ef34eccc9fd67460420cc09275c3caf48 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Wed, 26 Sep 2018 03:50:21 -0400 Subject: [PATCH 03/10] Drive-by improve a hashtable to hashset. Also added comments explaining what the hashset is about. --- src/runtime/typemanager.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index 6570ee083..db46c310f 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -454,7 +454,10 @@ internal static IntPtr AllocateTypeObject(string name) /// internal static void InitializeSlots(IntPtr type, Type impl) { - var seen = new Hashtable(8); + // We work from the most-derived class up; make sure to get + // the most-derived slot and not to override it with a base + // class's slot. + var seen = new HashSet(); Type offsetType = typeof(TypeOffset); while (impl != null) @@ -473,7 +476,7 @@ internal static void InitializeSlots(IntPtr type, Type impl) continue; } - if (seen[name] != null) + if (seen.Contains(name)) { continue; } @@ -484,7 +487,7 @@ internal static void InitializeSlots(IntPtr type, Type impl) IntPtr slot = Interop.GetThunk(method); Marshal.WriteIntPtr(type, offset, slot); - seen[name] = 1; + seen.Add(name); } impl = impl.BaseType; From 9ae91baa163a1c8ce4f78d2fcb6b45bd969f2386 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Wed, 26 Sep 2018 19:23:05 -0400 Subject: [PATCH 04/10] UNI-63112: implement platform-aware native code for tp_traverse et al 1. Implement tp_traverse, tp_clear, and tp_is_gc in native code in memory that is *not* released upon domain reload. Effect: fixes the crash on domain reload. Side effect: leaks a page every domain reload. 2. Use python's platform package to determine what we're running on, so we can use the right mmap/mprotect or VirtualAlloc/VirtualProtect routines, the right flags for mmap, and (theoretically) the right native code. It should be easy to add another system, as long as it has the same kinds of functionality. 3. Enable the domain reload test. Added some unit tests for the new stuff. 4. Remove tp_traverse, tp_clear, and tp_is_gc where it was implemented. Note: I haven't actually tested on windows and linux yet, I'm checking in so I can do that easily. --- src/embed_tests/Python.EmbeddingTest.csproj | 1 + src/embed_tests/TestDomainReload.cs | 1 - src/embed_tests/TestRuntime.cs | 19 ++ src/embed_tests/TestTypeManager.cs | 45 ++++ src/runtime/classbase.cs | 18 -- src/runtime/extensiontype.cs | 21 -- src/runtime/runtime.cs | 103 ++++++++ src/runtime/typemanager.cs | 255 +++++++++++++++++++- 8 files changed, 417 insertions(+), 46 deletions(-) create mode 100644 src/embed_tests/TestTypeManager.cs diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index d16ca4ba4..e50053f07 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -104,6 +104,7 @@ + diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs index 3a91e84ae..7278ce884 100644 --- a/src/embed_tests/TestDomainReload.cs +++ b/src/embed_tests/TestDomainReload.cs @@ -40,7 +40,6 @@ class TestDomainReload /// a hot reload). /// [Test] - [Ignore("Test crashes")] public static void DomainReloadAndGC() { // We're set up to run in the directory that includes the bin directory. diff --git a/src/embed_tests/TestRuntime.cs b/src/embed_tests/TestRuntime.cs index 2e0598da7..c6446653c 100644 --- a/src/embed_tests/TestRuntime.cs +++ b/src/embed_tests/TestRuntime.cs @@ -6,6 +6,25 @@ namespace Python.EmbeddingTest { public class TestRuntime { + /// + /// Test the cache of the information from the platform module. + /// + /// Test fails on platforms we haven't implemented yet. + /// + [Test] + public static void PlatformCache() + { + Runtime.Runtime.Initialize(); + + Assert.That(Runtime.Runtime.Machine, Is.Not.EqualTo(Runtime.Runtime.MachineType.Other)); + Assert.That(!string.IsNullOrEmpty(Runtime.Runtime.MachineName)); + + Assert.That(Runtime.Runtime.OperatingSystem, Is.Not.EqualTo(Runtime.Runtime.OperatingSystemType.Other)); + Assert.That(!string.IsNullOrEmpty(Runtime.Runtime.OperatingSystemName)); + + Runtime.Runtime.Shutdown(); + } + [Test] public static void Py_IsInitializedValue() { diff --git a/src/embed_tests/TestTypeManager.cs b/src/embed_tests/TestTypeManager.cs new file mode 100644 index 000000000..d1f35bac1 --- /dev/null +++ b/src/embed_tests/TestTypeManager.cs @@ -0,0 +1,45 @@ +using NUnit.Framework; +using Python.Runtime; +using System.Runtime.InteropServices; + +namespace Python.EmbeddingTest +{ + class TestTypeManager + { + [Test] + public static void TestNativeCode() + { + Runtime.Runtime.Initialize(); + + Assert.That(() => { var _ = TypeManager.NativeCode.Active; }, Throws.Nothing); + Assert.That(TypeManager.NativeCode.Active.Code.Length, Is.GreaterThan(0)); + + Runtime.Runtime.Shutdown(); + } + + [Test] + public static void TestMemoryMapping() + { + Runtime.Runtime.Initialize(); + + Assert.That(() => { var _ = TypeManager.CreateMemoryMapper(); }, Throws.Nothing); + var mapper = TypeManager.CreateMemoryMapper(); + + // Allocate a read-write page. + int len = 12; + var page = mapper.MapWriteable(len); + Assert.That(() => { Marshal.WriteInt64(page, 17); }, Throws.Nothing); + Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); + + // Mark it read-execute, now we can't write anymore (I'm not testing we can execute). + // We should be getting AccessViolationException, but Mono translates + // SIGSEGV to NullReferenceException instead, so just check for some exception. + mapper.SetReadExec(page, len); + Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); + Assert.That(() => { Marshal.WriteInt64(page, 18); }, Throws.Exception); + Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); + + Runtime.Runtime.Shutdown(); + } + } +} diff --git a/src/runtime/classbase.cs b/src/runtime/classbase.cs index 4dd3b5364..5846fa82a 100644 --- a/src/runtime/classbase.cs +++ b/src/runtime/classbase.cs @@ -247,24 +247,6 @@ public static IntPtr tp_str(IntPtr ob) } - /// - /// Default implementations for required Python GC support. - /// - public static int tp_traverse(IntPtr ob, IntPtr func, IntPtr args) - { - return 0; - } - - public static int tp_clear(IntPtr ob) - { - return 0; - } - - public static int tp_is_gc(IntPtr type) - { - return 1; - } - /// /// Standard dealloc implementation for instances of reflected types. /// diff --git a/src/runtime/extensiontype.cs b/src/runtime/extensiontype.cs index 9569b0485..693a46f42 100644 --- a/src/runtime/extensiontype.cs +++ b/src/runtime/extensiontype.cs @@ -81,27 +81,6 @@ public static int tp_descr_set(IntPtr ds, IntPtr ob, IntPtr val) } - /// - /// Required Python GC support. - /// - public static int tp_traverse(IntPtr ob, IntPtr func, IntPtr args) - { - return 0; - } - - - public static int tp_clear(IntPtr ob) - { - return 0; - } - - - public static int tp_is_gc(IntPtr type) - { - return 1; - } - - /// /// Default dealloc implementation. /// diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index b4ddc7f7e..0f136e7e7 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -2,6 +2,7 @@ using System.Runtime.InteropServices; using System.Security; using System.Text; +using System.Collections.Generic; namespace Python.Runtime { @@ -195,6 +196,57 @@ public class Runtime // .NET core: System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows) internal static bool IsWindows = Environment.OSVersion.Platform == PlatformID.Win32NT; + /// + /// Operating system type as reported by Python. + /// + public enum OperatingSystemType + { + Windows, + Darwin, + Linux, + Other + } + + static readonly Dictionary OperatingSystemTypeMapping = new Dictionary() + { + { "Windows", OperatingSystemType.Windows }, + { "Darwin", OperatingSystemType.Darwin }, + { "Linux", OperatingSystemType.Linux }, + }; + + /// + /// Gets the operating system as reported by python's platform.system(). + /// + public static OperatingSystemType OperatingSystem { get; private set; } + + /// + /// Gets the operating system as reported by python's platform.system(). + /// + public static string OperatingSystemName { get; private set; } + + public enum MachineType + { + i386, + x86_64, + Other + }; + + static readonly Dictionary MachineTypeMapping = new Dictionary() + { + { "i386", MachineType.i386 }, + { "x86_64", MachineType.x86_64 }, + }; + + /// + /// Gets the machine architecture as reported by python's platform.machine(). + /// + public static MachineType Machine { get; private set; }/* set in Initialize using python's platform.machine */ + + /// + /// Gets the machine architecture as reported by python's platform.machine(). + /// + public static string MachineName { get; private set; } + internal static bool IsPython2 = pyversionnumber < 30; internal static bool IsPython3 = pyversionnumber >= 30; @@ -331,6 +383,10 @@ internal static void Initialize() NativeMethods.FreeLibrary(dllLocal); } #endif + // Initialize data about the platform we're running on. We need + // this for the type manager and potentially other details. Must + // happen after caching the python types, above. + InitializePlatformData(); // Initialize modules that depend on the runtime class. AssemblyManager.Initialize(); @@ -348,6 +404,53 @@ internal static void Initialize() AssemblyManager.UpdatePath(); } + /// + /// Initializes the data about platforms. + /// + /// This must be the last step when initializing the runtime: + /// GetManagedString needs to have the cached values for types. + /// But it must run before initializing anything outside the runtime + /// because those rely on the platform data. + /// + private static void InitializePlatformData() + { + IntPtr op; + IntPtr fn; + IntPtr platformModule = PyImport_ImportModule("platform"); + IntPtr emptyTuple = PyTuple_New(0); + + fn = PyObject_GetAttrString(platformModule, "system"); + op = PyObject_Call(fn, emptyTuple, IntPtr.Zero); + OperatingSystemName = GetManagedString(op); + XDecref(op); + XDecref(fn); + + fn = PyObject_GetAttrString(platformModule, "machine"); + op = PyObject_Call(fn, emptyTuple, IntPtr.Zero); + MachineName = GetManagedString(op); + XDecref(op); + XDecref(fn); + + XDecref(emptyTuple); + XDecref(platformModule); + + // Now convert the strings into enum values so we can do switch + // statements rather than constant parsing. + OperatingSystemType OSType; + if (!OperatingSystemTypeMapping.TryGetValue(OperatingSystemName, out OSType)) + { + OSType = OperatingSystemType.Other; + } + OperatingSystem = OSType; + + MachineType MType; + if (!MachineTypeMapping.TryGetValue(MachineName, out MType)) + { + MType = MachineType.Other; + } + Machine = MType; + } + internal static void Shutdown() { AssemblyManager.Shutdown(); diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index db46c310f..55cf3bac7 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -447,6 +447,222 @@ internal static IntPtr AllocateTypeObject(string name) } + #region Native Page + /// + /// Initialized by InitializeNativeCodePage. + /// + /// This points to a page of memory allocated using mmap or VirtualAlloc + /// (depending on the system), and marked read and execute (not write). + /// Very much on purpose, the page is *not* released on a shutdown and + /// is instead leaked. See the TestDomainReload test case. + /// + /// The contents of the page are two native functions: one that returns 0, + /// one that returns 1. + /// + /// If python didn't keep its gc list through a Py_Finalize we could remove + /// this entire section. + /// + internal static IntPtr NativeCodePage = IntPtr.Zero; + + /// + /// Structure to describe native code. + /// + /// Use NativeCode.Active to get the native code for the current platform. + /// + /// Generate the code by creating the following C code: + /// + /// int Return0() { return 0; } + /// int Return1() { return 1; } + /// + /// Then compiling on the target platform, e.g. with gcc or clang: + /// cc -c -fomit-frame-pointer -O2 foo.c + /// And then analyzing the resulting functions with a hex editor, e.g.: + /// objdump -disassemble foo.o + /// + internal class NativeCode + { + /// + /// The code, as a string of bytes. + /// + public byte[] Code { get; private set; } + + /// + /// The offset for the "return 0" function. Usually zero. + /// + public int Return0 { get; private set; } + + /// + /// The offset for the "return 1" function. Usually one. + /// + public int Return1 { get; private set; } + + public static NativeCode Active + { + get + { + switch(Runtime.Machine) + { + case Runtime.MachineType.i386: + return I386; + case Runtime.MachineType.x86_64: + return X86_64; + default: + throw new NotImplementedException($"No support for ${Runtime.MachineName}"); + } + } + } + + /// + /// Code for x86_64. See the class comment for how it was generated. + /// + public static readonly NativeCode X86_64 = new NativeCode() + { + Return0 = 0x10, + Return1 = 0, + Code = new byte[] + { + // First Return1: + 0xb8, 0x01, 0x00, 0x00, 0x00, // movl $1, %eax + 0xc3, // ret + + // Now some padding so that Return0 can be 16-byte-aligned. + // I put Return1 first so there's not as much padding to type in. + 0x66, 0x2e, 0x0f, 0x1f, 0x84, 0x00, 0x00, 0x00, 0x00, 0x00, // nop + + // Now Return0. + 0x31, 0xc0, // xorl %eax, %eax + 0xc3, // ret + } + }; + + /// + /// Code for X86. + /// + /// It's bitwise identical to X86_64, so we just point to it. + /// + /// + public static readonly NativeCode I386 = X86_64; + } + + /// + /// Platform-dependent mmap and mprotect. + /// + internal interface IMemoryMapper + { + /// + /// Map at least numBytes of memory. Mark the page read-write (but not exec). + /// + IntPtr MapWriteable(int numBytes); + + /// + /// Sets the mapped memory to be read-exec (but not write). + /// + void SetReadExec(IntPtr mappedMemory, int numBytes); + } + + class WindowsMemoryMapper : IMemoryMapper + { + const UInt32 MEM_COMMIT = 0x1000; + const UInt32 MEM_RESERVE = 0x2000; + const UInt32 PAGE_READWRITE = 0x04; + const UInt32 PAGE_EXECUTE_READ = 0x20; + + [DllImport("kernel32.dll")] + static extern IntPtr VirtualAlloc(IntPtr lpAddress, IntPtr dwSize, UInt32 flAllocationType, UInt32 flProtect); + + [DllImport("kernel32.dll")] + static extern bool VirtualProtect(IntPtr lpAddress, IntPtr dwSize, UInt32 flNewProtect, out UInt32 lpflOldProtect); + + public IntPtr MapWriteable(int numBytes) + { + return VirtualAlloc(IntPtr.Zero, new IntPtr(numBytes), + MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); + } + + public void SetReadExec(IntPtr mappedMemory, int numBytes) + { + UInt32 _; + VirtualProtect(mappedMemory, new IntPtr(numBytes), PAGE_EXECUTE_READ, out _); + } + } + + class UnixMemoryMapper : IMemoryMapper + { + const int PROT_READ = 0x1; + const int PROT_WRITE = 0x2; + const int PROT_EXEC = 0x4; + + int MAP_ANONYMOUS + { + get + { + switch (Runtime.OperatingSystem) + { + case Runtime.OperatingSystemType.Darwin: + return 0x1000; + case Runtime.OperatingSystemType.Linux: + return 0x20; + default: + throw new NotImplementedException($"mmap is not supported on {Runtime.OperatingSystemName}"); + } + } + } + + [DllImport("__Internal")] + static extern IntPtr mmap(IntPtr addr, IntPtr len, int prot, int flags, int fd, IntPtr offset); + + [DllImport("__Internal")] + static extern int mprotect(IntPtr addr, IntPtr len, int prot); + + public IntPtr MapWriteable(int numBytes) + { + return mmap(IntPtr.Zero, new IntPtr(numBytes), PROT_READ | PROT_WRITE, MAP_ANONYMOUS, -1, IntPtr.Zero); + } + + public void SetReadExec(IntPtr mappedMemory, int numBytes) + { + mprotect(mappedMemory, new IntPtr(numBytes), PROT_READ | PROT_EXEC); + } + } + + internal static IMemoryMapper CreateMemoryMapper() + { + switch (Runtime.OperatingSystem) + { + case Runtime.OperatingSystemType.Darwin: + case Runtime.OperatingSystemType.Linux: + return new UnixMemoryMapper(); + case Runtime.OperatingSystemType.Windows: + return new WindowsMemoryMapper(); + default: + throw new NotImplementedException($"No support for {Runtime.OperatingSystemName}"); + } + } + + /// + /// Initializes the native code page. + /// + /// Safe to call if we already initialized (this function is idempotent). + /// + /// + internal static void InitializeNativeCodePage() + { + // Do nothing if we already initialized. + if (NativeCodePage != IntPtr.Zero) + { + return; + } + + // Allocate the page, write the native code into it, then set it + // to be executable. + IMemoryMapper mapper = CreateMemoryMapper(); + int codeLength = NativeCode.Active.Code.Length; + NativeCodePage = mapper.MapWriteable(codeLength); + Marshal.Copy(NativeCode.Active.Code, 0, NativeCodePage, codeLength); + mapper.SetReadExec(NativeCodePage, codeLength); + } +#endregion + /// /// Given a newly allocated Python type object and a managed Type that /// provides the implementation for the type, connect the type slots of @@ -458,7 +674,6 @@ internal static void InitializeSlots(IntPtr type, Type impl) // the most-derived slot and not to override it with a base // class's slot. var seen = new HashSet(); - Type offsetType = typeof(TypeOffset); while (impl != null) { @@ -481,19 +696,47 @@ internal static void InitializeSlots(IntPtr type, Type impl) continue; } - FieldInfo fi = offsetType.GetField(name); - var offset = (int)fi.GetValue(offsetType); - - IntPtr slot = Interop.GetThunk(method); - Marshal.WriteIntPtr(type, offset, slot); + InitializeSlot(type, Interop.GetThunk(method), name); seen.Add(name); } impl = impl.BaseType; } + + // See the TestDomainReload test: there was a crash related to + // the gc-related slots. They always return 0 or 1 because we don't + // really support gc: + // tp_traverse (returns 0) + // tp_clear (returns 0) + // tp_is_gc (returns 1) + // We can't do without: python really wants those slots to exist. + // We can't implement those in C# because the application domain + // can be shut down and the memory released. + InitializeNativeCodePage(); + InitializeSlot(type, NativeCodePage + NativeCode.Active.Return0, "tp_traverse"); + InitializeSlot(type, NativeCodePage + NativeCode.Active.Return0, "tp_clear"); + InitializeSlot(type, NativeCodePage + NativeCode.Active.Return1, "tp_is_gc"); } + /// + /// Helper for InitializeSlots. + /// + /// Initializes one slot to point to a function pointer. + /// The function pointer might be a thunk for C#, or it may be + /// an address in the NativeCodePage. + /// + /// Type being initialized. + /// Function pointer. + /// Name of the method. + static void InitializeSlot(IntPtr type, IntPtr slot, string name) + { + Type typeOffset = typeof(TypeOffset); + FieldInfo fi = typeOffset.GetField(name); + var offset = (int)fi.GetValue(typeOffset); + + Marshal.WriteIntPtr(type, offset, slot); + } /// /// Given a newly allocated Python type object and a managed Type that From bb76ff30fbbb96704d24bc7fe7611702efca72c6 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Thu, 27 Sep 2018 21:44:17 -0400 Subject: [PATCH 05/10] Domain reload work: port previous commit to Windows Nothing major: - python platform.machine() returns x86_64 on mac, and AMD64 on windows, for the same platform. Parse them properly. - Microsoft's runtime C# compiler wants language version 2.0, so no $"{foo}" syntax in our debug printing. - Microsoft has decided you can't catch SEGV even in a unit test, so disable that test on windows. Most of my time was spent trying to convince VS to see the unit tests. --- src/embed_tests/TestDomainReload.cs | 19 +++++++++++-------- src/embed_tests/TestTypeManager.cs | 16 +++++++++++----- src/runtime/runtime.cs | 9 ++++++++- src/runtime/typemanager.cs | 4 ++-- 4 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs index 7278ce884..79c4d55e6 100644 --- a/src/embed_tests/TestDomainReload.cs +++ b/src/embed_tests/TestDomainReload.cs @@ -65,6 +65,8 @@ public static void DomainReloadAndGC() // // What matters in the python code is gc.collect and clr.AddReference. // + // Note that the language version is 2.0, so no $"foo{bar}" syntax. + // const string TestCode = @" using Python.Runtime; using System; @@ -72,24 +74,25 @@ class PythonRunner { public static void RunPython() { AppDomain.CurrentDomain.DomainUnload += OnDomainUnload; string name = AppDomain.CurrentDomain.FriendlyName; - Console.WriteLine($""[{name} in .NET] In PythonRunner.RunPython""); + Console.WriteLine(string.Format(""[{0} in .NET] In PythonRunner.RunPython"", name)); using (Py.GIL()) { try { - var pyScript = ""import clr\n"" - + $""print('[{name} in python] imported clr')\n"" + var pyScript = string.Format(""import clr\n"" + + ""print('[{0} in python] imported clr')\n"" + ""clr.AddReference('System')\n"" - + $""print('[{name} in python] allocated a clr object')\n"" + + ""print('[{0} in python] allocated a clr object')\n"" + ""import gc\n"" + ""gc.collect()\n"" - + $""print('[{name} in python] collected garbage')\n""; + + ""print('[{0} in python] collected garbage')\n"", + name); PythonEngine.Exec(pyScript); } catch(Exception e) { - Console.WriteLine($""[{name} in .NET] Caught exception: {e}""); + Console.WriteLine(string.Format(""[{0} in .NET] Caught exception: {1}"", name, e)); } } } static void OnDomainUnload(object sender, EventArgs e) { - System.Console.WriteLine(string.Format($""[{AppDomain.CurrentDomain.FriendlyName} in .NET] unloading"")); + System.Console.WriteLine(string.Format(""[{0} in .NET] unloading"", AppDomain.CurrentDomain.FriendlyName)); } }"; @@ -139,7 +142,7 @@ class Proxy : MarshalByRefObject public void InitAssembly(string assemblyPath) { - theAssembly = Assembly.LoadFile(assemblyPath); + theAssembly = Assembly.LoadFile(System.IO.Path.GetFullPath(assemblyPath)); } public void RunPython() diff --git a/src/embed_tests/TestTypeManager.cs b/src/embed_tests/TestTypeManager.cs index d1f35bac1..467b8f3f2 100644 --- a/src/embed_tests/TestTypeManager.cs +++ b/src/embed_tests/TestTypeManager.cs @@ -31,13 +31,19 @@ public static void TestMemoryMapping() Assert.That(() => { Marshal.WriteInt64(page, 17); }, Throws.Nothing); Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); - // Mark it read-execute, now we can't write anymore (I'm not testing we can execute). - // We should be getting AccessViolationException, but Mono translates - // SIGSEGV to NullReferenceException instead, so just check for some exception. + // Mark it read-execute, now we can't write anymore. + // We can't actually test access protectoin under Windows, + // because AccessViolationException is assumed to mean we're in a + // corrupted state: + // https://stackoverflow.com/questions/3469368/how-to-handle-accessviolationexception mapper.SetReadExec(page, len); Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); - Assert.That(() => { Marshal.WriteInt64(page, 18); }, Throws.Exception); - Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); + if (Runtime.Runtime.OperatingSystem != Runtime.Runtime.OperatingSystemType.Windows) + { + // Mono throws NRE instead of AccessViolationException for some reason. + Assert.That(() => { Marshal.WriteInt64(page, 73); }, Throws.TypeOf()); + Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); + } Runtime.Runtime.Shutdown(); } diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index 0f136e7e7..095160cdd 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -231,10 +231,17 @@ public enum MachineType Other }; + /// + /// Map lower-case version of the python machine name to the processor + /// type. There are aliases, e.g. x86_64 and amd64 are two names for + /// the same thing. Make sure to lower-case the search string, because + /// capitalization can differ. + /// static readonly Dictionary MachineTypeMapping = new Dictionary() { { "i386", MachineType.i386 }, { "x86_64", MachineType.x86_64 }, + { "amd64", MachineType.x86_64 }, }; /// @@ -444,7 +451,7 @@ private static void InitializePlatformData() OperatingSystem = OSType; MachineType MType; - if (!MachineTypeMapping.TryGetValue(MachineName, out MType)) + if (!MachineTypeMapping.TryGetValue(MachineName.ToLower(), out MType)) { MType = MachineType.Other; } diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index 55cf3bac7..d0a709cce 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections; using System.Collections.Generic; using System.Reflection; @@ -507,7 +507,7 @@ public static NativeCode Active case Runtime.MachineType.x86_64: return X86_64; default: - throw new NotImplementedException($"No support for ${Runtime.MachineName}"); + throw new NotImplementedException($"No support for {Runtime.MachineName}"); } } } From 0c5b78fe9351145534bbf20232821256b572840d Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Thu, 27 Sep 2018 21:55:22 -0400 Subject: [PATCH 06/10] Fixed a bogus comment. --- src/runtime/typemanager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index d0a709cce..f0b7a5dc7 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -487,12 +487,12 @@ internal class NativeCode public byte[] Code { get; private set; } /// - /// The offset for the "return 0" function. Usually zero. + /// Where does the "return 0" function start? /// public int Return0 { get; private set; } /// - /// The offset for the "return 1" function. Usually one. + /// Where does the "return 1" function start? /// public int Return1 { get; private set; } From 9ffb705debc0472927110b2b9029b58b2435f5b8 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Thu, 27 Sep 2018 21:56:54 -0400 Subject: [PATCH 07/10] Make prettier --- src/runtime/typemanager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index f0b7a5dc7..5912a0d80 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -447,7 +447,7 @@ internal static IntPtr AllocateTypeObject(string name) } - #region Native Page + #region Native Code Page /// /// Initialized by InitializeNativeCodePage. /// From 2e03eb816da83bd7cedad90dab57305cdf80ffc0 Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Fri, 28 Sep 2018 16:56:58 -0400 Subject: [PATCH 08/10] Added author and changelog information. --- AUTHORS.md | 3 +++ CHANGELOG.md | 2 ++ 2 files changed, 5 insertions(+) diff --git a/AUTHORS.md b/AUTHORS.md index 3c39794e4..023b4d185 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -14,6 +14,7 @@ - Alexandre Catarino([@AlexCatarino](https://github.com/AlexCatarino)) - Arvid JB ([@ArvidJB](https://github.com/ArvidJB)) +- Benoît Hudson ([@benoithudson](https://github.com/benoithudson)) - Bradley Friedman ([@leith-bartrich](https://github.com/leith-bartrich)) - Callum Noble ([@callumnoble](https://github.com/callumnoble)) - Christian Heimes ([@tiran](https://github.com/tiran)) @@ -22,6 +23,7 @@ - Daniel Fernandez ([@fdanny](https://github.com/fdanny)) - Daniel Santana ([@dgsantana](https://github.com/dgsantana)) - Dave Hirschfeld ([@dhirschfeld](https://github.com/dhirschfeld)) +- David Lassonde ([@lassond](https://github.com/lassond)) - David Lechner ([@dlech](https://github.com/dlech)) - Dmitriy Se ([@dmitriyse](https://github.com/dmitriyse)) - He-chien Tsai ([@t3476](https://github.com/t3476)) @@ -39,6 +41,7 @@ - Sam Winstanley ([@swinstanley](https://github.com/swinstanley)) - Sean Freitag ([@cowboygneox](https://github.com/cowboygneox)) - Serge Weinstock ([@sweinst](https://github.com/sweinst)) +- Viktoria Kovescses ([@vkovec](https://github.com/vkovec)) - Ville M. Vainio ([@vivainio](https://github.com/vivainio)) - Virgil Dupras ([@hsoft](https://github.com/hsoft)) - Wenguang Yang ([@yagweb](https://github.com/yagweb)) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3dc668b0c..c165a6c4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ### Fixed - Fixed Visual Studio 2017 compat ([#434][i434]) for setup.py +- Fixed crashes when integrating pythonnet in Unity3d ([#714][i714]), + related to unloading the Application Domain - Fixed crash on exit of the Python interpreter if a python class derived from a .NET class has a `__namespace__` or `__assembly__` attribute ([#481][i481]) From 156f55454d13cac1ae3a8fceedcdf4955b09c82f Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Tue, 2 Oct 2018 16:34:37 -0400 Subject: [PATCH 09/10] Fix for linux mmap requiring MAP_PRIVATE mac mmap allows it, so just set it on both platforms. --- src/runtime/typemanager.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/runtime/typemanager.cs b/src/runtime/typemanager.cs index 5912a0d80..3a26dd5ad 100644 --- a/src/runtime/typemanager.cs +++ b/src/runtime/typemanager.cs @@ -592,6 +592,7 @@ class UnixMemoryMapper : IMemoryMapper const int PROT_WRITE = 0x2; const int PROT_EXEC = 0x4; + const int MAP_PRIVATE = 0x2; int MAP_ANONYMOUS { get @@ -616,7 +617,9 @@ int MAP_ANONYMOUS public IntPtr MapWriteable(int numBytes) { - return mmap(IntPtr.Zero, new IntPtr(numBytes), PROT_READ | PROT_WRITE, MAP_ANONYMOUS, -1, IntPtr.Zero); + // MAP_PRIVATE must be set on linux, even though MAP_ANON implies it. + // It doesn't hurt on darwin, so just do it. + return mmap(IntPtr.Zero, new IntPtr(numBytes), PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, IntPtr.Zero); } public void SetReadExec(IntPtr mappedMemory, int numBytes) From 84f508770bb834331f346438db4372ae45bfd76a Mon Sep 17 00:00:00 2001 From: Benoit Hudson Date: Tue, 2 Oct 2018 16:41:33 -0400 Subject: [PATCH 10/10] Doc and typo fixes. --- src/embed_tests/TestDomainReload.cs | 2 +- src/embed_tests/TestTypeManager.cs | 7 ++++--- src/runtime/runtime.cs | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs index 79c4d55e6..dd51bf531 100644 --- a/src/embed_tests/TestDomainReload.cs +++ b/src/embed_tests/TestDomainReload.cs @@ -14,7 +14,7 @@ class TestDomainReload /// At the time this test was written, there was a very annoying /// seemingly random crash bug when integrating pythonnet into Unity. /// - /// The repro steps we that David Lassonde, Viktoria Kovecses and + /// The repro steps that David Lassonde, Viktoria Kovecses and /// Benoit Hudson eventually worked out: /// 1. Write a HelloWorld.cs script that uses Python.Runtime to access /// some C# data from python: C# calls python, which calls C#. diff --git a/src/embed_tests/TestTypeManager.cs b/src/embed_tests/TestTypeManager.cs index 467b8f3f2..a7c74cfe3 100644 --- a/src/embed_tests/TestTypeManager.cs +++ b/src/embed_tests/TestTypeManager.cs @@ -32,9 +32,10 @@ public static void TestMemoryMapping() Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); // Mark it read-execute, now we can't write anymore. - // We can't actually test access protectoin under Windows, - // because AccessViolationException is assumed to mean we're in a - // corrupted state: + // + // We can't actually test access protection under Windows, because + // AccessViolationException is assumed to mean we're in a corrupted + // state: // https://stackoverflow.com/questions/3469368/how-to-handle-accessviolationexception mapper.SetReadExec(page, len); Assert.That(Marshal.ReadInt64(page), Is.EqualTo(17)); diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index 095160cdd..c5c36dec1 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -232,10 +232,10 @@ public enum MachineType }; /// - /// Map lower-case version of the python machine name to the processor - /// type. There are aliases, e.g. x86_64 and amd64 are two names for - /// the same thing. Make sure to lower-case the search string, because - /// capitalization can differ. + /// Map lower-case version of the python machine name to the processor + /// type. There are aliases, e.g. x86_64 and amd64 are two names for + /// the same thing. Make sure to lower-case the search string, because + /// capitalization can differ. /// static readonly Dictionary MachineTypeMapping = new Dictionary() {