On Sat, 1 Aug 2026 15:34:49 GMT, Roman Marchenko <[email protected]> wrote:
>> modules/javafx.media/src/main/native/gstreamer/plugins/dshowwrapper/Allocator.cpp
>> line 92:
>>
>>> 90: if (pSample->m_pMappedGstBuffer != NULL)
>>> 91: {
>>> 92: gst_buffer_unmap(pSample->m_pMappedGstBuffer,
>>> &pSample->m_GstMapInfo);
>>
>> `m_pMappedGstBuffer` is a copy of the pointer that was used to initialize
>> `m_pGstBuffer`, but there is nothing that will ensure ownership of the
>> buffer. If there is any code path where the buffer is freed prior to
>> `CAllocator::ReleaseBuffer` being called -- and it looks like there might be
>> -- this could lead to a use after free.
>>
>> I'd like @sashamatveev to take a closer look.
>
> @kevinrushforth
>> If there is any code path where the buffer is freed prior to
>> CAllocator::ReleaseBuffer ... this could lead to a use after free.
>
> I would only like to point out that the proposed change is intended to
> _upmap_ the buffer properly, and repeats the original behavior that has been
> disrupted after disabling the macro.
>
> OTOH we could move unmapping closer to ownership change in
> `CSink::DoRenderSampleInternal()`.
I think we should fix it correctly. Original implementation was not right to
unmap memory on GstBuffer which might be released already.
My suggestion is to add functions below in CSample:
`HRESULT SetGstBuffer(GstBuffer *pGstBuffer);`
`GstBuffer *TakeGstBuffer();`
Add `m_bGstBufferMapped` flag as well to CSample.
Allocator will call `SetGstBuffer` and `SetGstBuffer` will map it.
`CSink::DoRenderSampleInternal()` will call `TakeGstBuffer` and this function
will unmap it and set `m_pGstBuffer` to NULL. Call `TakeGstBuffer` in
`CAllocator::ReleaseBuffer` to release it in case if it was not delivered.
`m_pGstBuffer` and `m_GstMapInfo` should be private.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2234#discussion_r3708508081
