Fix incorrect delete in MXExecutorReshape exception handling #13376
Conversation
|
Taking a look ... |
|
So I see we potentially are deleting memory we shouldn't whenever an error is encountered. What I'm wondering is what would happen if an error is now encountered on line 569 for example? |
|
@mxnet-label-bot add [pr-awaiting-review] |
taliesinb
force-pushed
the
bugfix/fix_executor_reshape_delete
branch
from
73709fd
to
7fceae9
Compare
|
@KellenSunderland good point. I've moved the nullptr set up to just after API_BEGIN. |
src/c_api/c_api_executor.cc
Outdated
| @@ -560,6 +560,7 @@ int MXExecutorReshape(int partial_shaping, | |||
| ExecutorHandle *out) { | |||
| MXAPIThreadLocalEntry *ret = MXAPIThreadLocalStore::Get(); | |||
| API_BEGIN(); | |||
| *out = nullptr; // ensure we can know whether to free executor on early abort | |||
There was a problem hiding this comment.
Small linting issue:
src/c_api/c_api_executor.cc:563: At least two spaces is best between code and comments [whitespace/comments] [2]
Category 'whitespace' errors found: 1
There was a problem hiding this comment.
I've added the space, and am also creating a temporary pointer for the new executor as I see is done in other functions like MXExecutorGetOptimizedSymbol.
taliesinb
force-pushed
the
bugfix/fix_executor_reshape_delete
branch
from
7fceae9
to
0631d17
Compare
Delete the pointed-to handle on cleanup, not the location of the handle itself. Also don't delete it if we didn't set it in the first place.
taliesinb
force-pushed
the
bugfix/fix_executor_reshape_delete
branch
from
0631d17
to
b0ad315
Compare
|
@KellenSunderland Good to merge? |
|
@KellenSunderland I assume you've been busy with RE. Well done for the launch of all the exciting new products! Did you have a chance to look at this? Looks like the CI failure is spurious. |
|
Nah CI can be considered passed. PR-merge is deprecated :) |
|
@anirudh2290 @apeforest could you help take a look and merge if looks good? Thanks! |
|
@mxnet-label-bot add[C API, pr-awaiting-merge] |
marcoabreu
added
C API
pr-awaiting-merge
|
@taliesinb Sorry for the delay, looks good. Thanks for the fix! |
…13376) * Fix bad delete. Delete the pointed-to handle on cleanup, not the location of the handle itself. Also don't delete it if we didn't set it in the first place. * Remove unusued 'exec' var from MXExecutorBindEX.
…13376) * Fix bad delete. Delete the pointed-to handle on cleanup, not the location of the handle itself. Also don't delete it if we didn't set it in the first place. * Remove unusued 'exec' var from MXExecutorBindEX.
…13376) (apache#13824) * Fix bad delete. Delete the pointed-to handle on cleanup, not the location of the handle itself. Also don't delete it if we didn't set it in the first place. * Remove unusued 'exec' var from MXExecutorBindEX.
…13376) (apache#13824) * Fix bad delete. Delete the pointed-to handle on cleanup, not the location of the handle itself. Also don't delete it if we didn't set it in the first place. * Remove unusued 'exec' var from MXExecutorBindEX.
MXExecutorReshape makes a very obvious and simple error in its cleanup code, which is that it deletes the location of the out pointer rather the pointed-to executor. It also does this deletion even if the out pointer was not set, so it could delete a nullptr or other garbage value.