Skip to content

Conversation

@ldorau
Copy link
Contributor

@ldorau ldorau commented Mar 6, 2025

Description

Passing an incorrect pool handle to umfPoolFree() is an Undefined Behavior.

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly

Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>

umf_result_t umfPoolFree(umf_memory_pool_handle_t hPool, void *ptr) {
UMF_CHECK((hPool != NULL), UMF_RESULT_ERROR_INVALID_ARGUMENT);
umf_memory_pool_handle_t poolTracker = umfMemoryTrackerGetPool(ptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

with this call, we would search the tracker 2 times during the free?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK in case of Proxy Pool and Disjoint Pool - yes

Copy link
Contributor

Choose a reason for hiding this comment

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

so this could significantly hurt the performance. Could you run the benchmarks and ask @lplewa for interpretation of results?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this problem specific to the disjoint and proxy pools? Does the issue exist with the scalable or with the jemalloc pools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is theoretically possible for every pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @bratpiorka mentioned it might impact performance.
so it is obvious that if an incorrect pool handle is passed by the client it is an ill-formed client's code. The question is do we want to catch such issues (and probably pay for the performance impact) or we should just say it is "undefined behavior" if an incorrect pool handle is passed as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vote for UB in such case.

@ldorau
Copy link
Contributor Author

ldorau commented Mar 7, 2025

Passing an incorrect pool handle to umfPoolFree() is an Undefined Behavior.

@ldorau ldorau closed this Mar 7, 2025
@lukaszstolarczuk
Copy link
Contributor

lukaszstolarczuk commented Mar 7, 2025

Passing an incorrect pool handle to umfPoolFree() is an Undefined Behavior.

perhaps should we update the docs?

@ldorau ldorau deleted the Do_not_free_a_pointer_from_a_different_pool branch March 13, 2025 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants