From 380ecdf8ba5d938e1d06f5cf05ed49c23dfa1e8d Mon Sep 17 00:00:00 2001
From: Julian Smith <jules@op59.net>
Date: Fri, 18 Dec 2020 17:16:50 +0000
Subject: [PATCH] src/Viewer/viewmgr.*: added asserts, diagnostics and checking
 for out-of-range view numbers.

It seems that some aircraft sometimes incorrectly pass view indices rather
than numbers resulting in out-of-range access to FGViewMgr::views[]; See
flightgear-devel thread with subject "View crash (after loading errors)".

With this commit, if we are given an incorrect view number, we output a
diagnostic with SG_ALERT, assert fail, and cope with the problem. Arguably we
could do something more serious such as opening a popup or throw an exception.
---
 src/Viewer/viewmgr.cxx | 52 +++++++++++++++++++++++-------------------
 src/Viewer/viewmgr.hxx | 11 ++++-----
 2 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/src/Viewer/viewmgr.cxx b/src/Viewer/viewmgr.cxx
index b3670ec6f..0233f938f 100644
--- a/src/Viewer/viewmgr.cxx
+++ b/src/Viewer/viewmgr.cxx
@@ -75,6 +75,15 @@ FGViewMgr::init ()
 
     config_list = fgGetNode("/sim", true)->getChildren("view");
     _current = fgGetInt("/sim/current-view/view-number");
+    if (_current != 0 && (_current < 0 || _current >= (int) views.size())) {
+        SG_LOG(SG_VIEW, SG_ALERT,
+                "Invalid /sim/current-view/view-number=" << _current
+                << ". views.size()=" << views.size()
+                << ". Will assert false and use zero."
+                );
+        assert(0);
+        _current = 0;
+    }
 
     for (unsigned int i = 0; i < config_list.size(); i++) {
         SGPropertyNode* n = config_list[i];
@@ -185,23 +194,16 @@ void FGViewMgr::clear()
 flightgear::View*
 FGViewMgr::get_current_view()
 {
-    const auto numViews = static_cast<int>(views.size());
-    if ((_current >= 0) && (_current < numViews)) {
-        return views.at(_current);
-    } else {
+    if (views.empty())
         return nullptr;
-    }
+    assert(_current >= 0 && _current < (int) views.size());
+    return views[_current];
 }
 
 const flightgear::View*
 FGViewMgr::get_current_view() const
 {
-    const auto numViews = static_cast<int>(views.size());
-    if ((_current >= 0) && (_current < numViews)) {
-        return views.at(_current);
-    } else {
-        return nullptr;
-    }
+    return const_cast<FGViewMgr*>(this)->get_current_view();
 }
 
 
@@ -294,7 +296,6 @@ void FGViewMgr::setCurrentViewIndex(int newview)
     if (newview == _current) {
         return;
     }
-
     // negative numbers -> set view with node index -newview
     if (newview < 0) {
         for (int i = 0; i < (int)config_list.size(); i++) {
@@ -302,25 +303,28 @@ void FGViewMgr::setCurrentViewIndex(int newview)
             if (index == newview)
                 newview = i;
         }
-        if (newview < 0)
+        if (newview < 0) {
+            SG_LOG(SG_VIEW, SG_ALERT,
+                    "Failed to find -ve newview=" << newview
+                    << ". Will assert false and ignore."
+                    );
+            assert(0);
             return;
+        }
     }
-
-    // not sure it really makes sense to be doing this wrapping logic
-    // here, it could mask various strange inputs, But keeping for compat
-    // for now.
-    const auto numViews = static_cast<int>(views.size());
-    if (newview < 0) { // wrap to last
-        newview = numViews - 1;
-    } else if (newview >= numViews) { //  wrap to zero
-        newview = 0;
+    if (newview < 0 || newview >= (int) views.size()) {
+        SG_LOG(SG_VIEW, SG_ALERT, "Invalid newview=" << newview
+                << ". views.size()=" << views.size()
+                << ". Will assert false and ignore."
+                );
+        assert(0);
+        return;
     }
 
     if (get_current_view()) {
-	get_current_view()->unbind();
+	    get_current_view()->unbind();
     }
 
-    // set new view
     _current = newview;
 
     if (get_current_view()) {
diff --git a/src/Viewer/viewmgr.hxx b/src/Viewer/viewmgr.hxx
index 0036d5b71..da6c55d82 100644
--- a/src/Viewer/viewmgr.hxx
+++ b/src/Viewer/viewmgr.hxx
@@ -75,19 +75,16 @@ public:
     flightgear::View* next_view();
     flightgear::View* prev_view();
 
+    // Support for extra view windows. This only works if --compositer-viewer=1 was specified.
     //
+    
+    // Calls SviewPush().
     void view_push();
     
-    // Experimental. Only works if --compositer-viewer=1 was specified. Creates
-    // new window with clone of current view. As of 2020-09-03, the clone's
-    // scenery is not displayed correctly.
+    // These all end up calling SviewCreate().
     void clone_current_view(const SGPropertyNode* config);
-    
-    // 
     void clone_last_pair(const SGPropertyNode* config);
-
     void clone_last_pair_double(const SGPropertyNode* config);
-    
     void view_new(const SGPropertyNode* config);
 
     // setters