Skip to content

gh-151842: Make _PyXI_FreeExcInfo NULL-safe to fix crash on capture_exception() OOM (OOM-0031) #151843

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.

Sign up for GitHub

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
ZeroIntensity merged 4 commits into from
Jun 23, 2026
Merged

gh-151842: Make _PyXI_FreeExcInfo NULL-safe to fix crash on capture_exception() OOM (OOM-0031) #151843

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57a99d4
gh-151842: Make _PyXI_FreeExcInfo NULL-safe to fix crash on capture_e…
amruthamodela06 Jun 21, 2026
7d517f5
Fix NEWS rst: use inline code instead of :func: for internal _interpr…
amruthamodela06 Jun 21, 2026
48c87aa
Merge branch 'main' into fix-pyxi-freeexcinfo-null-guard
amruthamodela06 Jun 23, 2026
e4f031f
Address review: guard call site, raise MemoryError in _PyXI_NewExcInf…
amruthamodela06 Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2026-06-30-13-00-00.gh-issue-151842.OOM31g.rst
View file
Open in desktop

@StanFromIreland StanFromIreland Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need a news entry here either (I think we've decided that before too, I think it was on Jelle's PR), additionally it describes a lot of internal things (please see the devguide for more information about what we want in these entries).

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Fix a segfault in ``_interpreters.capture_exception()`` under memory
pressure. When ``_PyXI_NewExcInfo()`` failed to allocate, the cleanup path
called ``_PyXI_FreeExcInfo(NULL)``, which then dereferenced offset 0 in
``_excinfo_clear_type()``. ``_PyXI_FreeExcInfo()`` now accepts ``NULL`` like
the surrounding ``PyMem_RawFree`` idiom. Patch by Amrutha Modela.

@ZeroIntensity ZeroIntensity Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is too technical. Users reading the changelog will have no idea what this means. Let's just say something like "Fix crash when _interpreters.capture_memory runs out of memory."

5 changes: 5 additions & 0 deletions Python/crossinterp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,11 @@ _PyXI_NewExcInfo(PyObject *exc)
void
_PyXI_FreeExcInfo(_PyXI_excinfo *info)
{
if (info == NULL) {

@sobolevn sobolevn Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to not allow NULL values without a specific need. It might hide errors.

Please, take a look at #152013 You can copy my solution from there. I missed that you had a PR already and created my own one. But, I closed it, so you can contribute :)

This is not the only problem in the same function. _PyXI_NewExcInfo can also crash.

@amruthamodela06 amruthamodela06 Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sobolevn!! Really appreciated you closing #152013 and sharing your diff, especially the catch about _PyXI_NewExcInfo also being able to crash. The final fix is much better for addressing both bugs in one go. :)

// Matches the PyMem_RawFree(NULL) idiom: callers may pass NULL when
// _PyXI_NewExcInfo() failed (e.g. under OOM) before any allocation.
return;
}

@ZeroIntensity ZeroIntensity Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, but I don't think this is the right approach. Functions that do nothing under NULL don't make good APIs. To quote the Zen: "Errors should never pass silently".

To me, free() accepting NULL has always felt like a weird half-baked solution to error handling that can't be removed for the sake of backward compatibility. Rant aside, there are some real API-design consequences to this:

  1. A NULL pointer usually means there's some sort of underlying logic bug, and/or that something else has gone unchecked. Crashing on NULL forces the developer to double-check their error handling logic.
  2. This is inconsistent with the rest of the _PyXI_* API. I can't find anything else in _PyXI that has a no-op case when passed NULL.
  3. I'd argue that this encourages sloppy habit. _PyXI_FreeExcInfo accepting NULL makes it feel safer than it actually is. You still need to ensure that the memory is initialized and that it's called exactly once for every call to _PyXI_NewExcInfo.
  4. This one is nitpicky and might be optimized away, but every call to _PyXI_FreeExcInfo now has a performance tax of a NULL check. It's better to structure the code so that _PyXI_FreeExcInfo isn't called at all on an allocation failure, so a separate check isn't needed in the first place.

Please just add a NULL check at the call site, and perhaps add an info != NULL assertion here. It's hard to come up with concrete examples against this pattern when there's only a single use of the function, but I hope you respect my arguments as a maintainer.

_PyXI_excinfo_clear(info);
PyMem_RawFree(info);
}
Expand Down
Loading