Skip to content

Fix disjoint_pool_free() and proxy_free() #1157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix disjoint_pool_free() and proxy_free() #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/memory_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,10 @@ umf_result_t umfPoolGetTag(umf_memory_pool_handle_t hPool, void **tag) {
utils_mutex_unlock(&hPool->lock);
return UMF_RESULT_SUCCESS;
}

void *umfPoolGetPoolPriv(umf_memory_pool_handle_t hPool) {
if (hPool == NULL) {
return NULL;
}
return hPool->pool_priv;
}
2 changes: 2 additions & 0 deletions src/memory_pool_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ typedef struct umf_memory_pool_t {
void *tag;
} umf_memory_pool_t;

void *umfPoolGetPoolPriv(umf_memory_pool_handle_t hPool);

#ifdef __cplusplus
}
#endif
Expand Down
6 changes: 6 additions & 0 deletions src/pool/pool_disjoint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <umf/memory_provider.h>

#include "base_alloc_global.h"
#include "memory_pool_internal.h"
#include "pool_disjoint_internal.h"
#include "provider/provider_tracking.h"
#include "uthash/utlist.h"
Expand Down Expand Up @@ -819,6 +820,11 @@ umf_result_t disjoint_pool_free(void *pool, void *ptr) {
return ret;
}

if (pool != umfPoolGetPoolPriv(allocInfo.pool)) {
Copy link
Contributor

@vinser52 vinser52 Mar 5, 2025

Choose a reason for hiding this comment

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

Why do we need this check? It breaks encapsulation because now pool implementation calls umfPoolGetPoolPriv.

Copy link
Contributor Author

@ldorau ldorau Mar 5, 2025

Choose a reason for hiding this comment

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

This check prevents from freeing a pointer from a different pool.

Copy link
Contributor Author

@ldorau ldorau Mar 5, 2025

Choose a reason for hiding this comment

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

@vinser52 It is needed by #1143
See #1143 (comment)

Copy link
Contributor

@vinser52 vinser52 Mar 5, 2025

Choose a reason for hiding this comment

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

I am not sure we should expose UMF internals to the pool implementation.

Furthermore, this solution works only for UMF's pools. If someone implements their pool what he/she should do in that case?

Copy link
Contributor Author

@ldorau ldorau Mar 6, 2025

Choose a reason for hiding this comment

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

This PR was replaced with #1161 - please review this new PR instead.

LOG_ERR("pool mismatch");
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

size_t size = allocInfo.baseSize;
umf_memory_provider_handle_t provider = disjoint_pool->provider;
ret = umfMemoryProviderFree(provider, ptr, size);
Expand Down
7 changes: 7 additions & 0 deletions src/pool/pool_proxy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@
#include <assert.h>

#include "base_alloc_global.h"
#include "memory_pool_internal.h"
#include "provider/provider_tracking.h"
#include "utils_common.h"
#include "utils_log.h"

static __TLS umf_result_t TLS_last_allocation_error;

Expand Down Expand Up @@ -100,6 +102,11 @@ static umf_result_t proxy_free(void *pool, void *ptr) {
umf_alloc_info_t allocInfo = {NULL, 0, NULL};
umf_result_t umf_result = umfMemoryTrackerGetAllocInfo(ptr, &allocInfo);
if (umf_result == UMF_RESULT_SUCCESS) {
if (pool != umfPoolGetPoolPriv(allocInfo.pool)) {
LOG_ERR("pool mismatch");
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
}

size = allocInfo.baseSize;
}
}
Expand Down
Loading