FIX: Fix incorrect type castings and mismatched operators #2585

david-cortes-intel · 2025-07-04T09:47:57Z

Description

This PR fixes some issues in the logic of daal4py objects in which pointers were being casted incorrectly and operator delete was used incorrectly.

Note that this only avoids running into undefined behavior, but it does not fix memory leaks, which are left for a follow-up PR.

PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

I have reviewed my changes thoroughly before submitting this pull request.
Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
I have added a respective label(s) to PR if I have a permission for that.
I have resolved any merge conflicts that might occur with the base branch.

Testing

I have run it locally and tested the changes extensively.
All CI jobs are green or I have provided justification why they aren't.

david-cortes-intel · 2025-07-04T09:50:21Z

/intelci: run

codecov · 2025-07-04T10:17:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag	Coverage Δ
azure	`79.81% <ø> (ø)`
github	`72.51% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR updates pointer type signatures in Cython and refactors daal4py’s shared/raw pointer handling by replacing manual free functions and base classes with templated header-only functions.

Corrects Cython function signatures in gettree.pyx to use skl_tree_node* and double* instead of void*.
Refactors daal4py.h to introduce header-only templated functions (delete_daal_shared_ptr, set_sp_base, rawp_free_cap, set_rawp_base) and removes the old VSP/TVSP classes and free functions.
Cleans up daal4py.cpp by removing obsolete pointer free implementations.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/gettree.pyx	Updated extern declarations and removed manual casts for node/value pointers.
src/daal4py.h	Replaced custom VSP/TVSP and free functions with inline templates.
src/daal4py.cpp	Deleted deprecated free function definitions for raw and shared pointers.

Comments suppressed due to low confidence (1)

src/daal4py.h:282

Template functions defined in a header should be marked inline to avoid potential multiple-definition linker errors when included in multiple translation units. Consider adding inline to delete_daal_shared_ptr (and the other templates).

template <class T>

Copilot · 2025-07-04T11:39:54Z

src/daal4py.h

-    virtual ~TVSP() {};
-};
+    daal::services::SharedPtr<T> * shared_ptr_for_capsule = new daal::services::SharedPtr<T>();
+    *shared_ptr_for_capsule                               = std::move(sp);


Using std::move here transfers ownership out of the original shared_ptr, leaving the caller’s sp empty. To preserve expected semantics (a copy), assign sp directly instead of moving: *shared_ptr_for_capsule = sp;

Suggested change

*shared_ptr_for_capsule = std::move(sp);

*shared_ptr_for_capsule = sp;

I think this is a bad recommendation due to the nature of the shared pointer. We want the capsule to own it, and we don't want the ref count to increase. If that's the case, could you leave a comment to this effect?

Added a comment.

Should we have a move assignment operator defined in daal::services::SharedPtr in order to let this line work as expected?
Because for now daal::services::SharedPtr doesn't have a move assignment operator.

Changed to use initialization from another shared pointer instead of move. It has the downside that there's now a slight inefficiency in that a shared pointer is created and the old one destroyed immediately after the return of this function, but that shouldn't have any noticeable impact.

@Vika-F But what's the idea behind using a custom shared pointer class instead of just std::shared_ptr?

Yes, sorry. I misread the changes.
Regarding own implementation of shared_ptr, there are two reasons:

it is a legacy part. Originally DAAL was C++03 compatible.

there might be some issue with illegal instruction due to compiling std::shared_pointer multiple times with various ISA optimizations, but I am not sure that it still remains.

icfaust

I would like to have the impacts a bit more detailed in the description. To my mind there are two issues with respect to undefined behavior:

misuse of void in deleters, which is impacting return data from daal4py generally (all data is doing undefined behavior at deletion).
Misuse of array generation in gettree, which impacts only the daal4py ensemble algorithms (which are already been replaced in sklearnex by a onedal interface) through the use of void*.

icfaust · 2025-07-04T20:24:26Z

src/daal4py.h

-    virtual ~TVSP() {};
-};
+    daal::services::SharedPtr<T> * shared_ptr_for_capsule = new daal::services::SharedPtr<T>();
+    *shared_ptr_for_capsule                               = std::move(sp);


I think this is a bad recommendation due to the nature of the shared pointer. We want the capsule to own it, and we don't want the ref count to increase. If that's the case, could you leave a comment to this effect?

src/daal4py.h

icfaust · 2025-07-04T20:50:54Z

src/gettree.pyx

@@ -98,8 +98,8 @@ cdef class pyTreeState(object):
        self.node_count = treeState.node_count
        self.leaf_count = treeState.leaf_count
        self.class_count = treeState.class_count
-        self.node_ar = self._get_node_ndarray(<void*> treeState.node_ar, treeState.node_count)


I guess the cast to void* is now occurring in PyArray_SimpleNewFromData, rather than a function we define. I assume this is for UBSan to not complain? (I assume this is related to UBSan, if so it may be good to directly reference this in the description for posterity).

I haven't tried running ubsan on this, but either way, static-casting a void pointer to a type other than the one it was casted from is undefined behavior, even if that type is a parent class of the type from which the void pointer was casted.

See cppreference:
https://en.cppreference.com/w/cpp/language/pointer.html

Don't worry, I am onboard with the changes and know why this was not kosher. I guess I was trying to get at how you came about it (i.e. if you'd use a specific tool) such that future development would be aware of what has been done and what approaches have been taken in analyzing the codebase.

icfaust · 2025-07-04T21:02:14Z

Also merge main to fix the CI failures.

icfaust · 2025-07-04T21:03:40Z

private CI is acceptable.

icfaust · 2025-07-07T22:47:58Z

/intelci: run

icfaust

Sorry about the delay. Typical unrelated failures in private CI, thanks for the work.

avoid incorrect castings and undefined behavior

e38208f

david-cortes-intel requested a review from Vika-F July 4, 2025 09:47

david-cortes-intel added the bug Something isn't working label Jul 4, 2025

david-cortes-intel requested review from Alexsandruss and icfaust as code owners July 4, 2025 09:47

icfaust requested a review from Copilot July 4, 2025 11:38

Copilot AI reviewed Jul 4, 2025

View reviewed changes

icfaust reviewed Jul 4, 2025

View reviewed changes

david-cortes-intel added 3 commits July 6, 2025 23:39

add comment about move op on pointer

369f068

Merge branch 'main' into d4p_arr

846a155

avoid move operator

8b9cca1

david-cortes-intel requested a review from icfaust July 7, 2025 15:46

icfaust approved these changes Jul 11, 2025

View reviewed changes

	*shared_ptr_for_capsule = std::move(sp);
	*shared_ptr_for_capsule = sp;

FIX: Fix incorrect type castings and mismatched operators #2585

Are you sure you want to change the base?

FIX: Fix incorrect type castings and mismatched operators #2585

Uh oh!

Conversation

david-cortes-intel commented Jul 4, 2025

Description

Uh oh!

david-cortes-intel commented Jul 4, 2025

Uh oh!

codecov bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

icfaust Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-cortes-intel Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Vika-F Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

david-cortes-intel Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

Vika-F Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

icfaust left a comment

Choose a reason for hiding this comment

Uh oh!

icfaust Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

icfaust Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

david-cortes-intel Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

icfaust Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

icfaust commented Jul 4, 2025

Uh oh!

icfaust commented Jul 4, 2025

Uh oh!

icfaust commented Jul 7, 2025

Uh oh!

icfaust left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jul 4, 2025 •

edited

Loading

icfaust Jul 4, 2025 •

edited

Loading

icfaust Jul 4, 2025 •

edited

Loading