Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-130052: Fix some exceptions on error paths in _testexternalinspection#130053

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

sergey-miryanov
Copy link
Contributor

@sergey-miryanovsergey-miryanov commentedFeb 12, 2025•
edited by bedevere-appbot
Loading

Some error paths don't set Exceptions.
This PR fixes them.

@sergey-miryanov
Copy link
ContributorAuthor

sergey-miryanov commentedFeb 13, 2025•
edited
Loading

read_ptr(pid,task_address+sizeof(Py_ssize_t),&refcnt);

Refcnt doesn't used - maybe we may remove this reading?

"get_stack_trace is not supported on this platform");

This is a typo - should I fix them?get_stack_trace should beget_async_stack_trace

@sergey-miryanov
Copy link
ContributorAuthor

@pablogsal Please take a look. I saw your fixed some calls (#129557), I added a few more exceptions. I also added a check of the result before it is used.

@vstinner
Copy link
Member

@sergey-miryanov: Why did you mark this PR as a draft? Do you expect to add more changes? Or is there something else?

@sergey-miryanov
Copy link
ContributorAuthor

sergey-miryanov commentedFeb 17, 2025•
edited
Loading

@vstinner oops! I forgot to un-draft it. Thanks!

@sergey-miryanovsergey-miryanov marked this pull request as ready for reviewFebruary 17, 2025 13:13
@@ -217,7 +222,9 @@ search_map_for_section(pid_t pid, const char* secname, const char* substr) {

mach_port_t proc_ref = pid_to_task(pid);
if (proc_ref == 0) {
PyErr_SetString(PyExc_PermissionError, "Cannot get task for PID");
if (!PyErr_Occurred()) {
PyErr_SetString(PyExc_PermissionError, "Cannot get task for PID");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would it be posssible to move this code inside pid_to_task()? I'm not convinced that PermissionError is appropriate, I would suggest RuntimeError instead.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I believe it is redundant here. I will remove this check. But I saw on the internets thattask_for_pid may return code == 5 (os/kern failure) that we don't check - should we?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh! I thinkPyExc_PermissionError is just exactly for this. I will move it topid_to_task.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I keptPyExc_PermissionError and removed redundant check. Should I replace PermissionError with RuntimeError?

@sergey-miryanov
Copy link
ContributorAuthor

@vstinner Please take a look.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM.

@vstinnervstinner added the needs backport to 3.13bugs and security fixes labelFeb 20, 2025
@vstinnervstinnerenabled auto-merge (squash)February 20, 2025 17:05
@vstinnervstinner merged commit69426fc intopython:mainFeb 20, 2025
47 checks passed
@miss-islington-app
Copy link

Thanks@sergey-miryanov for the PR, and@vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry,@sergey-miryanov and@vstinner, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 69426fcee7fcecbe34be66d2c5b58b6d0ffe2809 3.13

@sergey-miryanov
Copy link
ContributorAuthor

@vstinner It looks like a lot of changes were made on 3.14 branch and not on 3.13. Should we backport this PR then?

@vstinner
Copy link
Member

The automated backport failed because of merge conflicts.

@sergey-miryanov: Can you please try to backport this change manually to 3.13?

@sergey-miryanov
Copy link
ContributorAuthor

sergey-miryanov commentedFeb 21, 2025•
edited
Loading

@vstinner I tried yesterday yet. There are commits that not backported:

* 69426fcee7f gh-130052: Fix some exceptions on error paths in _testexternalinspection (#130053)* 49b11033bd8 GH-91048: Correct error path in testexternalinspection (#129557)* 633853004c5 gh-130050: Fix memory leaks in _testexternalinspection (#130051)* 7eaef74561c gh-129430: Make walking vm regions more efficient in MacOS (#129494)* be98fda7c66 gh-129223: Raise KeyError in search_map_for_section() if not found (#129262)* 3a3a6b86f40 gh-129223: Do not allow the compiler to optimise away symbols for debug sections (#129225)* 67d804b494d GH-91048: Don't attempt to run on FreeBSD (#129189)* 188598851d5 GH-91048: Add utils for capturing async call stack for asyncio programs and enable profiling (#124640)* f5b6356a11b GH-128563: Add new frame owner type for interpreter entry frames (GH-129078)* 6d93690954d gh-125604: Move _Py_AuditHookEntry, etc. Out of pycore_runtime.h (gh-125605)* b2afe2aae48 gh-123923: Defer refcounting for `f_executable` in `_PyInterpreterFrame` (#123924)

Should I backport them all?

@vstinner
Copy link
Member

@pablogsal:Modules/_testexternalinspection.c lacks many backports in the 3.13 branch. Is it a deliberate choice? Or should we backport these changes to 3.13?

@pablogsal
Copy link
Member

@pablogsal:Modules/_testexternalinspection.c lacks many backports in the 3.13 branch. Is it a deliberate choice? Or should we backport these changes to 3.13?

3.13 has half the test cases because there is no async-related introspection and most of the bugs we have been fixing are in the new code. I will try to go over the PRs and see if anything applies to the other code that we had

vstinner reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner approved these changes

Assignees

@vstinnervstinner

Labels
needs backport to 3.13bugs and security fixesskip newstestsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@sergey-miryanov@vstinner@pablogsal@encukou
[8]ページ先頭
©2009-2025 Movatter.jp