From 6eb9258da0a35934cb8da9f4b1d8e63a61dc2387 Mon Sep 17 00:00:00 2001 From: Victor Milovanov Date: Tue, 3 Mar 2020 17:08:42 -0800 Subject: [PATCH] do not eat all exceptions when trying to add CLR reference, provide more info when CLR assemblies are failed to be loaded 1. When trying to implicitly load assemblies, and that fails NOT because an assembly is missing, but because loading failed for some reason, emit Python warning. 2. When trying to import a module in our import hook, if the module name is an assembly name, and we fail to load it, and Python also fails to find a module with the same name, add the exceptions we got during the attempt to load it into __cause__ of the final ImportError BREAKING: clr.AddReference will now throw exceptions besides FileNotFoundException. Additional: a few uses of BorrowedReference This addresses https://github.com/pythonnet/pythonnet/issues/261 It is an alternative to https://github.com/pythonnet/pythonnet/pull/298 --- CHANGELOG.md | 3 ++ src/embed_tests/pyimport.cs | 27 ++++++++++++-- src/runtime/assemblymanager.cs | 65 +++++++++++++--------------------- src/runtime/exceptions.cs | 6 ++-- src/runtime/importhook.cs | 34 ++++++++++-------- src/runtime/methodbinder.cs | 4 +-- src/runtime/moduleobject.cs | 9 +++-- src/runtime/pylist.cs | 5 +++ src/runtime/pysequence.cs | 2 ++ src/runtime/pytuple.cs | 9 +++++ src/runtime/runtime.cs | 19 +++++----- 11 files changed, 110 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9266b487..b62b291fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. ### Changed - Drop support for Python 2 +- `clr.AddReference` may now throw errors besides `FileNotFoundException`, that provide more +details about the cause of the failure +- `clr.AddReference` no longer adds ".dll" implicitly ### Fixed diff --git a/src/embed_tests/pyimport.cs b/src/embed_tests/pyimport.cs index 6b2408745..d78b030ea 100644 --- a/src/embed_tests/pyimport.cs +++ b/src/embed_tests/pyimport.cs @@ -38,8 +38,8 @@ public void SetUp() string testPath = Path.Combine(TestContext.CurrentContext.TestDirectory, s); IntPtr str = Runtime.Runtime.PyString_FromString(testPath); - IntPtr path = Runtime.Runtime.PySys_GetObject("path"); - Runtime.Runtime.PyList_Append(new BorrowedReference(path), str); + BorrowedReference path = Runtime.Runtime.PySys_GetObject("path"); + Runtime.Runtime.PyList_Append(path, str); } [TearDown] @@ -83,5 +83,28 @@ public void TestCastGlobalVar() Assert.AreEqual("2", foo.FOO.ToString()); Assert.AreEqual("2", foo.test_foo().ToString()); } + + [Test] + public void BadAssembly() + { + string path; + if (Python.Runtime.Runtime.IsWindows) + { + path = @"C:\Windows\System32\kernel32.dll"; + } + else + { + Assert.Pass("TODO: add bad assembly location for other platforms"); + return; + } + + string code = $@" +import clr +clr.AddReference('{path}') +"; + + var error = Assert.Throws(() => PythonEngine.Exec(code)); + Assert.AreEqual(nameof(FileLoadException), error.PythonTypeName); + } } } diff --git a/src/runtime/assemblymanager.cs b/src/runtime/assemblymanager.cs index 46909a370..ba6faa076 100644 --- a/src/runtime/assemblymanager.cs +++ b/src/runtime/assemblymanager.cs @@ -137,7 +137,7 @@ private static Assembly ResolveHandler(object ob, ResolveEventArgs args) /// internal static void UpdatePath() { - IntPtr list = Runtime.PySys_GetObject("path"); + BorrowedReference list = Runtime.PySys_GetObject("path"); var count = Runtime.PyList_Size(list); if (count != pypath.Count) { @@ -199,19 +199,14 @@ public static string FindAssembly(string name) /// public static Assembly LoadAssembly(string name) { - Assembly assembly = null; try { - assembly = Assembly.Load(name); + return Assembly.Load(name); } - catch (Exception) + catch (FileNotFoundException) { - //if (!(e is System.IO.FileNotFoundException)) - //{ - // throw; - //} + return null; } - return assembly; } @@ -221,18 +216,8 @@ public static Assembly LoadAssembly(string name) public static Assembly LoadAssemblyPath(string name) { string path = FindAssembly(name); - Assembly assembly = null; - if (path != null) - { - try - { - assembly = Assembly.LoadFrom(path); - } - catch (Exception) - { - } - } - return assembly; + if (path == null) return null; + return Assembly.LoadFrom(path); } /// @@ -242,25 +227,14 @@ public static Assembly LoadAssemblyPath(string name) /// public static Assembly LoadAssemblyFullPath(string name) { - Assembly assembly = null; if (Path.IsPathRooted(name)) { - if (!Path.HasExtension(name)) - { - name = name + ".dll"; - } if (File.Exists(name)) { - try - { - assembly = Assembly.LoadFrom(name); - } - catch (Exception) - { - } + return Assembly.LoadFrom(name); } } - return assembly; + return null; } /// @@ -291,7 +265,7 @@ public static Assembly FindLoadedAssembly(string name) /// actually loads an assembly. /// Call ONLY for namespaces that HAVE NOT been cached yet. /// - public static bool LoadImplicit(string name, bool warn = true) + public static bool LoadImplicit(string name, Action assemblyLoadErrorHandler, bool warn = true) { string[] names = name.Split('.'); var loaded = false; @@ -308,14 +282,23 @@ public static bool LoadImplicit(string name, bool warn = true) assembliesSet = new HashSet(AppDomain.CurrentDomain.GetAssemblies()); } Assembly a = FindLoadedAssembly(s); - if (a == null) - { - a = LoadAssemblyPath(s); - } - if (a == null) + try { - a = LoadAssembly(s); + if (a == null) + { + a = LoadAssemblyPath(s); + } + + if (a == null) + { + a = LoadAssembly(s); + } } + catch (FileLoadException e) { assemblyLoadErrorHandler(e); } + catch (BadImageFormatException e) { assemblyLoadErrorHandler(e); } + catch (System.Security.SecurityException e) { assemblyLoadErrorHandler(e); } + catch (PathTooLongException e) { assemblyLoadErrorHandler(e); } + if (a != null && !assembliesSet.Contains(a)) { loaded = true; diff --git a/src/runtime/exceptions.cs b/src/runtime/exceptions.cs index e5efecbcf..f4cb519a6 100644 --- a/src/runtime/exceptions.cs +++ b/src/runtime/exceptions.cs @@ -254,9 +254,9 @@ public static void SetError(IntPtr ob, string value) /// Sets the current Python exception given a Python object. /// This is a wrapper for the Python PyErr_SetObject call. /// - public static void SetError(IntPtr ob, IntPtr value) + public static void SetError(IntPtr type, IntPtr exceptionObject) { - Runtime.PyErr_SetObject(ob, value); + Runtime.PyErr_SetObject(new BorrowedReference(type), new BorrowedReference(exceptionObject)); } /// @@ -286,7 +286,7 @@ public static void SetError(Exception e) IntPtr op = CLRObject.GetInstHandle(e); IntPtr etype = Runtime.PyObject_GetAttrString(op, "__class__"); - Runtime.PyErr_SetObject(etype, op); + Runtime.PyErr_SetObject(new BorrowedReference(etype), new BorrowedReference(op)); Runtime.XDecref(etype); Runtime.XDecref(op); } diff --git a/src/runtime/importhook.cs b/src/runtime/importhook.cs index 4c797ff8d..96a8b7ebe 100644 --- a/src/runtime/importhook.cs +++ b/src/runtime/importhook.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Runtime.InteropServices; namespace Python.Runtime @@ -222,22 +223,12 @@ public static IntPtr __import__(IntPtr self, IntPtr args, IntPtr kw) // Check these BEFORE the built-in import runs; may as well // do the Incref()ed return here, since we've already found // the module. - if (mod_name == "clr") + if (mod_name == "clr" || mod_name == "CLR") { - IntPtr clr_module = GetCLRModule(fromList); - if (clr_module != IntPtr.Zero) + if (mod_name == "CLR") { - IntPtr sys_modules = Runtime.PyImport_GetModuleDict(); - if (sys_modules != IntPtr.Zero) - { - Runtime.PyDict_SetItemString(sys_modules, "clr", clr_module); - } + Exceptions.deprecation("The CLR module is deprecated. Please use 'clr'."); } - return clr_module; - } - if (mod_name == "CLR") - { - Exceptions.deprecation("The CLR module is deprecated. Please use 'clr'."); IntPtr clr_module = GetCLRModule(fromList); if (clr_module != IntPtr.Zero) { @@ -249,8 +240,10 @@ public static IntPtr __import__(IntPtr self, IntPtr args, IntPtr kw) } return clr_module; } + string realname = mod_name; string clr_prefix = null; + if (mod_name.StartsWith("CLR.")) { clr_prefix = "CLR."; // prepend when adding the module to sys.modules @@ -312,11 +305,22 @@ public static IntPtr __import__(IntPtr self, IntPtr args, IntPtr kw) AssemblyManager.UpdatePath(); if (!AssemblyManager.IsValidNamespace(realname)) { - if (!AssemblyManager.LoadImplicit(realname)) + var loadExceptions = new List(); + if (!AssemblyManager.LoadImplicit(realname, assemblyLoadErrorHandler: loadExceptions.Add)) { // May be called when a module being imported imports a module. // In particular, I've seen decimal import copy import org.python.core - return Runtime.PyObject_Call(py_import, args, kw); + IntPtr importResult = Runtime.PyObject_Call(py_import, args, kw); + // TODO: use ModuleNotFoundError in Python 3.6+ + if (importResult == IntPtr.Zero && loadExceptions.Count > 0 + && Exceptions.ExceptionMatches(Exceptions.ImportError)) + { + loadExceptions.Add(new PythonException()); + var importError = new PyObject(new BorrowedReference(Exceptions.ImportError)); + importError.SetAttr("__cause__", new AggregateException(loadExceptions).ToPython()); + Runtime.PyErr_SetObject(new BorrowedReference(Exceptions.ImportError), importError.Reference); + } + return importResult; } } diff --git a/src/runtime/methodbinder.cs b/src/runtime/methodbinder.cs index b3186a3f2..a3197093a 100644 --- a/src/runtime/methodbinder.cs +++ b/src/runtime/methodbinder.cs @@ -291,8 +291,8 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth IntPtr valueList = Runtime.PyDict_Values(kw); for (int i = 0; i < pynkwargs; ++i) { - var keyStr = Runtime.GetManagedString(Runtime.PyList_GetItem(keylist, i)); - kwargDict[keyStr] = Runtime.PyList_GetItem(valueList, i).DangerousGetAddress(); + var keyStr = Runtime.GetManagedString(Runtime.PyList_GetItem(new BorrowedReference(keylist), i)); + kwargDict[keyStr] = Runtime.PyList_GetItem(new BorrowedReference(valueList), i).DangerousGetAddress(); } Runtime.XDecref(keylist); Runtime.XDecref(valueList); diff --git a/src/runtime/moduleobject.cs b/src/runtime/moduleobject.cs index 15e4feee8..ea5bbbfa0 100644 --- a/src/runtime/moduleobject.cs +++ b/src/runtime/moduleobject.cs @@ -119,7 +119,7 @@ public ManagedType GetAttribute(string name, bool guess) // cost. Ask the AssemblyManager to do implicit loading for each // of the steps in the qualified name, then try it again. bool ignore = name.StartsWith("__"); - if (AssemblyManager.LoadImplicit(qname, !ignore)) + if (AssemblyManager.LoadImplicit(qname, assemblyLoadErrorHandler: ImportWarning, !ignore)) { if (AssemblyManager.IsValidNamespace(qname)) { @@ -161,6 +161,11 @@ public ManagedType GetAttribute(string name, bool guess) return null; } + static void ImportWarning(Exception exception) + { + Exceptions.warn(exception.ToString(), Exceptions.ImportWarning); + } + /// /// Stores an attribute in the instance dict for future lookups. @@ -365,7 +370,7 @@ internal void InitializePreload() if (interactive_preload) { interactive_preload = false; - if (Runtime.PySys_GetObject("ps1") != IntPtr.Zero) + if (!Runtime.PySys_GetObject("ps1").IsNull) { preload = true; } diff --git a/src/runtime/pylist.cs b/src/runtime/pylist.cs index 347cc3000..80267d81a 100644 --- a/src/runtime/pylist.cs +++ b/src/runtime/pylist.cs @@ -22,6 +22,11 @@ public PyList(IntPtr ptr) : base(ptr) { } + /// + /// Creates new pointing to the same object, as the given reference. + /// + internal PyList(BorrowedReference reference) : base(reference) { } + /// /// PyList Constructor diff --git a/src/runtime/pysequence.cs b/src/runtime/pysequence.cs index bfaee79a6..8df13f737 100644 --- a/src/runtime/pysequence.cs +++ b/src/runtime/pysequence.cs @@ -16,6 +16,8 @@ protected PySequence(IntPtr ptr) : base(ptr) { } + internal PySequence(BorrowedReference reference) : base(reference) { } + protected PySequence() { } diff --git a/src/runtime/pytuple.cs b/src/runtime/pytuple.cs index e534ff5d5..259d7ae0f 100644 --- a/src/runtime/pytuple.cs +++ b/src/runtime/pytuple.cs @@ -22,6 +22,15 @@ public PyTuple(IntPtr ptr) : base(ptr) { } + /// + /// PyTuple Constructor + /// + /// + /// Creates a new PyTuple from an existing object reference. + /// The object reference is not checked for type-correctness. + /// + internal PyTuple(BorrowedReference reference) : base(reference) { } + /// /// PyTuple Constructor diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index 9e0d91b70..55c827b67 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -95,6 +95,9 @@ public class Runtime // .NET core: System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows) internal static bool IsWindows = Environment.OSVersion.Platform == PlatformID.Win32NT; + internal static Version InteropVersion { get; } + = System.Reflection.Assembly.GetExecutingAssembly().GetName().Version; + static readonly Dictionary OperatingSystemTypeMapping = new Dictionary() { { "Windows", OperatingSystemType.Windows }, @@ -306,9 +309,9 @@ internal static void Initialize(bool initSigs = false) // Need to add the runtime directory to sys.path so that we // can find built-in assemblies like System.Data, et. al. string rtdir = RuntimeEnvironment.GetRuntimeDirectory(); - IntPtr path = PySys_GetObject("path"); + BorrowedReference path = PySys_GetObject("path"); IntPtr item = PyString_FromString(rtdir); - PyList_Append(new BorrowedReference(path), item); + PyList_Append(path, item); XDecref(item); AssemblyManager.UpdatePath(); } @@ -1565,13 +1568,13 @@ internal static IntPtr PyList_New(long size) [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] internal static extern IntPtr PyList_AsTuple(IntPtr pointer); - internal static BorrowedReference PyList_GetItem(IntPtr pointer, long index) + internal static BorrowedReference PyList_GetItem(BorrowedReference pointer, long index) { return PyList_GetItem(pointer, new IntPtr(index)); } [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] - private static extern BorrowedReference PyList_GetItem(IntPtr pointer, IntPtr index); + private static extern BorrowedReference PyList_GetItem(BorrowedReference pointer, IntPtr index); internal static int PyList_SetItem(IntPtr pointer, long index, IntPtr value) { @@ -1614,13 +1617,13 @@ internal static int PyList_SetSlice(IntPtr pointer, long start, long end, IntPtr [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] private static extern int PyList_SetSlice(IntPtr pointer, IntPtr start, IntPtr end, IntPtr value); - internal static long PyList_Size(IntPtr pointer) + internal static long PyList_Size(BorrowedReference pointer) { return (long)_PyList_Size(pointer); } [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl, EntryPoint = "PyList_Size")] - private static extern IntPtr _PyList_Size(IntPtr pointer); + private static extern IntPtr _PyList_Size(BorrowedReference pointer); //==================================================================== // Python tuple API @@ -1732,7 +1735,7 @@ int updatepath ); [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] - internal static extern IntPtr PySys_GetObject(string name); + internal static extern BorrowedReference PySys_GetObject(string name); [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] internal static extern int PySys_SetObject(string name, IntPtr ob); @@ -1835,7 +1838,7 @@ internal static IntPtr PyMem_Realloc(IntPtr ptr, long size) internal static extern void PyErr_SetString(IntPtr ob, string message); [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] - internal static extern void PyErr_SetObject(IntPtr ob, IntPtr message); + internal static extern void PyErr_SetObject(BorrowedReference type, BorrowedReference exceptionObject); [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] internal static extern IntPtr PyErr_SetFromErrno(IntPtr ob);