Skip to content

BUG: Backport release 5.4 Disown Python objects adopted by unique_ptr members - #6712

Open
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-5.4-disown-unique-ptr-adopters
Open

BUG: Backport release 5.4 Disown Python objects adopted by unique_ptr members #6712
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-5.4-disown-unique-ptr-adopters

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

Backport of #6707 to release-5.4. Two wrapped ITK methods adopt their raw-pointer argument into a std::unique_ptr member, but the Python wrapping left ownership with the proxy object. Both sides free it, and the interpreter aborts at shutdown. Fixed by applying SWIGTYPE *DISOWN to both parameters; no C++ change.

Affects itk.GradientImageFilter.OverrideBoundaryCondition and itk.OptimizerParameters.SetHelper.

Reproducer (release-5.4, before this change)
import itk
ImageType = itk.Image[itk.F, 2]
filt = itk.GradientImageFilter[ImageType, itk.F, itk.F].New()
bc = itk.PeriodicBoundaryCondition[ImageType]()
filt.OverrideBoundaryCondition(bc)
print(bc.thisown)   # True  <-- Python still owns it
# interpreter dumps core at shutdown
import itk
p = itk.OptimizerParameters[itk.D](3)
h = itk.OptimizerParametersHelper[itk.D]()
p.SetHelper(h)
print(h.thisown)    # True  <-- Python still owns it
# interpreter dumps core at shutdown

After this change, thisown is False in both cases and the interpreter exits cleanly.

Root cause

Both methods take a raw pointer and immediately adopt it:

Method Body Location in release-5.4
GradientImageFilter::OverrideBoundaryCondition m_BoundaryCondition.reset(boundaryCondition) itkGradientImageFilter.hxx:45
OptimizerParameters::SetHelper m_Helper.reset(helper) itkOptimizerParameters.h:142

OptimizerParameters::SetHelper documents the intent explicitly: "OptimizerParameters manages the helper once its been assigned."

SWIG's default typemap for a raw pointer parameter is non-owning, so the Python proxy keeps thisown == True. The C++ unique_ptr and the Python proxy then both destroy the object.

Test plan — built and run against release-5.4

This was not assumed to apply from main; it was built and tested on a release-5.4 worktree.

Stage Result
Full ITK_WRAP_PYTHON=ON build of release-5.4 8818/8818 targets, 0 failures
Rebuild after pinning the interpreter 3355/3355 targets, 0 failures
itkGradientImageFilterOwnershipPythonTest Passed
itkOptimizerParametersOwnershipPythonTest Passed
Full release-5.4 Python suite (ctest -R Python) 168/168 passed

Because the double-free manifests at interpreter shutdown, a non-zero exit code is itself part of the regression check — these tests fail on unpatched release-5.4 even with the assertions removed.

Differences from the main-branch change

The .wrap and .py content is identical to #6707. Only the two wrapping/test/CMakeLists.txt hunks differ, because release-5.4 predates the gersemi restyle — the new itk_python_add_test calls follow 5.4's local formatting (NAME on its own line, closing paren attached to the last argument).

release-5.4 has no .pre-commit-config.yaml, so the pre-commit hooks were not run against this branch.

GradientImageFilter::OverrideBoundaryCondition and
OptimizerParameters::SetHelper store their raw-pointer argument in a
unique_ptr member, but the Python wrapping left ownership with the proxy.
Both sides freed the object and the interpreter aborted at shutdown.

Apply SWIGTYPE *DISOWN to both parameters and add Python regression tests.
@github-actions github-actions Bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module labels Jul 26, 2026
@hjmjohnson hjmjohnson changed the title BUG: Disown Python objects adopted by unique_ptr members BUG: Backport release 5.4 Disown Python objects adopted by unique_ptr members Jul 26, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review July 26, 2026 19:18
@greptile-apps

greptile-apps Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This backport corrects Python ownership transfer for two raw-pointer arguments adopted by C++ unique_ptr members.

  • Applies SWIG DISOWN typemaps to OptimizerParameters.SetHelper and GradientImageFilter.OverrideBoundaryCondition.
  • Adds Python regression tests that verify thisown changes to false and that the gradient filter remains usable.
  • Registers both tests with their module-specific CTest configuration.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The typemaps target the raw-pointer parameters that the corresponding setters immediately adopt into unique_ptr members, and the added tests cover ownership transfer and clean runtime use.

T-Rex T-Rex Logs

What T-Rex did

  • Examined the blocker log to verify the exact CTest command issued, the working directory, the command-not-found error, and the exit status 127.
  • Reviewed the supporting diagnostics to confirm missing build tools, the absence of a reusable build tree, and the missing Python NumPy dependency.
  • Compiled and labeled the blocker and diagnostic artifacts for reviewer access.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/Core/Common/wrapping/itkOptimizerParameters.wrap Applies the ownership-transfer typemap to the correctly named helper pointer adopted by SetHelper.
Modules/Core/Common/wrapping/test/CMakeLists.txt Registers the new optimizer ownership regression test under Python wrapping builds.
Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py Verifies that SetHelper relinquishes Python ownership of a newly constructed helper.
Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap Applies the ownership-transfer typemap across the wrapped image types used by OverrideBoundaryCondition.
Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt Registers the gradient ownership test when its required float and two-dimensional wrapping configuration is enabled.
Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py Verifies ownership transfer and exercises the filter after adopting the boundary condition.

Sequence Diagram

sequenceDiagram
participant Python as Python proxy
participant SWIG as SWIG wrapper
participant Native as C++ object
Python->>SWIG: Pass owned helper/boundary condition
SWIG->>Python: Clear thisown via DISOWN
SWIG->>Native: Invoke adopting setter(raw pointer)
Native->>Native: unique_ptr.reset(pointer)
Note over Python,Native: Native object becomes the sole owner
Loading

Reviews (1): Last reviewed commit: "BUG: Disown Python objects adopted by un..." | Re-trigger Greptile

@hjmjohnson
hjmjohnson requested a review from thewtex July 27, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Python wrapping Python bindings for a class type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant