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

Core dump with specific call of find_frozen #126171

Closed
federicovalenso opened this issue Oct 30, 2024 · 16 comments
Closed

Core dump with specific call of find_frozen #126171

federicovalenso opened this issue Oct 30, 2024 · 16 comments
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes easy interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-importlib type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@federicovalenso
Copy link
Contributor

federicovalenso commented Oct 30, 2024

Crash report

What happened?

>>> from importlib import _imp
>>> _imp.find_frozen("zipimport", withdata=True)
python3: Objects/memoryobject.c:733: PyMemoryView_FromMemory: Assertion `mem != ((void *)0)' failed.
Aborted (core dumped)

python is configured with --with-pydebug --with-trace-refs --with-assertions

I think it happens here

CPython versions tested on:

3.11

Operating systems tested on:

Linux

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

Python 3.11.9 (main, Oct 30 2024, 09:46:33) [GCC 13.2.0]

Linked PRs

@federicovalenso federicovalenso added the type-crash A hard crash of the interpreter, possibly with a core dump label Oct 30, 2024
@picnixz picnixz added topic-importlib 3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Oct 30, 2024
@sobolevn
Copy link
Member

@federicovalenso how do you find these problems? Do you use some kind of tooling for that?

@ZeroIntensity
Copy link
Member

I'm guessing it's a static analyzer. Anyways, I can take a look at this later today, unless @picnixz has already started working on it.

@picnixz
Copy link
Contributor

picnixz commented Oct 30, 2024

I'm guessing it's a static analyzer. Anyways, I can take a look at this later today, unless @picnixz has already started working on it.

Please do so! I have some ongoing work elsewhere but I don't mind reviewing it!

@federicovalenso
Copy link
Contributor Author

I'm sorry for late answer.

I'm guessing it's a static analyzer.

@sobolevn , @ZeroIntensity , you're right.

@ZeroIntensity
Copy link
Member

We're fine with it as long as the crashes are actual bugs and not false positives (there have been false positive reports from static analyzers here in the past). But you seem to know what's real and what's not, keep doing what you're doing :)

@picnixz
Copy link
Contributor

picnixz commented Oct 31, 2024

@ZeroIntensity I have some time now; do you want me to take on that task or were you already working on it?

@ZeroIntensity
Copy link
Member

Go for it, I didn't get to this yesterday. I'll be happy to review your PR!

@picnixz picnixz self-assigned this Oct 31, 2024
@picnixz
Copy link
Contributor

picnixz commented Oct 31, 2024

Actually, I couldn't reproduce it on 3.12+ (even with the correct flags). It appears that it has been patched at some point. @federicovalenso can you 1) verify this 2) try to run your static analyzer tool on 3.12+ builds instead please? Thank you in advance!

@picnixz picnixz added the pending The issue will be closed if no feedback is provided label Oct 31, 2024
@federicovalenso
Copy link
Contributor Author

@picnixz , yeah, I'll do that, but it requires some time.

@picnixz

This comment was marked as resolved.

@sobolevn
Copy link
Member

sobolevn commented Nov 1, 2024

Otherwise, in order not to have a pending label for too long, I'd suggest closing this issue for now and come back later.

There's no real problem in having an issue with the "pending" label :)
Let's resolve the issue first: with finding the proper reproducer or with finding that it was fixed already. And then we can completelly close the issue.

@federicovalenso
Copy link
Contributor Author

@picnixz , I couldn't reproduce it on 3.12+ too. Also I did static analysis, it still continues to complain about the same place in the code.

@picnixz
Copy link
Contributor

picnixz commented Nov 6, 2024

Also I did static analysis, it still continues to complain about the same place in the code.

What does it say for this line? does it say that it's unsafe, or can lead to a core dump? (or something like that?)

@federicovalenso
Copy link
Contributor Author

federicovalenso commented Nov 8, 2024

@picnixz , sorry for late answer. Analyzer doesn't like this line. It states that data is previously assigned to NULL, and in case of withdata == False dereference in Py_DECREF happens. It can happen when origname == NULL, but I can't reproduce it. Then I look around I found another, more serious problem with crash.

@picnixz
Copy link
Contributor

picnixz commented Nov 8, 2024

Ah I see. Yes, we should use a Py_XDECREF just in case.

@picnixz picnixz added easy and removed pending The issue will be closed if no feedback is provided labels Nov 8, 2024
@picnixz picnixz removed their assignment Nov 8, 2024
@picnixz
Copy link
Contributor

picnixz commented Nov 8, 2024

@federicovalenso If you want to make a PR, go ahead, otherwise I'll do it a bit later.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 8, 2024
kumaraditya303 pushed a commit that referenced this issue Nov 8, 2024
…mpl (GH-126566) (#126567)

gh-126171: fix possible null dereference in _imp_find_frozen_impl (GH-126566)
(cherry picked from commit 9ecd8f7)

Co-authored-by: Valery Fedorenko <[email protected]>
kumaraditya303 pushed a commit that referenced this issue Nov 8, 2024
…mpl (GH-126566) (#126568)

gh-126171: fix possible null dereference in _imp_find_frozen_impl (GH-126566)
(cherry picked from commit 9ecd8f7)

Co-authored-by: Valery Fedorenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes easy interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-importlib type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Development

No branches or pull requests

5 participants