From df8f0da70c85039b0e0986e023c9e7753472645d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Thu, 16 Feb 2017 17:51:29 +0100 Subject: [PATCH] param & param_shmem: enable locking We need to protect access to the param_values array. This is dynamically allocated and resized (utarray_reserve() calls realloc). If some thread was iterating the array while another was resizing the array, the first one would iterate on a freed array, thus accessing invalid memory. On NuttX this could lead to hardfaults in rare conditions. Unfortunately we need to initialize the semaphore on startup, by calling sem_init(). This adds a param_init() method called by every board/config that uses the params (at least I think I've found all of them) --- src/drivers/boards/aerocore/aerocore_init.c | 3 ++ src/drivers/boards/aerofc-v1/aerofc_init.c | 3 ++ src/drivers/boards/auav-x21/auav_init.c | 3 ++ src/drivers/boards/crazyflie/crazyflie_init.c | 3 ++ src/drivers/boards/esc35-v1/esc35_init.c | 3 ++ src/drivers/boards/mindpx-v2/mindpx2_init.c | 3 ++ .../px4-stm32f4discovery/px4discovery_init.c | 3 ++ .../boards/px4cannode-v1/px4cannode_init.c | 3 ++ src/drivers/boards/px4esc-v1/px4esc_init.c | 3 ++ src/drivers/boards/px4fmu-v1/px4fmu_init.c | 3 ++ src/drivers/boards/px4fmu-v2/px4fmu2_init.c | 3 ++ src/drivers/boards/px4fmu-v4/px4fmu_init.c | 3 ++ src/drivers/boards/px4fmu-v4pro/px4fmu_init.c | 3 ++ src/drivers/boards/px4fmu-v5/px4fmu_init.c | 3 ++ .../px4nucleoF767ZI-v1/px4nucleo_init.c | 3 ++ src/drivers/boards/s2740vc-v1/s2740vc_init.c | 3 ++ src/drivers/boards/tap-v1/tap_init.c | 3 ++ src/modules/systemlib/param/param.c | 35 +++++++++++++------ src/modules/systemlib/param/param.h | 5 +++ src/modules/systemlib/param/param_shmem.c | 32 +++++++++++------ .../posix/px4_layer/px4_posix_impl.cpp | 3 +- .../qurt/px4_layer/px4_qurt_impl.cpp | 3 +- 22 files changed, 106 insertions(+), 23 deletions(-) diff --git a/src/drivers/boards/aerocore/aerocore_init.c b/src/drivers/boards/aerocore/aerocore_init.c index 26421eaa2f..e5a6b08324 100644 --- a/src/drivers/boards/aerocore/aerocore_init.c +++ b/src/drivers/boards/aerocore/aerocore_init.c @@ -70,6 +70,7 @@ #include #include +#include #if defined(CONFIG_HAVE_CXX) && defined(CONFIG_HAVE_CXXINITIALIZE) #include @@ -194,6 +195,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ dma_alloc_init(); diff --git a/src/drivers/boards/aerofc-v1/aerofc_init.c b/src/drivers/boards/aerofc-v1/aerofc_init.c index 7dbeef59e0..ed23078c70 100644 --- a/src/drivers/boards/aerofc-v1/aerofc_init.c +++ b/src/drivers/boards/aerofc-v1/aerofc_init.c @@ -72,6 +72,7 @@ #include #include #include +#include # if defined(FLASH_BASED_PARAMS) # include @@ -173,6 +174,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure CPU load estimation */ #ifdef CONFIG_SCHED_INSTRUMENTATION cpuload_initialize_once(); diff --git a/src/drivers/boards/auav-x21/auav_init.c b/src/drivers/boards/auav-x21/auav_init.c index 299ed4687a..5068c2a050 100644 --- a/src/drivers/boards/auav-x21/auav_init.c +++ b/src/drivers/boards/auav-x21/auav_init.c @@ -79,6 +79,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -242,6 +243,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/crazyflie/crazyflie_init.c b/src/drivers/boards/crazyflie/crazyflie_init.c index 2724e6cc2a..6f60f57c32 100644 --- a/src/drivers/boards/crazyflie/crazyflie_init.c +++ b/src/drivers/boards/crazyflie/crazyflie_init.c @@ -71,6 +71,7 @@ #include #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -164,6 +165,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure CPU load estimation */ #ifdef CONFIG_SCHED_INSTRUMENTATION cpuload_initialize_once(); diff --git a/src/drivers/boards/esc35-v1/esc35_init.c b/src/drivers/boards/esc35-v1/esc35_init.c index 67b34e8071..a2f40ce359 100644 --- a/src/drivers/boards/esc35-v1/esc35_init.c +++ b/src/drivers/boards/esc35-v1/esc35_init.c @@ -69,6 +69,7 @@ #include #include +#include #if defined(CONFIG_HAVE_CXX) && defined(CONFIG_HAVE_CXXINITIALIZE) #include @@ -179,6 +180,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* set up the serial DMA polling */ static struct hrt_call serial_dma_call; struct timespec ts; diff --git a/src/drivers/boards/mindpx-v2/mindpx2_init.c b/src/drivers/boards/mindpx-v2/mindpx2_init.c index 167dc0fa42..b067a02dba 100644 --- a/src/drivers/boards/mindpx-v2/mindpx2_init.c +++ b/src/drivers/boards/mindpx-v2/mindpx2_init.c @@ -79,6 +79,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -228,6 +229,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/px4-stm32f4discovery/px4discovery_init.c b/src/drivers/boards/px4-stm32f4discovery/px4discovery_init.c index a202c100db..548f4506c2 100644 --- a/src/drivers/boards/px4-stm32f4discovery/px4discovery_init.c +++ b/src/drivers/boards/px4-stm32f4discovery/px4discovery_init.c @@ -72,6 +72,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -165,6 +166,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure CPU load estimation */ #ifdef CONFIG_SCHED_INSTRUMENTATION cpuload_initialize_once(); diff --git a/src/drivers/boards/px4cannode-v1/px4cannode_init.c b/src/drivers/boards/px4cannode-v1/px4cannode_init.c index ab8cfa8af8..b1de786ae9 100644 --- a/src/drivers/boards/px4cannode-v1/px4cannode_init.c +++ b/src/drivers/boards/px4cannode-v1/px4cannode_init.c @@ -69,6 +69,7 @@ #include #include +#include #if defined(CONFIG_HAVE_CXX) && defined(CONFIG_HAVE_CXXINITIALIZE) #include @@ -172,6 +173,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* set up the serial DMA polling */ static struct hrt_call serial_dma_call; struct timespec ts; diff --git a/src/drivers/boards/px4esc-v1/px4esc_init.c b/src/drivers/boards/px4esc-v1/px4esc_init.c index 756eaaae59..9642cfa952 100644 --- a/src/drivers/boards/px4esc-v1/px4esc_init.c +++ b/src/drivers/boards/px4esc-v1/px4esc_init.c @@ -69,6 +69,7 @@ #include #include +#include #if defined(CONFIG_HAVE_CXX) && defined(CONFIG_HAVE_CXXINITIALIZE) #include @@ -173,6 +174,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* set up the serial DMA polling */ static struct hrt_call serial_dma_call; struct timespec ts; diff --git a/src/drivers/boards/px4fmu-v1/px4fmu_init.c b/src/drivers/boards/px4fmu-v1/px4fmu_init.c index e1c2914744..4ea27657d2 100644 --- a/src/drivers/boards/px4fmu-v1/px4fmu_init.c +++ b/src/drivers/boards/px4fmu-v1/px4fmu_init.c @@ -69,6 +69,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -189,6 +190,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure CPU load estimation */ #ifdef CONFIG_SCHED_INSTRUMENTATION cpuload_initialize_once(); diff --git a/src/drivers/boards/px4fmu-v2/px4fmu2_init.c b/src/drivers/boards/px4fmu-v2/px4fmu2_init.c index c22b41c76c..5778334d03 100644 --- a/src/drivers/boards/px4fmu-v2/px4fmu2_init.c +++ b/src/drivers/boards/px4fmu-v2/px4fmu2_init.c @@ -79,6 +79,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -237,6 +238,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/px4fmu-v4/px4fmu_init.c b/src/drivers/boards/px4fmu-v4/px4fmu_init.c index 4b52b8b8c8..42826aa9f5 100644 --- a/src/drivers/boards/px4fmu-v4/px4fmu_init.c +++ b/src/drivers/boards/px4fmu-v4/px4fmu_init.c @@ -80,6 +80,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -250,6 +251,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/px4fmu-v4pro/px4fmu_init.c b/src/drivers/boards/px4fmu-v4pro/px4fmu_init.c index e73d387ec5..3c2a68d423 100644 --- a/src/drivers/boards/px4fmu-v4pro/px4fmu_init.c +++ b/src/drivers/boards/px4fmu-v4pro/px4fmu_init.c @@ -80,6 +80,7 @@ #include #include +#include /**************************************************************************** * Pre-Processor Definitions @@ -266,6 +267,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/px4fmu-v5/px4fmu_init.c b/src/drivers/boards/px4fmu-v5/px4fmu_init.c index 028fdb63cd..f2739d90dc 100644 --- a/src/drivers/boards/px4fmu-v5/px4fmu_init.c +++ b/src/drivers/boards/px4fmu-v5/px4fmu_init.c @@ -80,6 +80,7 @@ #include #include +#include #include "up_internal.h" /**************************************************************************** @@ -311,6 +312,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/px4nucleoF767ZI-v1/px4nucleo_init.c b/src/drivers/boards/px4nucleoF767ZI-v1/px4nucleo_init.c index 438057528d..396692b5d7 100644 --- a/src/drivers/boards/px4nucleoF767ZI-v1/px4nucleo_init.c +++ b/src/drivers/boards/px4nucleoF767ZI-v1/px4nucleo_init.c @@ -80,6 +80,7 @@ #include #include +#include #include "up_internal.h" /**************************************************************************** @@ -250,6 +251,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/drivers/boards/s2740vc-v1/s2740vc_init.c b/src/drivers/boards/s2740vc-v1/s2740vc_init.c index f432d1423b..bb9e32580e 100644 --- a/src/drivers/boards/s2740vc-v1/s2740vc_init.c +++ b/src/drivers/boards/s2740vc-v1/s2740vc_init.c @@ -69,6 +69,7 @@ #include #include +#include #if defined(CONFIG_HAVE_CXX) && defined(CONFIG_HAVE_CXXINITIALIZE) #include @@ -165,6 +166,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* set up the serial DMA polling */ static struct hrt_call serial_dma_call; struct timespec ts; diff --git a/src/drivers/boards/tap-v1/tap_init.c b/src/drivers/boards/tap-v1/tap_init.c index fcc7941da5..b77009e566 100644 --- a/src/drivers/boards/tap-v1/tap_init.c +++ b/src/drivers/boards/tap-v1/tap_init.c @@ -72,6 +72,7 @@ #include #include #include +#include # if defined(FLASH_BASED_PARAMS) # include @@ -200,6 +201,8 @@ __EXPORT int board_app_initialize(uintptr_t arg) /* configure the high-resolution time/callout interface */ hrt_init(); + param_init(); + /* configure the DMA allocator */ if (board_dma_alloc_init() < 0) { diff --git a/src/modules/systemlib/param/param.c b/src/modules/systemlib/param/param.c index 02b72a28e5..9f9d146025 100644 --- a/src/modules/systemlib/param/param.c +++ b/src/modules/systemlib/param/param.c @@ -54,7 +54,7 @@ #include #include #include -#include +#include #include #include @@ -117,7 +117,7 @@ struct param_wbuf_s { }; -uint8_t *param_changed_storage = 0; +uint8_t *param_changed_storage = NULL; int size_param_changed_storage_bytes = 0; const int bits_per_allocation_unit = (sizeof(*param_changed_storage) * 8); @@ -127,6 +127,7 @@ get_param_info_count(void) { /* Singleton creation of and array of bits to track changed values */ if (!param_changed_storage) { + /* Note that we have a (highly unlikely) race condition here: in the worst case the allocation is done twice */ size_param_changed_storage_bytes = (param_info_count / bits_per_allocation_unit) + 1; param_changed_storage = calloc(size_param_changed_storage_bytes, 1); @@ -156,18 +157,20 @@ static void param_set_used_internal(param_t param); static param_t param_find_internal(const char *name, bool notification); +static px4_sem_t param_sem; + /** lock the parameter store */ static void param_lock(void) { - //do {} while (px4_sem_wait(¶m_sem) != 0); + do {} while (px4_sem_wait(¶m_sem) != 0); } /** unlock the parameter store */ static void param_unlock(void) { - //px4_sem_post(¶m_sem); + px4_sem_post(¶m_sem); } /** assert that the parameter store is locked */ @@ -177,6 +180,12 @@ param_assert_locked(void) /* XXX */ } +void +param_init(void) +{ + px4_sem_init(¶m_sem, 0, 1); +} + /** * Test whether a param_t is value. * @@ -425,15 +434,22 @@ param_name(param_t param) bool param_value_is_default(param_t param) { - return param_find_changed(param) ? false : true; + struct param_wbuf_s *s; + param_lock(); + s = param_find_changed(param); + param_unlock(); + return s ? false : true; } bool param_value_unsaved(param_t param) { - static struct param_wbuf_s *s; + struct param_wbuf_s *s; + param_lock(); s = param_find_changed(param); - return (s && s->unsaved) ? true : false; + bool ret = s && s->unsaved; + param_unlock(); + return ret; } enum param_type_e @@ -719,8 +735,6 @@ param_reset_all(void) void param_reset_excludes(const char *excludes[], int num_excludes) { - param_lock(); - param_t param; for (param = 0; handle_in_range(param); param++) { @@ -743,8 +757,6 @@ param_reset_excludes(const char *excludes[], int num_excludes) } } - param_unlock(); - _param_notify_changes(false); } @@ -755,6 +767,7 @@ int param_set_default_file(const char *filename) { if (param_user_file != NULL) { + // we assume this is not in use by some other thread free(param_user_file); param_user_file = NULL; } diff --git a/src/modules/systemlib/param/param.h b/src/modules/systemlib/param/param.h index 5350a4f22e..630280d7a6 100644 --- a/src/modules/systemlib/param/param.h +++ b/src/modules/systemlib/param/param.h @@ -87,6 +87,11 @@ typedef uintptr_t param_t; */ #define PARAM_HASH ((uintptr_t)INT32_MAX) +/** + * Initialize the param backend. Call this on startup before calling any other methods. + */ +__EXPORT void param_init(void); + /** * Look up a parameter by name. * diff --git a/src/modules/systemlib/param/param_shmem.c b/src/modules/systemlib/param/param_shmem.c index ce2dbc6e5c..fcf46d699b 100644 --- a/src/modules/systemlib/param/param_shmem.c +++ b/src/modules/systemlib/param/param_shmem.c @@ -51,7 +51,7 @@ #include #include #include -#include +#include #include @@ -165,18 +165,20 @@ static void param_set_used_internal(param_t param); static param_t param_find_internal(const char *name, bool notification); +static px4_sem_t param_sem; + /** lock the parameter store */ static void param_lock(void) { - //do {} while (px4_sem_wait(¶m_sem) != 0); + do {} while (px4_sem_wait(¶m_sem) != 0); } /** unlock the parameter store */ static void param_unlock(void) { - //px4_sem_post(¶m_sem); + px4_sem_post(¶m_sem); } /** assert that the parameter store is locked */ @@ -186,6 +188,12 @@ param_assert_locked(void) /* TODO */ } +void +param_init(void) +{ + px4_sem_init(¶m_sem, 0, 1); +} + /** * Test whether a param_t is value. * @@ -431,15 +439,22 @@ param_name(param_t param) bool param_value_is_default(param_t param) { - return param_find_changed(param) ? false : true; + struct param_wbuf_s *s; + param_lock(); + s = param_find_changed(param); + param_unlock(); + return s ? false : true; } bool param_value_unsaved(param_t param) { - static struct param_wbuf_s *s; + struct param_wbuf_s *s; + param_lock(); s = param_find_changed(param); - return (s && s->unsaved) ? true : false; + bool ret = s && s->unsaved; + param_unlock(); + return ret; } enum param_type_e @@ -773,8 +788,6 @@ param_reset_all(void) void param_reset_excludes(const char *excludes[], int num_excludes) { - param_lock(); - param_t param; for (param = 0; handle_in_range(param); param++) { @@ -797,8 +810,6 @@ param_reset_excludes(const char *excludes[], int num_excludes) } } - param_unlock(); - _param_notify_changes(false); } @@ -813,6 +824,7 @@ int param_set_default_file(const char *filename) { if (param_user_file != NULL) { + // we assume this is not in use by some other thread free(param_user_file); param_user_file = NULL; } diff --git a/src/platforms/posix/px4_layer/px4_posix_impl.cpp b/src/platforms/posix/px4_layer/px4_posix_impl.cpp index 052ce1917b..90dc3630be 100644 --- a/src/platforms/posix/px4_layer/px4_posix_impl.cpp +++ b/src/platforms/posix/px4_layer/px4_posix_impl.cpp @@ -46,7 +46,7 @@ #include #include #include -#include "systemlib/param/param.h" +#include #include "hrt_work.h" #include #include "px4_time.h" @@ -76,6 +76,7 @@ void init_once() work_queues_init(); hrt_work_queue_init(); hrt_init(); + param_init(); #ifdef CONFIG_SHMEM PX4_DEBUG("Syncing params to shared memory\n"); diff --git a/src/platforms/qurt/px4_layer/px4_qurt_impl.cpp b/src/platforms/qurt/px4_layer/px4_qurt_impl.cpp index e956b9da94..12b4c2667e 100644 --- a/src/platforms/qurt/px4_layer/px4_qurt_impl.cpp +++ b/src/platforms/qurt/px4_layer/px4_qurt_impl.cpp @@ -48,7 +48,7 @@ #include #include #include -#include "systemlib/param/param.h" +#include #include "hrt_work.h" #include "px4_log.h" @@ -106,6 +106,7 @@ void init_once(void) work_queues_init(); hrt_work_queue_init(); hrt_init(); + param_init(); /* Shared memory param sync*/ init_params();