Skip to content

BUG: Disown Python objects adopted by unique_ptr members - #6707

Open
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-boundary-condition-ownership
Open

BUG: Disown Python objects adopted by unique_ptr members#6707
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:enh-boundary-condition-ownership

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Jul 26, 2026

Copy link
Copy Markdown
Member

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 (current main, before this PR)
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 PR, 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
GradientImageFilter::OverrideBoundaryCondition m_BoundaryCondition.reset(boundaryCondition) (itkGradientImageFilter.hxx:45)
OptimizerParameters::SetHelper m_Helper.reset(helper) (itkOptimizerParameters.h:140)

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.

These two are the complete set of public methods with this pattern. A scan for m_*.reset(<parameter>) across headers and sources finds three further adopters — SetCostFunctionAdaptor on SingleValuedNonLinearVnlOptimizer, MultipleValuedNonLinearVnlOptimizer, and SingleValuedNonLinearVnlOptimizerv4 — but all three are protected. They appear in the generated .i files inside protected: blocks, SWIG does not wrap protected members, and the generated Python modules contain no reference to them, so they are not reachable from Python and are out of scope here.

Why DISOWN rather than a C++ signature change

Changing the signature to std::unique_ptr would express ownership in the type system, which is the better long-term API. That is deliberately not this PR — it is an API change requiring deprecation of the existing overload, and it is not backportable. This PR is the minimal, backportable crash fix against the API as it exists.

The follow-up API work is planned separately and supersedes the approach in #4533.

Test plan

Two new Python tests, one per affected module:

  • Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py
  • Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py

Each asserts the thisown transition and then exercises the object. 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 main even if the assertions are removed.

Both built and run locally in a ITK_WRAP_PYTHON=ON tree before pushing. pre-commit run --all-files passes.

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 marked this pull request as ready for review July 26, 2026 16:17
@greptile-apps

greptile-apps Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes 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 transitions to false and that processes exit without double-free failures.
  • Registers both ownership tests with their modules’ CTest configuration.

Confidence Score: 5/5

The PR appears safe to merge, with ownership transfer correctly aligned between the Python proxies and the adopting C++ members.

The parameter-specific SWIG typemaps clear Python ownership before the raw pointers are adopted by their corresponding unique-pointer members, and focused tests cover both ownership transitions and the prior shutdown failure.

T-Rex T-Rex Logs

What T-Rex did

  • Reviewed the general-contract-validation-proof and confirmed this run is environment-blocked because a valid generated wrapper build cannot be supplied in this environment.
  • Compared the two environment logs and noted they capture the build-marker checks and the reason for the failure (exit status 127) when attempting to exercise the wrapper build.
  • Concluded that the overall proof is environment-blocked rather than a runtime proof of changed SWIG ownership behavior.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/Core/Common/wrapping/itkOptimizerParameters.wrap Applies a parameter-specific DISOWN typemap matching the helper type and SetHelper parameter name.
Modules/Core/Common/wrapping/test/itkOptimizerParametersOwnershipTest.py Verifies ownership transfer for SetHelper, with process termination covering the previous shutdown double-free.
Modules/Filtering/ImageGradient/wrapping/itkGradientImageFilter.wrap Applies DISOWN to boundary-condition arguments for the wrapped image specializations.
Modules/Filtering/ImageGradient/wrapping/test/itkGradientImageFilterOwnershipTest.py Verifies ownership transfer and confirms the filter remains operational with the adopted boundary condition.
Modules/Core/Common/wrapping/test/CMakeLists.txt Registers the optimizer ownership regression test when Python wrapping is enabled.
Modules/Filtering/ImageGradient/wrapping/test/CMakeLists.txt Registers the gradient-filter ownership test under its required Python, float, and 2D wrapping configuration.

Sequence Diagram

sequenceDiagram
participant Python as Python proxy
participant SWIG as SWIG wrapper
participant Cpp as C++ object
participant Member as unique_ptr member
Python->>SWIG: SetHelper / OverrideBoundaryCondition(argument)
SWIG->>Python: Clear thisown via DISOWN
SWIG->>Cpp: Pass raw pointer
Cpp->>Member: reset(pointer)
Note over Python,Member: C++ member becomes the sole owner
Member-->>Member: Delete adopted object during replacement or destruction
Loading

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

@dzenanz dzenanz left a comment

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.

Looks good on a glance. It would be good if someone else reviewed this too.

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.

2 participants