Full Text Bug Listing

Philippe Normand

Reported 2018-09-09 03:15:04 PDT

With ASan enabled, run-webkit-tests --gtk --debug http/tests/media/media-stream/getusermedia-with-canvas.html I think the issue is that the wrapper gst buffer created takes full ownership of BGRA data Vector, so the next call to updateSampleBuffer() might lead to reading an invalid pointer. ==6262==ERROR: AddressSanitizer: heap-use-after-free on address 0x7fa9f4e78800 at pc 0x7faa92ca14ae bp 0x7fa9f3f0c0a0 sp 0x7fa9f3f0b850 READ of size 2560 at 0x7fa9f4e78800 thread T34 (multiqueue0:src) #0 0x7faa92ca14ad (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f4ad) #1 0x7faa69b35f55 in gst_video_scaler_2d /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-scaler.c:1473 #2 0x7faa69b2b452 in convert_plane_hv_task /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-converter.c:5712 #3 0x7faa69b13c50 in gst_parallelized_task_runner_run /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-converter.c:298 #4 0x7faa69b2b8df in convert_plane_hv /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-converter.c:5776 #5 0x7faa69b2b94c in convert_scale_planes /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-converter.c:5789 #6 0x7faa69b1b4b6 in gst_video_converter_frame /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/video-converter.c:2646 #7 0x7faa17aebe4c in gst_video_convert_transform_frame /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst/videoconvert/gstvideoconvert.c:714 #8 0x7faa69b37f9c in gst_video_filter_transform /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst-libs/gst/video/gstvideofilter.c:272 #9 0x7faa69e7b8c7 in default_generate_output /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/libs/gst/base/gstbasetransform.c:2132 #10 0x7faa69e7bf46 in gst_base_transform_chain /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/libs/gst/base/gstbasetransform.c:2285 #11 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #12 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #13 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #14 0x7faa69d31dea in gst_proxy_pad_chain_default /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstghostpad.c:127 #15 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #16 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #17 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #18 0x7faa1704cb5a in gst_stream_synchronizer_sink_chain /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gst-plugins-base-1.14.2/gst/playback/gststreamsynchronizer.c:711 #19 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #20 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #21 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #22 0x7faa69d31dea in gst_proxy_pad_chain_default /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstghostpad.c:127 #23 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #24 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #25 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #26 0x7faa17a73723 in gst_concat_sink_chain /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/plugins/elements/gstconcat.c:454 #27 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #28 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #29 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #30 0x7faa69d31dea in gst_proxy_pad_chain_default /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstghostpad.c:127 #31 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #32 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #33 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #34 0x7faa69d31dea in gst_proxy_pad_chain_default /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstghostpad.c:127 #35 0x7faa69d50b49 in gst_pad_chain_data_unchecked /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4320 #36 0x7faa69d517a6 in gst_pad_push_data /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4576 #37 0x7faa69d51f0d in gst_pad_push /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gstpad.c:4695 #38 0x7faa17a980a7 in gst_single_queue_push_one /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/plugins/elements/gstmultiqueue.c:1643 #39 0x7faa17a99acd in gst_multi_queue_loop /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/plugins/elements/gstmultiqueue.c:1963 #40 0x7faa69d8cf47 in gst_task_func /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gsttask.c:332 #41 0x7faa69d8e10f in default_func /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.2/gst/gsttaskpool.c:69 #42 0x7faa68e36932 in g_thread_pool_thread_proxy /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthreadpool.c:307 #43 0x7faa68e35fd4 in g_thread_proxy /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthread.c:784 #44 0x7faa92c48f29 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7f29) #45 0x7faa6726cede in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xf7ede) 0x7fa9f4e78800 is located 0 bytes inside of 1228800-byte region [0x7fa9f4e78800,0x7fa9f4fa4800) freed by thread T0 here: #0 0x7faa92d4ab50 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe8b50) #1 0x7faa71c5d9f5 in bmalloc::DebugHeap::free(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57e89f5) #2 0x7faa71c5cf7d in bmalloc::Deallocator::deallocateSlowCase(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57e7f7d) #3 0x7faa84474552 in bmalloc::Deallocator::deallocate(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xfbb2552) #4 0x7faa84474700 in bmalloc::Cache::deallocate(bmalloc::HeapKind, void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xfbb2700) #5 0x7faa844747be in bmalloc::api::free(void*, bmalloc::HeapKind) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xfbb27be) #6 0x7faa71b42f88 in WTF::fastFree(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x56cdf88) #7 0x7faa8098a16b in WTF::FastMalloc::free(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xc0c816b) #8 0x7faa81d3fefb in WTF::MallocPtr<unsigned char, WTF::FastMalloc>::~MallocPtr() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xd47defb) #9 0x7faa874d81a8 in WebCore::WrappedMockRealtimeVideoSource::updateSampleBuffer() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x12c161a8) #10 0x7faa85ecfa2c in WebCore::MockRealtimeVideoSource::generateFrame() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x1160da2c) #11 0x7faa85ee6b94 in WTF::RunLoop::Timer<WebCore::MockRealtimeVideoSource>::fired() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x11624b94) #12 0x7faa71c47349 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::operator()(void*) const (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d2349) #13 0x7faa71c473d4 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d23d4) #14 0x7faa71c462d5 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::operator()(_GSource*, int (*)(void*), void*) const (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d12d5) #15 0x7faa71c46305 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d1305) #16 0x7faa68e0f8d7 in g_main_dispatch /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148 #17 0x7faa68e0f8d7 in g_main_context_dispatch /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813 previously allocated by thread T0 here: #0 0x7faa92d4aed0 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe8ed0) #1 0x7faa71c5d777 in bmalloc::DebugHeap::malloc(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57e8777) #2 0x7faa71c58b16 in bmalloc::Allocator::allocateSlowCase(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57e3b16) #3 0x7faa71b43c51 in bmalloc::Allocator::allocate(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x56cec51) #4 0x7faa71b43d8c in bmalloc::Cache::allocate(bmalloc::HeapKind, unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x56ced8c) #5 0x7faa71b44025 in bmalloc::api::malloc(unsigned long, bmalloc::HeapKind) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x56cf025) #6 0x7faa71b42b0c in WTF::fastMalloc(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x56cdb0c) #7 0x7faa8031faef in WTF::VectorBufferBase<unsigned char>::allocateBuffer(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xba5daef) #8 0x7faa80325a0b in WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::reserveCapacity(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xba63a0b) #9 0x7faa8031fd2b in WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::expandCapacity(unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xba5dd2b) #10 0x7faa80a4e2a0 in unsigned char const* WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::expandCapacity<unsigned char const>(unsigned long, unsigned char const*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xc18c2a0) #11 0x7faa80a4784c in void WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>::append<unsigned char>(unsigned char const*, unsigned long) (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0xc18584c) #12 0x7faa85cfce7a in WebCore::ImageBuffer::toBGRAData() const (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x1143ae7a) #13 0x7faa874d7ec3 in WebCore::WrappedMockRealtimeVideoSource::updateSampleBuffer() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x12c15ec3) #14 0x7faa85ecfa2c in WebCore::MockRealtimeVideoSource::generateFrame() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x1160da2c) #15 0x7faa85ee6b94 in WTF::RunLoop::Timer<WebCore::MockRealtimeVideoSource>::fired() (/home/phil/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37+0x11624b94) #16 0x7faa71c47349 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::operator()(void*) const (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d2349) #17 0x7faa71c473d4 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d23d4) #18 0x7faa71c462d5 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::operator()(_GSource*, int (*)(void*), void*) const (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d12d5) #19 0x7faa71c46305 in WTF::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) (/home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18+0x57d1305) #20 0x7faa68e0f8d7 in g_main_dispatch /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148 #21 0x7faa68e0f8d7 in g_main_context_dispatch /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813 Thread T34 (multiqueue0:src) created by T32 (appsrc1:src) here: #0 0x7faa92cabef0 in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x49ef0) #1 0x7faa68e533bf in g_system_thread_new /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthread-posix.c:1170 Thread T32 (appsrc1:src) created by T0 here: #0 0x7faa92cabef0 in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x49ef0) #1 0x7faa68e533bf in g_system_thread_new /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthread-posix.c:1170 SUMMARY: AddressSanitizer: heap-use-after-free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f4ad) Shadow bytes around the buggy address: 0x0ff5be9c70b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0ff5be9c70c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0ff5be9c70d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0ff5be9c70e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0ff5be9c70f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0ff5be9c7100:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0ff5be9c7110: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0ff5be9c7120: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0ff5be9c7130: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0ff5be9c7140: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0ff5be9c7150: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==6262==ABORTING

Philippe Normand

Comment 1 2018-09-09 03:43:03 PDT

Created attachment 349285 [details] Patch

EWS Watchlist

Comment 2 2018-09-09 03:46:32 PDT

Attachment 349285 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:3: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Thibault Saunier

Comment 3 2018-09-09 05:51:30 PDT

Comment on attachment 349285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349285&action=review If we don't have choice but memcpy, ok, I would rather avoid it though. > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:-53 > - auto gstsample = gst_sample_new(gst_buffer_new_wrapped(static_cast<guint8*>(data.releaseBuffer().get()), size), I think data.releaseBuffer().get() was giving us ownership of the data, am I wrong? I know it is for testing only but this introduces a big memcpy that we should avoid fmpov.

Philippe Normand

Comment 4 2018-09-09 06:36:37 PDT

(In reply to Thibault Saunier from comment #3) > Comment on attachment 349285 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349285&action=review > > If we don't have choice but memcpy, ok, I would rather avoid it though. > > > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:-53 > > - auto gstsample = gst_sample_new(gst_buffer_new_wrapped(static_cast<guint8*>(data.releaseBuffer().get()), size), > > I think data.releaseBuffer().get() was giving us ownership of the data, am I > wrong? I know it is for testing only but this introduces a big memcpy that > we should avoid fmpov. Hum yeah it should be possible to avoid the memcpy... It seems the issue is actually related with the MallocPtr returned by releaseBuffer(). I'll try another approach with gst_buffer_wrapped_full()...

Philippe Normand

Comment 5 2018-09-09 07:06:47 PDT

Right now I don't see how to avoid the memcpy because I don't see how to avoid the MallocPtr destruction, that class has no refcount management... The problem is that the MallocPtr is destroyed when the scope ends, leaving the gst sample with an invalid pointer.

Thibault Saunier

Comment 6 2018-09-09 07:19:42 PDT

(In reply to Philippe Normand from comment #5) > Right now I don't see how to avoid the memcpy because I don't see how to > avoid the MallocPtr destruction, that class has no refcount management... > > The problem is that the MallocPtr is destroyed when the scope ends, leaving > the gst sample with an invalid pointer. OK, then if there is no way to give ownership of MallocPtr to the GstBuffer, let's memcopy. Informal r+ for me.

Xabier Rodríguez Calvar

Comment 7 2018-09-10 01:45:28 PDT

Comment on attachment 349285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349285&action=review >>> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:-53 >>> - auto gstsample = gst_sample_new(gst_buffer_new_wrapped(static_cast<guint8*>(data.releaseBuffer().get()), size), >> >> I think data.releaseBuffer().get() was giving us ownership of the data, am I wrong? I know it is for testing only but this introduces a big memcpy that we should avoid fmpov. > > Hum yeah it should be possible to avoid the memcpy... It seems the issue is actually related with the MallocPtr returned by releaseBuffer(). I'll try another approach with gst_buffer_wrapped_full()... From what understand there, the problem of this line is data.releaseBuffer() returns a MallocPtr and get() gets that pointer that is passed to gst_buffer_new_wrapped. The problem happens when that MallocPtr goes out of scope just after running the get() so that pointer we pass with [transfer full] disappears with ~MallocPtr. I think what we want here is to do data.releaseBuffer().leakPtr() which will "leak" the pointer directly into the gst_buffer_new_wrapper [transfer full]. Am I missing anything here?

Philippe Normand

Comment 8 2018-09-10 02:19:42 PDT

Created attachment 349303 [details] Patch

EWS Watchlist

Comment 9 2018-09-10 02:22:12 PDT

Attachment 349303 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:3: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Xabier Rodríguez Calvar

Comment 10 2018-09-10 03:29:37 PDT

Comment on attachment 349303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349303&action=review > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:58 > + auto gstSample = adoptGRef(gst_sample_new(gst_buffer_new_wrapped(data.releaseBuffer().leakPtr(), > + size), caps.get(), nullptr, nullptr)); I think we're leaking the buffer here. gst_buffer_new_wrapped is [transfer full] and gst_sample_new receives [transfer none].

Xabier Rodríguez Calvar

Comment 11 2018-09-10 03:30:14 PDT

Comment on attachment 349303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349303&action=review > Source/WebCore/ChangeLog:3 > + [GStreamer] use-after-free in MockVideoCaptureSource And please, honor the style checker here ;)

Philippe Normand

Comment 12 2018-09-10 03:40:48 PDT

(In reply to Xabier Rodríguez Calvar from comment #11) > Comment on attachment 349303 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349303&action=review > > > Source/WebCore/ChangeLog:3 > > + [GStreamer] use-after-free in MockVideoCaptureSource > > And please, honor the style checker here ;) For this specific issue I see no reason to, because: - This issue is specific to the test infrastructure, mock sources are not exposed - The whole mediastream feature isn't shipped yet beyond developer builds.

Philippe Normand

Comment 13 2018-09-10 09:54:48 PDT

Created attachment 349313 [details] Patch

Philippe Normand

Comment 14 2018-09-10 09:56:27 PDT

(In reply to Xabier Rodríguez Calvar from comment #7) > Comment on attachment 349285 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349285&action=review > > >>> Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:-53 > >>> - auto gstsample = gst_sample_new(gst_buffer_new_wrapped(static_cast<guint8*>(data.releaseBuffer().get()), size), > >> > >> I think data.releaseBuffer().get() was giving us ownership of the data, am I wrong? I know it is for testing only but this introduces a big memcpy that we should avoid fmpov. > > > > Hum yeah it should be possible to avoid the memcpy... It seems the issue is actually related with the MallocPtr returned by releaseBuffer(). I'll try another approach with gst_buffer_wrapped_full()... > > From what understand there, the problem of this line is data.releaseBuffer() > returns a MallocPtr and get() gets that pointer that is passed to > gst_buffer_new_wrapped. The problem happens when that MallocPtr goes out of > scope just after running the get() so that pointer we pass with [transfer > full] disappears with ~MallocPtr. I think what we want here is to do > data.releaseBuffer().leakPtr() which will "leak" the pointer directly into > the gst_buffer_new_wrapper [transfer full]. > > Am I missing anything here? This looks good in theory but doesn't work in practice. I don't know why and don't plan to debug this further. I think that we can live with a memcpy here for this code specific to the testing infrastructure.

EWS Watchlist

Comment 15 2018-09-10 09:56:38 PDT

Attachment 349313 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:3: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Xabier Rodríguez Calvar

Comment 16 2018-09-10 22:45:06 PDT

Comment on attachment 349313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349313&action=review > Source/WebCore/platform/mediastream/gstreamer/MockGStreamerVideoCaptureSource.cpp:57 > + auto buffer = adoptGRef(gst_buffer_new_wrapped(g_memdup(data.releaseBuffer().leakPtr(), size), size)); If you do a g_memdup you shouldn't leakPtr() but get().

Philippe Normand

Comment 17 2018-09-11 01:20:37 PDT

Committed r235890: <https://trac.webkit.org/changeset/235890>

Radar WebKit Bug Importer

Comment 18 2018-09-11 01:21:27 PDT

<rdar://problem/44334490>

Attachments
Patch (3.08 KB, patch) 2018-09-09 03:43 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (3.05 KB, patch) 2018-09-10 02:19 PDT, Philippe Normand	no flags	Details Formatted Diff Diff
Patch (3.09 KB, patch) 2018-09-10 09:54 PDT, Philippe Normand	calvaris: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Bug 189462