Fix memory leak from repeated ProjDataInfo::clone() calls #1673

NikEfth · 2025-12-23T09:23:02Z

This PR fixes a memory leak detected by AddressSanitizer in ScatterSimulation::set_template_proj_data_info.
The issue was caused by calling arg.clone() multiple times inside dynamic_cast expressions, leading to leaked heap allocations when the cast failed.

The fix:
• Calls clone() exactly once
• Transfers ownership safely using a smart pointer
• Preserves the existing runtime type checks for supported ProjDataInfo types

This change is purely a memory-safety fix and does not alter behaviour or interfaces.

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

I have performed a self-review of my code
[] I have added docstrings/doxygen in line with the guidance in the developer guide
[] I have implemented unit tests that cover any new or modified functionality (if applicable)
The code builds and runs on my machine
[] documentation/release_XXX.md has been updated with any functionality change (if applicable)

Contribution Notes

Please tick the following:

The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in STIR (the Work) under the terms and conditions of the Apache-2.0 License.
I (or my institution) have signed the STIR Contribution License Agreement (not required for small changes).

NikEfth · 2025-12-23T09:56:03Z

Based on CODACY suggestion I simplified a bit the set_template_proj_data_info() to use the shared_ptr

KrisThielemans

I have flagged some minor style issues. In addition, it'd be best to add an overload for backwards compatibility.

void set_template_proj_data_info(const ProjDataInfo&);

which would then have to do the clone and call the other one, e.g.

set_template_proj_data_info(arg.make_shared_clone());

It's possible that you'd have to %ignore the new one in SWIG though.

KrisThielemans · 2026-01-19T17:19:20Z

src/recon_test/test_blocks_on_cylindrical_projectors.cxx

  write_to_file("image_for", *image1_sptr);

-  image = *image.get_empty_copy();
+  image.fill(0); //= *image.get_empty_copy();


I'm happy with this, but let's remove the comment.

Suggested change

image.fill(0); //= *image.get_empty_copy();

image.fill(0);

KrisThielemans · 2026-01-19T17:20:20Z

src/recon_test/test_blocks_on_cylindrical_projectors.cxx

  //    rotate by 30 degrees
  phi2 = 30 * _PI / 180;
-  VoxelsOnCartesianGrid<float> image2 = *image.get_empty_copy();
+  VoxelsOnCartesianGrid<float> image2(image); // = *image.get_empty_copy();


I see no good reason to do this, although I don't mind. (Should be equivalent, no?). If you prefer it this way, please delete the comment.

KrisThielemans · 2026-01-19T17:20:33Z

src/recon_test/test_blocks_on_cylindrical_projectors.cxx

  write_to_file("image_to_fwp", *image1_sptr);

-  image = *image.get_empty_copy();
+  image.fill(0); //= *image.get_empty_copy();


delete comment

KrisThielemans · 2026-01-19T17:20:38Z

src/recon_test/test_blocks_on_cylindrical_projectors.cxx

  write_to_file("image_with_voxel_at_30_0", *image1_sptr);

-  image = *image.get_empty_copy();
+  image.fill(0); //= *image.get_empty_copy();


delete comment

KrisThielemans · 2026-01-19T17:22:43Z

src/scatter_buildblock/ScatterSimulation.cxx

+    if (dynamic_cast<const ProjDataInfoCylindricalNoArcCorr*>(proj_data_info_sptr.get()))
      {
-        auto ptr = dynamic_cast<ProjDataInfoCylindricalNoArcCorr*>(proj_data_info_sptr.get());
+        const auto* ptr = dynamic_cast<const ProjDataInfoCylindricalNoArcCorr*>(proj_data_info_sptr.get());


I'm a bit confused that this works. const auto is a bit better than auto, but why make things more complicated by adding the *? It works without it (and means the same thing, I believe).

Same comment below of course.

KrisThielemans · 2026-01-19T17:23:42Z

src/scatter_buildblock/ScatterSimulation.cxx


 void
-ScatterSimulation::set_template_proj_data_info(const ProjDataInfo& arg)
+ScatterSimulation::set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg)


Suggested change

ScatterSimulation::set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg)

ScatterSimulation::set_template_proj_data_info(shared_ptr<const ProjDataInfo> arg)

Of course, stir::shared_ptr is now std::shared_ptr, but we use this idiom everywhere...

KrisThielemans · 2026-01-19T17:24:33Z

src/scatter_buildblock/scatter_detection_modelling.cxx

 #endif
  CartesianCoordinate3D<float> detector_coord_A, detector_coord_B;
-  auto ptr = dynamic_cast<ProjDataInfoBlocksOnCylindricalNoArcCorr*>(proj_data_info_sptr.get());
+  const auto* ptr = dynamic_cast<const ProjDataInfoBlocksOnCylindricalNoArcCorr*>(proj_data_info_sptr.get());


same comment on const auto*. I'd just use const auto

KrisThielemans · 2026-01-19T17:30:13Z

src/include/stir/scatter/ScatterSimulation.h

  void set_template_proj_data_info(const std::string&);

-  void set_template_proj_data_info(const ProjDataInfo&);
+  void set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg);


Suggested change

void set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg);

void set_template_proj_data_info(shared_ptr<const ProjDataInfo> arg);

to be consistent with STIR styling

Fix memory leak from repeated ProjDataInfo::clone() calls

42104a5

NikEfth assigned NikEfth and KrisThielemans Dec 23, 2025

NikEfth added the bug label Dec 23, 2025

Memory leak fixes.

149fb55

KrisThielemans requested changes Jan 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak from repeated ProjDataInfo::clone() calls #1673

Fix memory leak from repeated ProjDataInfo::clone() calls #1673

NikEfth commented Dec 23, 2025

Uh oh!

NikEfth commented Dec 23, 2025

Uh oh!

KrisThielemans left a comment

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

KrisThielemans Jan 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	ScatterSimulation::set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg)
	ScatterSimulation::set_template_proj_data_info(shared_ptr<const ProjDataInfo> arg)

	void set_template_proj_data_info(std::shared_ptr<const ProjDataInfo> arg);
	void set_template_proj_data_info(shared_ptr<const ProjDataInfo> arg);

Fix memory leak from repeated ProjDataInfo::clone() calls #1673

Are you sure you want to change the base?

Fix memory leak from repeated ProjDataInfo::clone() calls #1673

Conversation

NikEfth commented Dec 23, 2025

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

Contribution Notes

Uh oh!

NikEfth commented Dec 23, 2025

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants