Repository navigation
Using String.Format crashes inside pythonnet #1642
Description
Environment
- Pythonnet version: 3.x
- Python version: 3.8.10
- Operating System: Windows 11 64bit
- .NET Runtime: net6.0
Details
How to reproduce(with project Console.vsproj): @filmor
PythonEngine.Initialize();
PythonEngine.Exec("from System import String\nString.Format('{0},{1}', 1, 2)"); // crashes here
int i = Runtime.Py_Main(cmd.Length, cmd);
PythonEngine.Shutdown();
Activity
filmor commentedon Dec 22, 2021
I can't reproduce this in the current alpha. Please provide the exact commit of pythonnet that you are running against and the exact project that reproduces the error. Also, please do not open another issue (this is the same as #1641 already). Edit this one, upload things here and reopen the issue afterwards.
0xBEEBAD commentedon Dec 22, 2021
im using the master branch
here is the project @filmor
pythonnet-3.0.0-a2 (2).zip
PS: another branch https://github.com/QuantConnect/pythonnet works fine with String.Format
0xBEEBAD commentedon Dec 23, 2021
System.AccessViolationException: 'Attempted to read or write protected memory. This is often an indication that other memory is corrupt.'
[External Code]
lostmsu commentedon Dec 23, 2021
I can confirm this reproduces. I believe the reason is that we no longer automatically convert
PyInttoSystem.Int32, because it is an unsafe conversion due to potential overflows.Instead, you should do
String.Format('{0},{1}', str(1), str(2))orIn general though this was always a problem with any Python objects passed to methods like this.
String.Formatdoes not acquire GIL when callingToStringon the arguments. E.g. in 2.x or QuantConnect fork this would probably crash with a similar error:String.Format('{0},{1}', str(1), { 'a': 'b' }).In my TensorFlow binding for that reason I wrap all
PyObjectinstances into a nearly identical type, that delegates all its members toPyObject, but acquires GIL for every method call.However, I do not think it is a good approach for the core library. If somebody wants to autoacquire GIL on all Python object accesses, we can provide a guidance on how to set it up.
@filmor thoughts?
P.S. keeping open until we make it less surprising, or at least document it.
filmor commentedon Dec 23, 2021
Hmm, this is not good. It should not be possible to crash the interpreter with a simple thing like this. We should enforce safe operation of all .NET methods that
PyObjectimplements (i.e. either work properly or throw), so all methods ofobjectandDynamicObject, and maybeIDisposableandISerializable.lostmsu commentedon Dec 23, 2021
@filmor anything will crash the interpreter without acquiring GIL.
filmor commentedon Dec 23, 2021
I am well aware of that :). I just think that it shouldn't be possible to crash the interpreter (easily) from within Python.
lostmsu commentedon Dec 23, 2021
Well, checking if we have GIL is pretty expensive (would slow down all members in
PyObject), and not releasing it upon calling .NET methods will prevent multithreaded code from progressing Python threads while .NET is running.Maybe we could make calling .NET methods safe from Python by default, and have
clr.no_gilwrapper for unsafe calls. E.g.But frankly I think we should just keep the current behavior, or add
clr.with_gilso thatclr.with_gil(String.Format)('{0},{1}', 1, 2)would not crash.0xBEEBAD commentedon Dec 24, 2021
Any conclusions with this issue? Are we going to leave it as it is or alter the usage of such case?
filmor commentedon Dec 24, 2021
I definitely want to have this fixed before we consider releasing an rc, but I won't think about this over the holidays :)
lostmsu commentedon Dec 24, 2021
@sekkit regardless of what we decide, you should be using one of the samples I provided above depending on who's formatting abilities you need, .NET (use
Int32(val), then the arguments are .NET values and the entire call is .NET business) or Python (usestr(val), then the arguments are converted to strings by Python before being passed toString.Format), they are guaranteed to work in any case.lostmsu commentedon Jan 6, 2022
One option is to have a debug build that would fail an assertion or raise an exception upon any attempt to call PyObject methods without GIL. In NuGet it could be sufficient to suffix version with -debug I think. Not sure about PyPi.
filmor commentedon Jan 7, 2022
I don't think that would be a good idea. There is no need to do this in a separate build, we can pass the respective information dynamically. Checking a
readonly boolshould be something that the JIT compiler should be able to optimise. I'll create a draft PR where we can discuss the exact behaviour further. My goal would be that (by default, when embedded in Python), all overrides of "native" .NET classes inPyObjectcheck for the GIL and either acquire it or throw.filmor commentedon Mar 3, 2022
We decided that we are going to GIL-protect all overridden
objectandDynamicObjectmethods.lostmsu commentedon Mar 3, 2022
@sekkit @filmor FYI, the workaround we are planning is going to be somewhat misleading. In your example if you added integer format specifiers, and still used number literals in Python, you would get your format specifiers ignored.
UPD. NVM, I will just add
IFormattableimplementation toPyInt.support for BigInteger <-> PyInt
support for BigInteger <-> PyInt
make methods of PyObject inherited from its base C# classes GIL-safe
make methods of PyObject inherited from its base C# classes GIL-safe
make methods of PyObject inherited from its base C# classes GIL-safe
make methods of PyObject inherited from its base C# classes GIL-safe
make methods of PyObject inherited from its base C# classes GIL-safe
support for BigInteger <-> PyInt (#1710)