Fix new-delete type mismatch in MSDK dispatcher#196
Open
NathanGray-ChurchillNavigation wants to merge 1 commit into
Open
Fix new-delete type mismatch in MSDK dispatcher#196NathanGray-ChurchillNavigation wants to merge 1 commit into
NathanGray-ChurchillNavigation wants to merge 1 commit into
Conversation
MFXInitEx allocates session handles as MFX_DISP_HANDLE_EX, but MFXClose deletes them through a MFX_DISP_HANDLE pointer. The base class destructor is not virtual, so this is undefined behavior and is reported by AddressSanitizer as new-delete-type-mismatch (size of the allocated type: 1072 bytes; size of the deallocated type: 1048 bytes), aborting any ASan-instrumented process that opens and closes a session through the dispatcher. Making ~MFX_DISP_HANDLE virtual is not an option: the passthrough functions cast the session pointer directly to struct _mfxSession* and rely on the base subobject living at offset zero, and the header documents that the handle layout must stay compatible. Adding a vtable pointer would shift the base subobject. Instead, allocate MFX_DISP_HANDLE_EX at every handle creation site (it only extends the base with POD members, so this is safe for the two sites that previously allocated the base class) and spell the matching type at every delete of a base-typed pointer. Allocation and deallocation types now agree on all paths, including handles reaching MFXClose from MFXInitEx, MFXInitEx2, and MFXCloneSession. Signed-off-by: Nathan Gray <nathan.gray@shotover.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
MFXInitExallocates dispatcher session handles asMFX_DISP_HANDLE_EX, butMFXClosereleases them withdelete pHandlethrough aMFX_DISP_HANDLE *. Since~MFX_DISP_HANDLEis not virtual, deleting the derived object through the base pointer is undefined behavior ([expr.delete]/3). In practice this aborts any AddressSanitizer-instrumented process that opens and closes a session through the dispatcher (MSVC/fsanitize=address, similarly Clang/GCC ASan):We hit this in our application's ASan CI the first time it exercised
MFXInitEx/MFXClosethrough an instrumented dispatcher.Solution
Making
~MFX_DISP_HANDLEvirtual is not viable: the passthrough functions cast the session pointer directly tostruct _mfxSession *and read members assuming the base subobject is at offset zero (e.g.MFXMemory_GetSurfaceForVPP), andmfx_dispatcher.hdocuments that the handle layout must remain compatible. Adding a vtable pointer would shift the base subobject.Instead, this change makes the allocation and deallocation types agree on every path:
MFX_DISP_HANDLE_EXat the two remaining creation sites that used the base class (MFXInitEx2-style DLL-name init andAllocateCloneHandle).MFX_DISP_HANDLE_EXonly extends the base with POD members (mediaAdapterType+reserved), so this is behavior-neutral for those paths.deleteof a base-typed pointer (MFXClose, the DLL-name init error path, and the two clone error paths).MFXInitEx's own local/HandleVectorsites were already correctly typed asMFX_DISP_HANDLE_EX *.No layout, API, or behavior changes; 6 lines total.
How Tested
libvpl.dll.MFXInitEx+MFXQuery/MFXVideoENCODE_Query+MFXClosecycles (via our capability-probing code, executed per encoder/decoder node) now run to completion; unpatched they abort with the report above on the firstMFXClose.git clang-formatreports no diff for this change).