Visitar URL original
Using String.Format crashes inside pythonnet · Issue #1642 · pythonnet/pythonnet · GitHub
Skip to content

Using String.Format crashes inside pythonnet #1642

Closed
#1711
#1710
Closed
Using String.Format crashes inside pythonnet#1642
#1711
#1710
Assignees
lostmsu
Labels
proposal
Milestone
3.0.0

Description

@sekkit
sekkit
opened on Dec 21, 2021

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

  1. filmor commented on Dec 22, 2021

    @filmor
    filmor
    on Dec 22, 2021
    Member

    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.

  2. filmor
    closed this as completedon Dec 22, 2021

  3. sekkit commented on Dec 22, 2021

    @sekkit
    sekkit
    on Dec 22, 2021
    Author

    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.

    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

  4. sekkit commented on Dec 23, 2021

    @sekkit
    sekkit
    on Dec 23, 2021
    Author

    System.AccessViolationException: 'Attempted to read or write protected memory. This is often an indication that other memory is corrupt.'
    [External Code]

    Python.Runtime.dll!Python.Runtime.Runtime.PyErr_Occurred() Line 1853 C#
    Python.Runtime.dll!Python.Runtime.Exceptions.ErrorOccurred() Line 315 C#
    Python.Runtime.dll!Python.Runtime.Runtime.AssertNoErorSet() Line 1130 C#
    Python.Runtime.dll!Python.Runtime.Runtime.PyObject_Str(Python.Runtime.BorrowedReference pointer) Line 1122 C#
    Python.Runtime.dll!Python.Runtime.PyObject.ToString() Line 1054 C#
    [External Code]
    Python.Runtime.dll!Python.Runtime.MethodBinder.Invoke(Python.Runtime.BorrowedReference inst, Python.Runtime.BorrowedReference args, Python.Runtime.BorrowedReference kw, System.Reflection.MethodBase info, System.Reflection.MethodInfo[] methodinfo) Line 919 C#
    Python.Runtime.dll!Python.Runtime.MethodObject.Invoke(Python.Runtime.BorrowedReference target, Python.Runtime.BorrowedReference args, Python.Runtime.BorrowedReference kw, System.Reflection.MethodBase info) Line 68 C#
    Python.Runtime.dll!Python.Runtime.MethodBinding.tp_call(Python.Runtime.BorrowedReference ob, Python.Runtime.BorrowedReference args, Python.Runtime.BorrowedReference kw) Line 238 C#
    [External Code]
    Python.Runtime.dll!Python.Runtime.Runtime.PyRun_String(string code, Python.Runtime.RunFlagType st, Python.Runtime.BorrowedReference globals, Python.Runtime.BorrowedReference locals) Line 941 C#
    Python.Runtime.dll!Python.Runtime.PythonEngine.RunString(string code, Python.Runtime.BorrowedReference globals, Python.Runtime.BorrowedReference locals, Python.Runtime.RunFlagType flag) Line 670 C#
    Python.Runtime.dll!Python.Runtime.PythonEngine.Exec(string code, Python.Runtime.PyDict globals, Python.Runtime.PyObject locals) Line 578 C#
    nPython.exe!Python.Runtime.PythonConsole.Main(string[] args) Line 36 C#

  5. lostmsu
    reopened this on Dec 23, 2021

  6. lostmsu commented on Dec 23, 2021

    @lostmsu
    lostmsu
    on Dec 23, 2021
    Member

    I can confirm this reproduces. I believe the reason is that we no longer automatically convert PyInt to System.Int32, because it is an unsafe conversion due to potential overflows.

    Instead, you should do String.Format('{0},{1}', str(1), str(2)) or

    from System import Int32, String
String.Format('{0},{1}', Int32(1), Int32(2))

    In general though this was always a problem with any Python objects passed to methods like this. String.Format does not acquire GIL when calling ToString on 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 PyObject instances into a nearly identical type, that delegates all its members to PyObject, 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.

  7. filmor commented on Dec 23, 2021

    @filmor
    filmor
    on Dec 23, 2021
    Member

    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 PyObject implements (i.e. either work properly or throw), so all methods of object and DynamicObject, and maybe IDisposable and ISerializable.

  8. filmor
    added this to the 3.0.0 milestone on Dec 23, 2021

  9. lostmsu commented on Dec 23, 2021

    @lostmsu
    lostmsu
    on Dec 23, 2021
    Member

    @filmor anything will crash the interpreter without acquiring GIL.

  10. filmor commented on Dec 23, 2021

    @filmor
    filmor
    on Dec 23, 2021
    Member

    I am well aware of that :). I just think that it shouldn't be possible to crash the interpreter (easily) from within Python.

  11. lostmsu commented on Dec 23, 2021

    @lostmsu
    lostmsu
    on Dec 23, 2021
    Member

    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_gil wrapper for unsafe calls. E.g.

    String.Format('{0},{1}', 1, 2) # this would be OK
clr.no_gil(String.Format)('{0},{1}', 1, 2) # would crash as in the current issue

    But frankly I think we should just keep the current behavior, or add clr.with_gil so that clr.with_gil(String.Format)('{0},{1}', 1, 2) would not crash.

  12. sekkit commented on Dec 24, 2021

    @sekkit
    sekkit
    on Dec 24, 2021
    Author

    Any conclusions with this issue? Are we going to leave it as it is or alter the usage of such case?

  13. filmor commented on Dec 24, 2021

    @filmor
    filmor
    on Dec 24, 2021
    Member

    I definitely want to have this fixed before we consider releasing an rc, but I won't think about this over the holidays :)

  14. lostmsu commented on Dec 24, 2021

    @lostmsu
    lostmsu
    on Dec 24, 2021
    Member

    @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 (use str(val), then the arguments are converted to strings by Python before being passed to String.Format), they are guaranteed to work in any case.

  15. lostmsu
    added
    proposal
    on Dec 24, 2021

  16. lostmsu commented on Jan 6, 2022

    @lostmsu
    lostmsu
    on Jan 6, 2022
    Member

    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.

  17. filmor commented on Jan 7, 2022

    @filmor
    filmor
    on Jan 7, 2022
    Member

    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 bool should 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 in PyObject check for the GIL and either acquire it or throw.

  18. filmor commented on Mar 3, 2022

    @filmor
    filmor
    on Mar 3, 2022
    Member

    We decided that we are going to GIL-protect all overridden object and DynamicObject methods.

  19. lostmsu
    self-assigned this
    on Mar 3, 2022

  20. lostmsu commented on Mar 3, 2022

    @lostmsu
    lostmsu
    on Mar 3, 2022
    Member

    @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 IFormattable implementation to PyInt.

  21. lostmsu
    added 6 commits that reference this issue on Mar 3, 2022

    support for BigInteger <-> PyInt

    4ddb38f

    support for BigInteger <-> PyInt

    0eff192

    make methods of PyObject inherited from its base C# classes GIL-safe

    0d51486

    make methods of PyObject inherited from its base C# classes GIL-safe

    4c21778

    make methods of PyObject inherited from its base C# classes GIL-safe

    12501f8

    make methods of PyObject inherited from its base C# classes GIL-safe

    b988292
  22. lostmsu
    closed this as completedin #1711on Mar 4, 2022
  23. lostmsu
    added a commit that references this issue on Mar 4, 2022

    make methods of PyObject inherited from its base C# classes GIL-safe

    eec30c0
  24. filmor
    added a commit that references this issue on Apr 8, 2022

    support for BigInteger <-> PyInt (#1710)

    0e57cdd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

proposal

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions