From 870657969ba9eca6fa720c5a89aa2034c0bc952e Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Wed, 11 Oct 2017 15:43:29 -0700 Subject: [PATCH 1/3] Revert "mm-video-v4l2: venc: Use client allocated memory if available" This reverts commit 38641613a6eb7b761429fcdfa31218e8c63c1c56. Bug: 67670457 Bug: 62452543 Test: capture a video (cherry picked from commit 99560b95dc3b9d5b0f01285328275a0a1c619974) Change-Id: I4b6cc58d582c2aaf72c818008bca2870b02f4c3e --- msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h | 1 - msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp | 8 +------- msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp | 2 -- 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index 26ca1f1b..afe31ef9 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -703,7 +703,6 @@ class omx_video: public qc_omx_component uint64_t m_out_bm_count; uint64_t m_client_out_bm_count; - uint64_t m_client_in_bm_count; uint64_t m_inp_bm_count; uint64_t m_flags; uint64_t m_etb_count; diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index 746d025a..43b5f6b7 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -290,7 +290,6 @@ omx_video::omx_video(): allocate_native_handle(false), m_out_bm_count(0), m_client_out_bm_count(0), - m_client_in_bm_count(0), m_inp_bm_count(0), m_flags(0), m_etb_count(0), @@ -2625,7 +2624,6 @@ OMX_ERRORTYPE omx_video::use_input_buffer( *bufferHdr = (m_inp_mem_ptr + i); BITMASK_SET(&m_inp_bm_count,i); - BITMASK_SET(&m_client_in_bm_count,i); (*bufferHdr)->pBuffer = (OMX_U8 *)buffer; (*bufferHdr)->nSize = sizeof(OMX_BUFFERHEADERTYPE); @@ -3646,10 +3644,6 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, nPortIndex = buffer - (OMX_BUFFERHEADERTYPE*)m_out_mem_ptr; if(BITMASK_PRESENT(&m_client_out_bm_count, nPortIndex)) BITMASK_CLEAR(&m_client_out_bm_count,nPortIndex); - } else if (port == PORT_INDEX_IN) { - nPortIndex = buffer - (meta_mode_enable?meta_buffer_hdr:m_inp_mem_ptr); - if(BITMASK_PRESENT(&m_client_in_bm_count, nPortIndex)) - BITMASK_CLEAR(&m_client_in_bm_count,nPortIndex); } if (m_state == OMX_StateIdle && (BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) { @@ -4012,7 +4006,7 @@ OMX_ERRORTYPE omx_video::empty_this_buffer_proxy(OMX_IN OMX_HANDLETYPE hComp, auto_lock l(m_buf_lock); pmem_data_buf = (OMX_U8 *)m_pInput_pmem[nBufIndex].buffer; - if (pmem_data_buf && BITMASK_PRESENT(&m_client_in_bm_count, nBufIndex)) { + if (pmem_data_buf && BITMASK_PRESENT(&m_inp_bm_count, nBufIndex)) { memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset), buffer->nFilledLen); } diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp index 526ebb49..20213b30 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp @@ -2391,8 +2391,6 @@ OMX_ERRORTYPE omx_venc::component_deinit(OMX_IN OMX_HANDLETYPE hComp) for (i=0; i Date: Wed, 11 Oct 2017 15:45:03 -0700 Subject: [PATCH 2/3] Revert "mm-video-v4l2: venc: Avoid buffer access after free" This reverts commit d53750a9db5b622fa19423ab82f0ee3ab1e25cbb. Bug: 67670457 Bug: 36130225 Test: capture a video (cherry picked from commit 16df6cadaee3fc5b47fb0bc43214bfed658a87e7) Change-Id: I1c288e5398814cbeef302fcf7919e6efd86d2f89 --- .../vidc/venc/inc/omx_video_base.h | 1 - .../vidc/venc/src/omx_video_base.cpp | 17 +++++------------ .../vidc/venc/src/omx_video_encoder.cpp | 7 +------ 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index afe31ef9..13b5025a 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -702,7 +702,6 @@ class omx_video: public qc_omx_component bool allocate_native_handle; uint64_t m_out_bm_count; - uint64_t m_client_out_bm_count; uint64_t m_inp_bm_count; uint64_t m_flags; uint64_t m_etb_count; diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index 43b5f6b7..5d0f4458 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -289,7 +289,6 @@ omx_video::omx_video(): pending_output_buffers(0), allocate_native_handle(false), m_out_bm_count(0), - m_client_out_bm_count(0), m_inp_bm_count(0), m_flags(0), m_etb_count(0), @@ -2903,7 +2902,6 @@ OMX_ERRORTYPE omx_video::use_output_buffer( } BITMASK_SET(&m_out_bm_count,i); - BITMASK_SET(&m_client_out_bm_count,i); } else { DEBUG_PRINT_ERROR("ERROR: All o/p Buffers have been Used, invalid use_buf call for " "index = %u", i); @@ -2941,8 +2939,6 @@ OMX_ERRORTYPE omx_video::use_buffer( DEBUG_PRINT_ERROR("ERROR: Use Buffer in Invalid State"); return OMX_ErrorInvalidState; } - - auto_lock l(m_buf_lock); if (port == PORT_INDEX_IN) { auto_lock l(m_lock); eRet = use_input_buffer(hComp,bufferHdr,port,appData,bytes,buffer); @@ -2952,6 +2948,7 @@ OMX_ERRORTYPE omx_video::use_buffer( DEBUG_PRINT_ERROR("ERROR: Invalid Port Index received %d",(int)port); eRet = OMX_ErrorBadPortIndex; } + if (eRet == OMX_ErrorNone) { if (allocate_done()) { if (BITMASK_PRESENT(&m_flags,OMX_COMPONENT_IDLE_PENDING)) { @@ -3014,6 +3011,7 @@ OMX_ERRORTYPE omx_video::free_input_buffer(OMX_BUFFERHEADERTYPE *bufferHdr) } if (index < m_sInPortDef.nBufferCountActual && m_pInput_pmem) { + auto_lock l(m_lock); if (mUseProxyColorFormat) { if (m_opq_pmem_q.m_size) { @@ -3567,7 +3565,7 @@ OMX_ERRORTYPE omx_video::allocate_buffer(OMX_IN OMX_HANDLETYPE h DEBUG_PRINT_ERROR("ERROR: Allocate Buf in Invalid State"); return OMX_ErrorInvalidState; } - auto_lock l(m_buf_lock); + // What if the client calls again. if (port == PORT_INDEX_IN) { auto_lock l(m_lock); @@ -3639,12 +3637,7 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, unsigned int nPortIndex; DEBUG_PRINT_LOW("In for encoder free_buffer"); - auto_lock l(m_buf_lock); - if (port == PORT_INDEX_OUT) { //client called freebuffer, clearing client buffer bitmask right away to avoid use after free - nPortIndex = buffer - (OMX_BUFFERHEADERTYPE*)m_out_mem_ptr; - if(BITMASK_PRESENT(&m_client_out_bm_count, nPortIndex)) - BITMASK_CLEAR(&m_client_out_bm_count,nPortIndex); - } + if (m_state == OMX_StateIdle && (BITMASK_PRESENT(&m_flags ,OMX_COMPONENT_LOADING_PENDING))) { DEBUG_PRINT_LOW(" free buffer while Component in Loading pending"); @@ -4004,7 +3997,7 @@ OMX_ERRORTYPE omx_video::empty_this_buffer_proxy(OMX_IN OMX_HANDLETYPE hComp, { DEBUG_PRINT_LOW("Heap UseBuffer case, so memcpy the data"); - auto_lock l(m_buf_lock); + auto_lock l(m_lock); pmem_data_buf = (OMX_U8 *)m_pInput_pmem[nBufIndex].buffer; if (pmem_data_buf && BITMASK_PRESENT(&m_inp_bm_count, nBufIndex)) { memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset), diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp index 20213b30..b8ee0935 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp @@ -2361,15 +2361,11 @@ OMX_ERRORTYPE omx_venc::component_deinit(OMX_IN OMX_HANDLETYPE hComp) DEBUG_PRINT_ERROR("WARNING:Rxd DeInit,OMX not in LOADED state %d",\ m_state); } - - auto_lock l(m_buf_lock); if (m_out_mem_ptr) { DEBUG_PRINT_LOW("Freeing the Output Memory"); for (i=0; i< m_sOutPortDef.nBufferCountActual; i++ ) { if (BITMASK_PRESENT(&m_out_bm_count, i)) { BITMASK_CLEAR(&m_out_bm_count, i); - if (BITMASK_PRESENT(&m_client_out_bm_count, i)) - BITMASK_CLEAR(&m_client_out_bm_count, i); free_output_buffer (&m_out_mem_ptr[i]); } @@ -2729,8 +2725,7 @@ int omx_venc::async_message_process (void *context, void* message) omxhdr->nFlags = m_sVenc_msg->buf.flags; /*Use buffer case*/ - if (BITMASK_PRESENT(&(omx->m_client_out_bm_count), bufIndex) && - omx->output_use_buffer && !omx->m_use_output_pmem && !omx->is_secure_session()) { + if (omx->output_use_buffer && !omx->m_use_output_pmem && !omx->is_secure_session()) { DEBUG_PRINT_LOW("memcpy() for o/p Heap UseBuffer"); memcpy(omxhdr->pBuffer, (m_sVenc_msg->buf.ptrbuffer), From c47a7d64eac60f4285ff82209e1451c82851ef8b Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Wed, 11 Oct 2017 15:47:32 -0700 Subject: [PATCH 3/3] Revert "mm-video-v4l2: venc: Protect buffer from being freed while accessing" This reverts commit 2c15b5832ac2631b27b2ec20936b3161fd167939. Bug: 67670457 Bug: 36130225 Test: capture a video (cherry picked from commit b12448b86095f6c8a46f2bb095608f9e96d2bdb7) Change-Id: I7132d0d5e8f514c8c7377be99fcaa796c0741b9d --- msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h | 1 - msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp | 6 ------ .../mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp | 10 +--------- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h index 13b5025a..8643e3f0 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h +++ b/msm8998/mm-video-v4l2/vidc/venc/inc/omx_video_base.h @@ -690,7 +690,6 @@ class omx_video: public qc_omx_component omx_cmd_queue m_opq_meta_q; omx_cmd_queue m_opq_pmem_q; OMX_BUFFERHEADERTYPE meta_buffer_hdr[MAX_NUM_INPUT_BUFFERS]; - pthread_mutex_t m_buf_lock; bool input_flush_progress; bool output_flush_progress; diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp index 5d0f4458..c091162a 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp @@ -320,8 +320,6 @@ omx_video::omx_video(): property_get("ro.board.platform", platform_name, "0"); strlcpy(m_platform, platform_name, sizeof(m_platform)); #endif - - pthread_mutex_init(&m_buf_lock, NULL); } @@ -363,8 +361,6 @@ omx_video::~omx_video() sem_destroy(&m_cmd_lock); DEBUG_PRINT_HIGH("m_etb_count = %" PRIu64 ", m_fbd_count = %" PRIu64, m_etb_count, m_fbd_count); - - pthread_mutex_destroy(&m_buf_lock); DEBUG_PRINT_HIGH("omx_video: Destructor exit"); DEBUG_PRINT_HIGH("Exiting OMX Video Encoder ..."); } @@ -2751,7 +2747,6 @@ OMX_ERRORTYPE omx_video::use_output_buffer( return OMX_ErrorBadParameter; } - auto_lock l(m_buf_lock); if (!m_out_mem_ptr) { output_use_buffer = true; int nBufHdrSize = 0; @@ -3717,7 +3712,6 @@ OMX_ERRORTYPE omx_video::free_buffer(OMX_IN OMX_HANDLETYPE hComp, nPortIndex, (unsigned int)m_sOutPortDef.nBufferCountActual); if (nPortIndex < m_sOutPortDef.nBufferCountActual && BITMASK_PRESENT(&m_out_bm_count, nPortIndex)) { - auto_lock l(m_buf_lock); // Clear the bit associated with it. BITMASK_CLEAR(&m_out_bm_count,nPortIndex); m_sOutPortDef.bPopulated = OMX_FALSE; diff --git a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp index b8ee0935..f0468bf8 100644 --- a/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp +++ b/msm8998/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp @@ -2706,17 +2706,10 @@ int omx_venc::async_message_process (void *context, void* message) OMX_COMPONENT_GENERATE_EBD); break; case VEN_MSG_OUTPUT_BUFFER_DONE: - { omxhdr = (OMX_BUFFERHEADERTYPE*)m_sVenc_msg->buf.clientdata; - OMX_U32 bufIndex = (OMX_U32)(omxhdr - omx->m_out_mem_ptr); if ( (omxhdr != NULL) && - (bufIndex < omx->m_sOutPortDef.nBufferCountActual)) { - auto_lock l(omx->m_buf_lock); - if (BITMASK_ABSENT(&(omx->m_out_bm_count), bufIndex)) { - DEBUG_PRINT_ERROR("Recieved FBD for buffer that is already freed !"); - break; - } + ((OMX_U32)(omxhdr - omx->m_out_mem_ptr) < omx->m_sOutPortDef.nBufferCountActual)) { if (!omx->is_secure_session() && (m_sVenc_msg->buf.len <= omxhdr->nAllocLen)) { omxhdr->nFilledLen = m_sVenc_msg->buf.len; omxhdr->nOffset = m_sVenc_msg->buf.offset; @@ -2759,7 +2752,6 @@ int omx_venc::async_message_process (void *context, void* message) omx->post_event ((unsigned long)omxhdr,m_sVenc_msg->statuscode, OMX_COMPONENT_GENERATE_FBD); break; - } case VEN_MSG_NEED_OUTPUT_BUFFER: //TBD what action needs to be done here?? break;