Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_PyFrame_Clear can crash in for loop if frame->localsplus is NULL. #119128

Closed
AraHaan opened this issue May 17, 2024 · 6 comments
Closed

_PyFrame_Clear can crash in for loop if frame->localsplus is NULL. #119128

AraHaan opened this issue May 17, 2024 · 6 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@AraHaan
Copy link
Contributor

AraHaan commented May 17, 2024

Crash report

What happened?

When I was debugging my C extension I eventually found a way to get it to work using the interactive debug version of the interpreter and using _PyObject_Dump to also check the reference counts of everything to ensure my extension module works properly and has proper reference counting.

However when I typed in exit() to close the interpreter I crashed in _PyFrame_Clear within the for loop as it fails to check first if frame->localsplus is not NULL before attempting to index into it resulting in an Access Violation.

This should be a trivial patch. Issue is how come frame->localsplus is NULL to begin with?

CPython versions tested on:

3.11, 3.12, CPython main branch

Operating systems tested on:

Windows

Output from running 'python -VV' on the command line:

python_d.exe -X dev (no script file for interactive console mode)

@AraHaan AraHaan added the type-crash A hard crash of the interpreter, possibly with a core dump label May 17, 2024
@sobolevn sobolevn added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 19, 2024
@iritkatriel
Copy link
Member

CC @markshannon

@markshannon
Copy link
Member

frame->localsplus is an array, not a pointer. It cannot be NULL.

Can you provide a way to reproduce the issue?
It sounds like your C extension is corrupting memory.

@AraHaan
Copy link
Contributor Author

AraHaan commented May 24, 2024

frame->localsplus is an array, not a pointer. It cannot be NULL.

Can you provide a way to reproduce the issue? It sounds like your C extension is corrupting memory.

Yeah I have code in a zip file that can help reproduce it if you want to build it and help debug it. It got so hard to find the issue that even Visual Studio with all the tools it has is no help at all.

Note:

  • includes a database with mocked up values (all fake)
  • python code
  • c extension code
  • a test file to debug with.

To Build:

  • py -m pip install -r requirements.txt (make sure it goes into %AppData%\Python\PythonXY\site-packages\ on Windows)
  • py -m pip install -e .

DiscordBot.zip

@iritkatriel
Copy link
Member

@AraHaan We do not run code on our system without reading it first to see what it will do. So sending us a multi-file application in a zip file is a big ask.

As @markshannon indicated, this is probably a bug in your program. I suggest that you debug it with a view to determining whether this is indeed memory corruption in your code. If you think this is a cpython bug, please narrow down your code to at most one C file and one python file, with no dependencies, which demonstrate the issue.

@iritkatriel iritkatriel added the pending The issue will be closed if no feedback is provided label May 24, 2024
@devdanzin
Copy link

Yeah I have code in a zip file that can help reproduce it if you want to build it and help debug it.

You have a lot of unnecessary code in that zip file. Here's a much reduced version: dbot_reduced2.zip

It seems the issue in the reduced example comes from get_tiers, but it doesn't manifest by calling:

results = await __get_results('SELECT * FROM V_Tiers')
print[create_tier(result) for result in results]

So it probably comes from get_tiers itself, list_results_cb or code from pyawaitable.

Here's the simplified code for test_Database.py:

import asyncio
from DiscordBot._capi import get_tiers

async def main():
    # test what these print.
    print(1)
    print(await get_tiers())
    # print(await get_tiers())
    # print(await get_tiers())
    print(2)

if __name__ == '__main__':
    print(0)
    asyncio.run(main())
    print(3)

By running python -X dev test_Database.py with a single call to get_tiers, I get the following output:

0
1
[<_capi.Tier object at 0x000002329290AE30>, <_capi.Tier object at 0x0000023294E3C4A0>, <_capi.Tier object at 0x0000023294E3C130>, <_capi.Tier object at 0x0000023294E3C4F0>, <_capi.Tier object at 0x0000023294E3C590>, <_capi.Tier object at 0x0000023294E3C630>]
2
3
Windows fatal exception: access violation

Current thread 0x00004f9c (most recent call first):
  Garbage-collecting
  <no Python frame>

But if I call get_tiers three times, the "2" and "3" are never printed, meaning Windows fatal exception: access violation happens earlier. Also, the <no Python frame> isn't printed in that case.

I hope this helps finding the root cause.

@AraHaan
Copy link
Contributor Author

AraHaan commented May 25, 2024

I think the issue was in line 70 where I do PyObject *result2 = PyObject_CallFunction(method, "N", result); instead of PyObject *result2 = PyObject_CallFunction(method, "O", result); because PyList_GetItem returns a borrowed reference. 😄

@iritkatriel iritkatriel closed this as not planned Won't fix, can't repro, duplicate, stale May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) pending The issue will be closed if no feedback is provided type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

5 participants