From 5564d5909a465fa57f6339993ffdc35273f471da Mon Sep 17 00:00:00 2001 From: Jeongmo Yang Date: Tue, 7 Feb 2023 20:51:38 +0900 Subject: [PATCH] Fix ASAN issue: heap-use-after-free - The callback for sound play could be called after handle is freed. - This patch allocates structures for the parameter of callback. It will not be freed if the callback is not called when handle is destroyed and prevents the heap-use-after-free issue as a result. [Version] 0.10.285 [Issue Type] ASAN issue Change-Id: Ie15ed724b47d9f4615d4a476420e02fbd203fb27 Signed-off-by: Jeongmo Yang --- packaging/libmm-camcorder.spec | 2 +- src/include/mm_camcorder_internal.h | 4 +- src/include/mm_camcorder_util.h | 5 +- src/mm_camcorder_internal.c | 32 ++-------- src/mm_camcorder_sound.c | 21 +++---- src/mm_camcorder_util.c | 93 ++++++++++++++++++++++++----- 6 files changed, 97 insertions(+), 60 deletions(-) diff --git a/packaging/libmm-camcorder.spec b/packaging/libmm-camcorder.spec index e9fcbf1..625951c 100755 --- a/packaging/libmm-camcorder.spec +++ b/packaging/libmm-camcorder.spec @@ -1,6 +1,6 @@ Name: libmm-camcorder Summary: Camera and recorder library -Version: 0.10.284 +Version: 0.10.285 Release: 0 Group: Multimedia/Libraries License: Apache-2.0 diff --git a/src/include/mm_camcorder_internal.h b/src/include/mm_camcorder_internal.h index 09d4d4e..b984024 100644 --- a/src/include/mm_camcorder_internal.h +++ b/src/include/mm_camcorder_internal.h @@ -851,8 +851,8 @@ typedef struct mmf_camcorder { /* gdbus */ GDBusConnection *gdbus_conn; /**< gdbus connection */ - _MMCamcorderGDbusCbInfo gdbus_info_sound; /**< Information for the gdbus cb of sound play */ - _MMCamcorderGDbusCbInfo gdbus_info_solo_sound; /**< Information for the gdbus cb of solo sound play */ + _MMCamcorderGDbusCbInfo *gdbus_info_sound; /**< Information for the gdbus cb of sound play */ + _MMCamcorderGDbusCbInfo *gdbus_info_solo_sound; /**< Information for the gdbus cb of solo sound play */ /* DPM(device policy manager) */ device_policy_manager_h dpm_handle; /**< DPM handle */ diff --git a/src/include/mm_camcorder_util.h b/src/include/mm_camcorder_util.h index b322f68..7e7eb49 100644 --- a/src/include/mm_camcorder_util.h +++ b/src/include/mm_camcorder_util.h @@ -213,7 +213,7 @@ typedef struct { int param; int is_playing; guint subscribe_id; - void *mm_handle; + gboolean free_in_cb; } _MMCamcorderGDbusCbInfo; /** @@ -349,6 +349,9 @@ int _mmcamcorder_get_camera_id(int device_type, int *camera_id); int _mmcamcorder_get_device_led_brightness(GDBusConnection *conn, int *brightness); /* sound play via dbus*/ +_MMCamcorderGDbusCbInfo *_mmcamcorder_gdbus_info_new(void); +void _mmcamcorder_gdbus_info_free(_MMCamcorderGDbusCbInfo *info); +void _mmcamcorder_gdbus_info_check_free(_MMCamcorderGDbusCbInfo *info); int _mmcamcorder_send_sound_play_message(GDBusConnection *conn, _MMCamcorderGDbusCbInfo *gdbus_info, const char *sample_name, const char *stream_role, const char *volume_gain, int sync_play); diff --git a/src/mm_camcorder_internal.c b/src/mm_camcorder_internal.c index 0069000..eb34cec 100644 --- a/src/mm_camcorder_internal.c +++ b/src/mm_camcorder_internal.c @@ -155,13 +155,8 @@ static gint __mmcamcorder_init_handle(mmf_camcorder_t **hcamcorder, int device_t new_handle->task_thread_state = _MMCAMCORDER_TASK_THREAD_STATE_NONE; if (device_type != MM_VIDEO_DEVICE_NONE) { - new_handle->gdbus_info_sound.mm_handle = new_handle; - g_mutex_init(&new_handle->gdbus_info_sound.sync_mutex); - g_cond_init(&new_handle->gdbus_info_sound.sync_cond); - - new_handle->gdbus_info_solo_sound.mm_handle = new_handle; - g_mutex_init(&new_handle->gdbus_info_solo_sound.sync_mutex); - g_cond_init(&new_handle->gdbus_info_solo_sound.sync_cond); + new_handle->gdbus_info_sound = _mmcamcorder_gdbus_info_new(); + new_handle->gdbus_info_solo_sound = _mmcamcorder_gdbus_info_new(); } /* create task thread */ @@ -264,10 +259,8 @@ static void __mmcamcorder_deinit_handle(mmf_camcorder_t *hcamcorder) g_mutex_clear(&hcamcorder->restart_preview_lock); if (hcamcorder->device_type != MM_VIDEO_DEVICE_NONE) { - g_mutex_clear(&hcamcorder->gdbus_info_sound.sync_mutex); - g_cond_clear(&hcamcorder->gdbus_info_sound.sync_cond); - g_mutex_clear(&hcamcorder->gdbus_info_solo_sound.sync_mutex); - g_cond_clear(&hcamcorder->gdbus_info_solo_sound.sync_cond); + _mmcamcorder_gdbus_info_check_free(hcamcorder->gdbus_info_sound); + _mmcamcorder_gdbus_info_check_free(hcamcorder->gdbus_info_solo_sound); } g_mutex_clear(&hcamcorder->task_thread_lock); @@ -1417,23 +1410,6 @@ int _mmcamcorder_stop(MMHandleType handle) _mmcamcorder_set_state(handle, MM_CAMCORDER_STATE_READY); - if (hcamcorder->type != MM_CAMCORDER_MODE_AUDIO) { - /* unsubscribe remained unsubscribed signal */ - g_mutex_lock(&hcamcorder->gdbus_info_sound.sync_mutex); - if (hcamcorder->gdbus_info_sound.subscribe_id > 0) { - MMCAM_LOG_WARNING("subscribe_id[%u] is remained. remove it.", hcamcorder->gdbus_info_sound.subscribe_id); - g_dbus_connection_signal_unsubscribe(hcamcorder->gdbus_conn, hcamcorder->gdbus_info_sound.subscribe_id); - } - g_mutex_unlock(&hcamcorder->gdbus_info_sound.sync_mutex); - - g_mutex_lock(&hcamcorder->gdbus_info_solo_sound.sync_mutex); - if (hcamcorder->gdbus_info_solo_sound.subscribe_id > 0) { - MMCAM_LOG_WARNING("subscribe_id[%u] is remained. remove it.", hcamcorder->gdbus_info_solo_sound.subscribe_id); - g_dbus_connection_signal_unsubscribe(hcamcorder->gdbus_conn, hcamcorder->gdbus_info_solo_sound.subscribe_id); - } - g_mutex_unlock(&hcamcorder->gdbus_info_solo_sound.sync_mutex); - } - _MMCAMCORDER_UNLOCK_CMD(hcamcorder); return MM_ERROR_NONE; diff --git a/src/mm_camcorder_sound.c b/src/mm_camcorder_sound.c index 4b2f3d5..96301ae 100644 --- a/src/mm_camcorder_sound.c +++ b/src/mm_camcorder_sound.c @@ -25,13 +25,10 @@ #include "mm_camcorder_internal.h" #include "mm_camcorder_sound.h" -/*--------------------------------------------------------------------------------------- -| GLOBAL VARIABLE DEFINITIONS for internal | ----------------------------------------------------------------------------------------*/ - /*--------------------------------------------------------------------------------------- | LOCAL VARIABLE DEFINITIONS for internal | ---------------------------------------------------------------------------------------*/ +#define _MMCAMCORDER_SOLO_PLAY_WAIT_TIMEOUT (G_TIME_SPAN_SECOND * 3) /*--------------------------------------------------------------------------------------- @@ -126,7 +123,7 @@ gboolean _mmcamcorder_sound_play(MMHandleType handle, const char *sample_name, g MMCAM_LOG_INFO("Play start - sample name [%s]", sample_name); _mmcamcorder_send_sound_play_message(hcamcorder->gdbus_conn, - &hcamcorder->gdbus_info_sound, sample_name, "system", volume_gain, sync_play); + hcamcorder->gdbus_info_sound, sample_name, "system", volume_gain, sync_play); g_mutex_unlock(&info->open_mutex); @@ -199,7 +196,7 @@ int _mmcamcorder_sound_solo_play(MMHandleType handle, const char *sample_name, g if (hcamcorder->shutter_sound_policy == VCONFKEY_CAMERA_SHUTTER_SOUND_POLICY_ON || hcamcorder->sub_context->info_image->sound_status) { _mmcamcorder_send_sound_play_message(hcamcorder->gdbus_conn, - &hcamcorder->gdbus_info_solo_sound, sample_name, "system", "shutter1", sync_play); + hcamcorder->gdbus_info_solo_sound, sample_name, "system", "shutter1", sync_play); } else { MMCAM_LOG_WARNING("skip shutter sound : sound policy %d, sound status %d", hcamcorder->shutter_sound_policy, hcamcorder->sub_context->info_image->sound_status); @@ -220,17 +217,17 @@ void _mmcamcorder_sound_solo_play_wait(MMHandleType handle) MMCAM_LOG_INFO("START"); /* check playing sound count */ - g_mutex_lock(&hcamcorder->gdbus_info_solo_sound.sync_mutex); + g_mutex_lock(&hcamcorder->gdbus_info_solo_sound->sync_mutex); - if (hcamcorder->gdbus_info_solo_sound.is_playing) { + if (hcamcorder->gdbus_info_solo_sound->is_playing) { gint64 end_time = 0; MMCAM_LOG_INFO("Wait for signal"); - end_time = g_get_monotonic_time() + G_TIME_SPAN_SECOND; + end_time = g_get_monotonic_time() + _MMCAMCORDER_SOLO_PLAY_WAIT_TIMEOUT; - if (g_cond_wait_until(&hcamcorder->gdbus_info_solo_sound.sync_cond, - &hcamcorder->gdbus_info_solo_sound.sync_mutex, end_time)) { + if (g_cond_wait_until(&hcamcorder->gdbus_info_solo_sound->sync_cond, + &hcamcorder->gdbus_info_solo_sound->sync_mutex, end_time)) { MMCAM_LOG_INFO("signal received."); } else { MMCAM_LOG_WARNING("capture sound play timeout."); @@ -239,7 +236,7 @@ void _mmcamcorder_sound_solo_play_wait(MMHandleType handle) MMCAM_LOG_WARNING("no playing sound"); } - g_mutex_unlock(&hcamcorder->gdbus_info_solo_sound.sync_mutex); + g_mutex_unlock(&hcamcorder->gdbus_info_solo_sound->sync_mutex); MMCAM_LOG_INFO("DONE"); diff --git a/src/mm_camcorder_util.c b/src/mm_camcorder_util.c index 42afe2d..3614ffe 100644 --- a/src/mm_camcorder_util.c +++ b/src/mm_camcorder_util.c @@ -166,37 +166,44 @@ static void __gdbus_stream_eos_cb(GDBusConnection *connection, { int played_idx = 0; gboolean stopped_by_user = FALSE; - _MMCamcorderGDbusCbInfo *gdbus_info = NULL; - mmf_camcorder_t *hcamcorder = NULL; - - MMCAM_LOG_WARNING("entered"); + gboolean do_free = FALSE; + _MMCamcorderGDbusCbInfo *info = NULL; if (!param || !user_data) { MMCAM_LOG_ERROR("invalid parameter %p %p", param, user_data); return; } - gdbus_info = (_MMCamcorderGDbusCbInfo *)user_data; - hcamcorder = (mmf_camcorder_t *)gdbus_info->mm_handle; + info = (_MMCamcorderGDbusCbInfo *)user_data; + + MMCAM_LOG_WARNING("entered[gdbus_info:%p]", info); g_variant_get(param, "(ib)", &played_idx, &stopped_by_user); - g_mutex_lock(&gdbus_info->sync_mutex); + g_mutex_lock(&info->sync_mutex); - MMCAM_LOG_WARNING("gdbus_info->param %d, played_idx %d, stopped_by_user %d, handle %p", - gdbus_info->param, played_idx, stopped_by_user, hcamcorder); + MMCAM_LOG_WARNING("gdbus_info->param %d, played_idx %d, stopped_by_user %d", + info->param, played_idx, stopped_by_user); - if (gdbus_info->param == played_idx) { - g_dbus_connection_signal_unsubscribe(connection, gdbus_info->subscribe_id); + if (info->param == played_idx) { + g_dbus_connection_signal_unsubscribe(connection, info->subscribe_id); - gdbus_info->is_playing = FALSE; - gdbus_info->subscribe_id = 0; - gdbus_info->param = 0; + info->is_playing = FALSE; + info->subscribe_id = 0; + info->param = 0; - g_cond_signal(&gdbus_info->sync_cond); + g_cond_signal(&info->sync_cond); } - g_mutex_unlock(&gdbus_info->sync_mutex); + if (info->free_in_cb) + do_free = TRUE; + + g_mutex_unlock(&info->sync_mutex); + + if (do_free) { + MMCAM_LOG_WARNING("free gdbus_info[%p]", info); + _mmcamcorder_gdbus_info_free(info); + } MMCAM_LOG_WARNING("done"); @@ -943,6 +950,60 @@ int _mmcamcorder_get_device_led_brightness(GDBusConnection *conn, int *brightnes } +_MMCamcorderGDbusCbInfo *_mmcamcorder_gdbus_info_new(void) +{ + _MMCamcorderGDbusCbInfo *new_info = g_new0(_MMCamcorderGDbusCbInfo, 1); + + g_mutex_init(&new_info->sync_mutex); + g_cond_init(&new_info->sync_cond); + + MMCAM_LOG_INFO("new gdbus_info[%p]", new_info); + + return new_info; +} + + +void _mmcamcorder_gdbus_info_free(_MMCamcorderGDbusCbInfo *info) +{ + if (!info) { + MMCAM_LOG_WARNING("NULL info"); + return; + } + + MMCAM_LOG_INFO("free gdbus_info[%p]", info); + + g_mutex_clear(&info->sync_mutex); + g_cond_clear(&info->sync_cond); + + g_free(info); +} + + +void _mmcamcorder_gdbus_info_check_free(_MMCamcorderGDbusCbInfo *info) +{ + gboolean do_free = FALSE; + + if (!info) { + MMCAM_LOG_WARNING("NULL info"); + return; + } + + g_mutex_lock(&info->sync_mutex); + + if (info->subscribe_id > 0) { + MMCAM_LOG_WARNING("gdbus_info[%p] will be freed in sound EOS cb", info); + info->free_in_cb = TRUE; + } else { + do_free = TRUE; + } + + g_mutex_unlock(&info->sync_mutex); + + if (do_free) + _mmcamcorder_gdbus_info_free(info); +} + + int _mmcamcorder_send_sound_play_message(GDBusConnection *conn, _MMCamcorderGDbusCbInfo *gdbus_info, const char *sample_name, const char *stream_role, const char *volume_gain, int sync_play) { -- 2.34.1