mm-video-v4l2: venc: Avoid buffer access after free
client expects buffer to be free if free_buffer
is called, but if omx is in executing state free
buffer call will error out.
When async thread tries to copy data to client
buffer which is already freed,it leads to crash.
Added a bitmask to avoid copy to buffer after free.
Bug: 36130225
CRs-Fixed: 2106434
Author: Uma Mehta <umamehta@codeaurora.org>
Change-Id: Id439aac54ee64a65ea68b6431a9f5150255a6980
(cherry picked from commit b701fb182bdb32dbafc72406cc27480e12925d71)
diff --git a/msm8996/mm-video-v4l2/vidc/venc/inc/omx_video_base.h b/msm8996/mm-video-v4l2/vidc/venc/inc/omx_video_base.h
index 1a537e4..8fc78ce 100644
--- a/msm8996/mm-video-v4l2/vidc/venc/inc/omx_video_base.h
+++ b/msm8996/mm-video-v4l2/vidc/venc/inc/omx_video_base.h
@@ -659,6 +659,7 @@
int pending_output_buffers;
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/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp b/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp
index fa6b08f..abbcea3 100644
--- a/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp
+++ b/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_base.cpp
@@ -295,6 +295,7 @@
pending_input_buffers(0),
pending_output_buffers(0),
m_out_bm_count(0),
+ m_client_out_bm_count(0),
m_inp_bm_count(0),
m_flags(0),
m_etb_count(0),
@@ -2598,7 +2599,6 @@
return OMX_ErrorBadParameter;
}
- auto_lock l(m_buf_lock);
if (!m_out_mem_ptr) {
output_use_buffer = true;
int nBufHdrSize = 0;
@@ -2749,6 +2749,7 @@
}
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);
@@ -2786,6 +2787,8 @@
DEBUG_PRINT_ERROR("ERROR: Use Buffer in Invalid State");
return OMX_ErrorInvalidState;
}
+
+ auto_lock l(m_buf_lock);
if (port == PORT_INDEX_IN) {
eRet = use_input_buffer(hComp,bufferHdr,port,appData,bytes,buffer);
} else if (port == PORT_INDEX_OUT) {
@@ -2857,7 +2860,6 @@
}
if (index < m_sInPortDef.nBufferCountActual && m_pInput_pmem) {
- auto_lock l(m_lock);
if (m_pInput_pmem[index].fd > 0 && input_use_buffer == false) {
DEBUG_PRINT_LOW("FreeBuffer:: i/p AllocateBuffer case");
@@ -3369,7 +3371,7 @@
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) {
#ifdef _ANDROID_ICS_
@@ -3440,7 +3442,12 @@
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");
@@ -3517,7 +3524,6 @@
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;
@@ -3778,7 +3784,7 @@
{
DEBUG_PRINT_LOW("Heap UseBuffer case, so memcpy the data");
- auto_lock l(m_lock);
+ auto_lock l(m_buf_lock);
pmem_data_buf = (OMX_U8 *)m_pInput_pmem[nBufIndex].buffer;
if (pmem_data_buf) {
memcpy (pmem_data_buf, (buffer->pBuffer + buffer->nOffset),
diff --git a/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp b/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp
index b09400a..1d7c8ba 100644
--- a/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp
+++ b/msm8996/mm-video-v4l2/vidc/venc/src/omx_video_encoder.cpp
@@ -2098,11 +2098,15 @@
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]);
}
@@ -2449,7 +2453,8 @@
omxhdr->nFlags = m_sVenc_msg->buf.flags;
/*Use buffer case*/
- if (omx->output_use_buffer && !omx->m_use_output_pmem && !omx->is_secure_session()) {
+ if (BITMASK_PRESENT(&(omx->m_client_out_bm_count), bufIndex) &&
+ 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),
