Skip to content

[RFC] Fixing dangling pointers caused by urCommandBufferRelease #1898

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 3 commits into from
Closed

[RFC] Fixing dangling pointers caused by urCommandBufferRelease #1898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c0e66a
Change urCommandBufferReleaseExp to avoid dangling pointers
ianayl Jul 24, 2024
7c4f45f
Updated function tables
ianayl Jul 25, 2024
548cb9f
Amend type signature of native_cpu urCommandBufferReleaseExp
ianayl Jul 25, 2024
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
2 changes: 1 addition & 1 deletion include/ur_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8314,7 +8314,7 @@ urCommandBufferRetainExp(
/// - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t hCommandBuffer ///< [in][release] Handle of the command-buffer object.
ur_exp_command_buffer_handle_t &hCommandBuffer ///< [in][release] Handle of the command-buffer object.
Copy link
Contributor

@kbenzie kbenzie Jul 26, 2024

Choose a reason for hiding this comment

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

This header is a C API, there can not be use of C++ references in it. Additionally, this header is generated from a spec description in yaml files which reside in scripts/core/*.yml.

);

///////////////////////////////////////////////////////////////////////////////
Expand Down
2 changes: 1 addition & 1 deletion include/ur_ddi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,7 @@ typedef ur_result_t(UR_APICALL *ur_pfnCommandBufferRetainExp_t)(
///////////////////////////////////////////////////////////////////////////////
/// @brief Function-pointer for urCommandBufferReleaseExp
typedef ur_result_t(UR_APICALL *ur_pfnCommandBufferReleaseExp_t)(
ur_exp_command_buffer_handle_t);
ur_exp_command_buffer_handle_t&);

///////////////////////////////////////////////////////////////////////////////
/// @brief Function-pointer for urCommandBufferFinalizeExp
Expand Down
5 changes: 3 additions & 2 deletions source/adapters/cuda/command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@

namespace {
ur_result_t
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t CommandBuffer) {
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t &CommandBuffer) {
if (CommandBuffer->decrementInternalReferenceCount() != 0) {
return UR_RESULT_SUCCESS;
}

delete CommandBuffer;
CommandBuffer = nullptr;
return UR_RESULT_SUCCESS;
}

Expand Down Expand Up @@ -320,7 +321,7 @@ urCommandBufferRetainExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
}

UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t &hCommandBuffer) {
if (hCommandBuffer->decrementExternalReferenceCount() == 0) {
// External ref count has reached zero, internal release of created
// commands.
Expand Down
5 changes: 3 additions & 2 deletions source/adapters/hip/command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@

namespace {
ur_result_t
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t CommandBuffer) {
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t &CommandBuffer) {
if (CommandBuffer->decrementInternalReferenceCount() != 0) {
return UR_RESULT_SUCCESS;
}

delete CommandBuffer;
CommandBuffer = nullptr;
return UR_RESULT_SUCCESS;
}

Expand Down Expand Up @@ -303,7 +304,7 @@ urCommandBufferRetainExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
}

UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t &hCommandBuffer) {
if (hCommandBuffer->decrementExternalReferenceCount() == 0) {
// External ref count has reached zero, internal release of created
// commands.
Expand Down
3 changes: 2 additions & 1 deletion source/adapters/level_zero/command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,11 +699,12 @@ urCommandBufferRetainExp(ur_exp_command_buffer_handle_t CommandBuffer) {
}

UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t CommandBuffer) {
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t &CommandBuffer) {
if (!CommandBuffer->RefCount.decrementAndTest())
return UR_RESULT_SUCCESS;

delete CommandBuffer;
CommandBuffer = nullptr;
Copy link
Contributor

@kbenzie kbenzie Jul 26, 2024

Choose a reason for hiding this comment

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

It is not the responsibility of the level zero adapter to zero out the handle pointer as part of the urCommandBufferReleaseExp entry point. The proper fix for this should be in the layer above which is calling this entry point.

return UR_RESULT_SUCCESS;
}

Expand Down
2 changes: 1 addition & 1 deletion source/adapters/mock/ur_mockddi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8198,7 +8198,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferRetainExp(
/// @brief Intercept function for urCommandBufferReleaseExp
__urdlllocal ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) try {
ur_result_t result = UR_RESULT_SUCCESS;

Expand Down
2 changes: 1 addition & 1 deletion source/adapters/native_cpu/command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ urCommandBufferRetainExp(ur_exp_command_buffer_handle_t) {
}

UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t) {
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t &) {
detail::ur::die("Experimental Command-buffer feature is not "
"implemented for the NativeCPU adapter.");
return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
Expand Down
5 changes: 3 additions & 2 deletions source/adapters/opencl/command_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@

namespace {
ur_result_t
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t CommandBuffer) {
commandBufferReleaseInternal(ur_exp_command_buffer_handle_t &CommandBuffer) {
if (CommandBuffer->decrementInternalReferenceCount() != 0) {
return UR_RESULT_SUCCESS;
}

delete CommandBuffer;
CommandBuffer = nullptr;
return UR_RESULT_SUCCESS;
}

Expand Down Expand Up @@ -109,7 +110,7 @@ urCommandBufferRetainExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
}

UR_APIEXPORT ur_result_t UR_APICALL
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t hCommandBuffer) {
urCommandBufferReleaseExp(ur_exp_command_buffer_handle_t &hCommandBuffer) {
if (hCommandBuffer->decrementExternalReferenceCount() == 0) {
// External ref count has reached zero, internal release of created
// commands.
Expand Down
2 changes: 1 addition & 1 deletion source/loader/layers/tracing/ur_trcddi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6367,7 +6367,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferRetainExp(
/// @brief Intercept function for urCommandBufferReleaseExp
__urdlllocal ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) {
auto pfnReleaseExp =
getContext()->urDdiTable.CommandBufferExp.pfnReleaseExp;
Expand Down
2 changes: 1 addition & 1 deletion source/loader/layers/validation/ur_valddi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7892,7 +7892,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferRetainExp(
/// @brief Intercept function for urCommandBufferReleaseExp
__urdlllocal ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) {
auto pfnReleaseExp =
getContext()->urDdiTable.CommandBufferExp.pfnReleaseExp;
Expand Down
2 changes: 1 addition & 1 deletion source/loader/ur_ldrddi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6996,7 +6996,7 @@ __urdlllocal ur_result_t UR_APICALL urCommandBufferRetainExp(
/// @brief Intercept function for urCommandBufferReleaseExp
__urdlllocal ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) {
ur_result_t result = UR_RESULT_SUCCESS;

Expand Down
2 changes: 1 addition & 1 deletion source/loader/ur_libapi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7418,7 +7418,7 @@ ur_result_t UR_APICALL urCommandBufferRetainExp(
/// - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) try {
auto pfnReleaseExp =
ur_lib::getContext()->urDdiTable.CommandBufferExp.pfnReleaseExp;
Expand Down
2 changes: 1 addition & 1 deletion source/ur_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6299,7 +6299,7 @@ ur_result_t UR_APICALL urCommandBufferRetainExp(
/// - ::UR_RESULT_ERROR_OUT_OF_HOST_MEMORY
ur_result_t UR_APICALL urCommandBufferReleaseExp(
ur_exp_command_buffer_handle_t
hCommandBuffer ///< [in][release] Handle of the command-buffer object.
&hCommandBuffer ///< [in][release] Handle of the command-buffer object.
) {
ur_result_t result = UR_RESULT_SUCCESS;
return result;
Expand Down