Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
19 changes: 18 additions & 1 deletion Modules/_testexternalinspection.c
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ return_section_address(

cmd = (struct segment_command_64*)((void*)cmd + cmd->cmdsize);
}

// We should not be here, but if we are there, we should say about this
PyErr_SetString(
PyExc_RuntimeError, "Cannot find section address.\n");
return 0;
}

Expand Down Expand Up @@ -188,6 +192,7 @@ search_section_in_file(

munmap(map, fs.st_size);
if (close(fd) != 0) {
// This might hide one of the above exceptions, maybe we should chain them?
PyErr_SetFromErrno(PyExc_OSError);
}
return result;
Expand Down Expand Up @@ -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
Contributor Author

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 that task_for_pid may return code == 5 (os/kern failure) that we don't check - should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I think PyExc_PermissionError is just exactly for this. I will move it to pid_to_task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
return 0;
}

Expand Down Expand Up @@ -260,6 +267,8 @@ search_map_for_section(pid_t pid, const char* secname, const char* substr) {

address += size;
}

PyErr_SetString(PyExc_RuntimeError, "mach_vm_region failed");
return 0;
}

Expand Down Expand Up @@ -306,6 +315,7 @@ find_map_start_address(pid_t pid, char* result_filename, const char* map)

if (!match_found) {
map_filename[0] = '\0';
PyErr_Format(PyExc_RuntimeError, "Cannot find map start address for map: %s", map);
}

return result_address;
Expand Down Expand Up @@ -401,6 +411,7 @@ search_map_for_section(pid_t pid, const char* secname, const char* map)
static uintptr_t
search_map_for_section(pid_t pid, const char* secname, const char* map)
{
PyErr_SetString(PyExc_NotImplementedError, "Not supported on this platform");
return 0;
}
#endif
Expand Down Expand Up @@ -482,6 +493,9 @@ read_memory(pid_t pid, uintptr_t remote_address, size_t len, void* dst)
}
total_bytes_read = len;
#else
PyErr_SetString(
PyExc_RuntimeError,
"Memory reading is not supported on this platform");
return -1;
#endif
return total_bytes_read;
Expand Down Expand Up @@ -789,6 +803,9 @@ parse_coro_chain(
pid,
coro_address + offsets->gen_object.gi_frame_state,
&gi_frame_state);
if (err) {
return -1;
}

if (gi_frame_state == FRAME_SUSPENDED_YIELD_FROM) {
char owner;
Expand Down
Loading