From db0d3c5e89bbbecfd446197b082ac8af872c43d9 Mon Sep 17 00:00:00 2001
From: Andrew Tridgell <andrew@tridgell.net>
Date: Sun, 19 Apr 2020 09:24:54 +1000
Subject: [PATCH] AP_Param: fixed race in param count handling

---
 libraries/AP_Param/AP_Param.cpp | 32 ++++++++++++++++++++++++++------
 libraries/AP_Param/AP_Param.h   |  5 ++++-
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/libraries/AP_Param/AP_Param.cpp b/libraries/AP_Param/AP_Param.cpp
index 01e809b4be..c5af9106b4 100644
--- a/libraries/AP_Param/AP_Param.cpp
+++ b/libraries/AP_Param/AP_Param.cpp
@@ -73,6 +73,9 @@ uint16_t AP_Param::_num_vars;
 
 // cached parameter count
 uint16_t AP_Param::_parameter_count;
+uint16_t AP_Param::_count_marker;
+uint16_t AP_Param::_count_marker_done;
+HAL_Semaphore AP_Param::_count_sem;
 
 // storage and naming information about all types that can be saved
 const AP_Param::Info *AP_Param::_var_info;
@@ -2325,19 +2328,33 @@ void AP_Param::send_parameter(const char *name, enum ap_var_type var_type, uint8
 uint16_t AP_Param::count_parameters(void)
 {
     // if we haven't cached the parameter count yet...
-    uint16_t ret = _parameter_count;
-    if (0 == ret) {
+    WITH_SEMAPHORE(_count_sem);
+    if (_parameter_count != 0 &&
+        _count_marker == _count_marker_done) {
+        return _parameter_count;
+    }
+    /*
+      cope with another thread invalidating the count while we are
+      counting
+     */
+    uint8_t limit = 4;
+    while ((_parameter_count == 0 ||
+            _count_marker != _count_marker_done) &&
+           limit--) {
         AP_Param  *vp;
         AP_Param::ParamToken token;
+        uint16_t count = 0;
+        uint16_t marker = _count_marker;
 
         for (vp = AP_Param::first(&token, nullptr);
              vp != nullptr;
              vp = AP_Param::next_scalar(&token, nullptr)) {
-            ret++;
+            count++;
         }
-        _parameter_count = ret;
+        _parameter_count = count;
+        _count_marker_done = marker;
     }
-    return ret;
+    return _parameter_count;
 }
 
 /*
@@ -2345,7 +2362,10 @@ uint16_t AP_Param::count_parameters(void)
  */
 void AP_Param::invalidate_count(void)
 {
-    _parameter_count = 0;
+    // we don't take the semaphore here as we don't want to block. The
+    // not-equal test is strong enough to ensure we get the right
+    // answer
+    _count_marker++;
 }
 
 /*
diff --git a/libraries/AP_Param/AP_Param.h b/libraries/AP_Param/AP_Param.h
index f4822a8488..c2ed166eeb 100644
--- a/libraries/AP_Param/AP_Param.h
+++ b/libraries/AP_Param/AP_Param.h
@@ -485,7 +485,7 @@ public:
 
     // set frame type flags. Used to unhide frame specific parameters
     static void set_frame_type_flags(uint16_t flags_to_set) {
-        _parameter_count = 0;
+        invalidate_count();
         _frame_type_flags |= flags_to_set;
     }
 
@@ -674,6 +674,9 @@ private:
     static StorageAccess        _storage;
     static uint16_t             _num_vars;
     static uint16_t             _parameter_count;
+    static uint16_t             _count_marker;
+    static uint16_t             _count_marker_done;
+    static HAL_Semaphore        _count_sem;
     static const struct Info *  _var_info;
 
     /*