B #6580: Fix casting in Template::remove (#3057)

The Template::remove could crash if user assign Single value to attribute, which is expected to be a Vector, e.g. SCHED_ACTION, PCI, ... Other minor changes: * refactor the 'set' method - no need to call private _set * Simplify some callers of the Template::remove method
OpenNebula · May 14, 2024 · 5dda6c3 · 5dda6c3
1 parent 52420f3
commit 5dda6c3
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 40 deletions.
diff --git a/include/Template.h b/include/Template.h
@@ -221,16 +221,15 @@ class Template
      *  will be freed when the template destructor is called.
      *    @param attr pointer to the attribute
      */
-    virtual void set(Attribute * attr);
+    void set(Attribute * attr);
 
-    virtual void set(std::vector<SingleAttribute *>& values)
-    {
-        _set<SingleAttribute>(values);
-    }
-
-    virtual void set(std::vector<VectorAttribute *>& values)
+    template<typename T>
+    void set(const std::vector<T *>& values)
     {
-        _set<VectorAttribute>(values);
+        for (auto v : values)
+        {
+            set(v);
+        }
     }
 
     /**
@@ -302,7 +301,15 @@ class Template
 
         for ( auto i = index.first; i != index.second; i++,j++ )
         {
-            values.push_back(static_cast<T *>(i->second));
+            auto att = dynamic_cast<T*>(i->second);
+
+            if (!att)
+            {
+                delete i->second;
+                continue;
+            }
+
+            values.push_back(att);
         }
 
         attributes.erase(index.first, index.second);
@@ -319,7 +326,14 @@ class Template
 
         for ( auto i = index.first; i != index.second; i++,j++ )
         {
-            std::unique_ptr<T> va(static_cast<T *>(i->second));
+            std::unique_ptr<T> va(dynamic_cast<T *>(i->second));
+
+            if (!va)
+            {
+                delete i->second;
+                continue;
+            }
+
             values.push_back(std::move(va));
         }
 
@@ -699,15 +713,6 @@ class Template
         return const_cast<T *>(
                 static_cast<const Template&>(*this).__get<T>(s));
     }
-
-    template<typename T>
-    void _set(const std::vector<T *>& values)
-    {
-        for (auto v : values)
-        {
-            set(v);
-        }
-    }
 };
 
 /* -------------------------------------------------------------------------- */

diff --git a/src/host/HostShareDatastore.cc b/src/host/HostShareDatastore.cc
@@ -24,7 +24,7 @@ using namespace std;
 
 void HostShareDatastore::set_monitorization(Template& ht)
 {
-    vector<Attribute *> vector_ds;
+    vector<VectorAttribute *> vector_ds;
 
     // -------------------------------------------------------------------------
     // Get overall datastore information
@@ -47,17 +47,9 @@ void HostShareDatastore::set_monitorization(Template& ht)
 
     ht.remove("DS", vector_ds);
 
-    for (auto it = vector_ds.begin(); it != vector_ds.end(); ++it)
+    for (auto ds: vector_ds)
     {
-        auto ds_attr = dynamic_cast<VectorAttribute*>(*it);
-
-        if (ds_attr == 0)
-        {
-            delete *it;
-            continue;
-        }
-
-        set(*it);
+        set(ds);
     }
 }
 

diff --git a/src/monitor/src/data_model/VirtualMachineBase.cc b/src/monitor/src/data_model/VirtualMachineBase.cc
@@ -156,7 +156,7 @@ static bool isVolatile(const VectorAttribute * disk)
 
 void VirtualMachineBase::init_storage_usage()
 {
-    vector<Attribute  *>            disks;
+    vector<VectorAttribute *> disks;
 
     long long   size;
     long long   snapshot_size;
@@ -168,15 +168,8 @@ void VirtualMachineBase::init_storage_usage()
 
     int num = vm_template->remove("DISK", disks);
 
-    for (auto d : disks)
+    for (auto disk : disks)
     {
-        const VectorAttribute * disk = dynamic_cast<const VectorAttribute*>(d);
-
-        if (disk == 0)
-        {
-            continue;
-        }
-
         if (disk->vector_value("SIZE", size) != 0)
         {
             continue;

diff --git a/src/vm/Backups.cc b/src/vm/Backups.cc
@@ -120,7 +120,7 @@ int Backups::from_xml(const ObjectXML* xml)
 int Backups::parse(Template *tmpl, bool can_increment,
                    bool append, std::string& error_str)
 {
-    vector<VectorAttribute *> cfg_a;
+    vector<Attribute *> cfg_a;
 
     int    iattr;
     bool   battr;