[All] Security : memory management, destructors and al (sofa-framework#5904)

* fix unchecked fopen() return

* Incorrect fwrite return value check

  - Issue: fwrite() returns number of items written, not bytes. Comparison was incorrect.
  - Fix: Changed comparisons from == sizeof(DDSHeader) and == getImageSize() to == 1

* fix potential command injection with sanity check

* fix thread safety by replacing volatile with std::atomic

* polymorphic base classes were missing virtual destructors - this could cause undefined behavior when deleting derived objects through base pointers

* Replace raw new/delete with smart pointers and vectors, and fix memory leak in debug file

* fix test of pointer and test ftell result

* add a new file to harden code, and move function to escape strings

* fix deletion of index which is not an C-array anymore

* fix compilation

* fix linking

Author: fredroy

Sofa/Component/Constraint/Lagrangian/Model/src/sofa/component/constraint/lagrangian/model/BaseContactLagrangianConstraint.h

@@ -40,6 +40,7 @@ namespace sofa::component::constraint::lagrangian::model
 // Every ContactParams should implement hasTangentialComponent
 struct BaseContactParams
 {
+    virtual ~BaseContactParams() = default;
     virtual bool hasTangentialComponent() const = 0;
 };
 

Sofa/Component/Mapping/NonLinear/src/sofa/component/mapping/nonlinear/DistanceFromTargetMapping.h

@@ -43,6 +43,7 @@ class DistanceFromTargetMappingInternalData
 
 struct BaseDistanceFromTargetMapping
 {
+    virtual ~BaseDistanceFromTargetMapping() = default;
     virtual void updateTarget( unsigned index, SReal x, SReal y, SReal z ) = 0;
 };
 

Sofa/GL/src/sofa/gl/VideoRecorderFFMPEG.cpp

@@ -31,6 +31,7 @@
 #include <sofa/helper/system/FileSystem.h>
 using sofa::helper::system::FileSystem;
 #include <sofa/helper/Utils.h>
+#include <sofa/type/hardening.h>
 
 namespace sofa::gl
 {
@@ -114,7 +115,7 @@ bool VideoRecorderFFMPEG::init(const std::string& ffmpeg_exec_filepath, const st
        << " -pix_fmt " << codec // yuv420p " // " yuv444p "
        << " -crf 17 "
        << " -vf vflip "
-       << "\"" << m_filename << "\""; // @TODO C++14 : replace with std::quoted
+       << "\"" << sofa::type::hardening::escapeForShell(m_filename) << "\"";
 
     const std::string& command_line = ss.str();
 

Sofa/GUI/Common/src/sofa/gui/common/ArgumentParser.cpp

@@ -81,15 +81,21 @@ void ArgumentParser::parse()
     // the original argv and argc
     // TODO: upgrade cxxopts to v3 and remove this copy
 
-    // copy argv into a temporary
-    char** copyArgv = new char* [m_argc + 1];
-    for (int i = 0; i < m_argc; i++) {
-        const size_t len = strlen(m_argv[i]) + 1;
-        copyArgv[i] = new char[len];
-        strncpy(copyArgv[i], m_argv[i], len);
-        copyArgv[i][len - 1] = '\0';
+    // copy argv into a vector (automatic cleanup via RAII)
+    std::vector<std::string> argStrings;
+    argStrings.reserve(m_argc);
+    for (int i = 0; i < m_argc; i++) 
+    {
+        argStrings.emplace_back(m_argv[i]);
+    }
+
+    // build char* array pointing to the strings
+    std::vector<char*> copyArgv;
+    copyArgv.reserve(m_argc + 1);
+    for (auto& s : argStrings) {
+        copyArgv.push_back(s.data());
     }
-    copyArgv[m_argc] = nullptr;
+    copyArgv.push_back(nullptr);
 
     int copyArgc = m_argc;
 
@@ -98,7 +104,7 @@ void ArgumentParser::parse()
     {
         extra.clear();
         m_options.parse_positional("input-file");
-        const auto temp = m_options.parse(copyArgc, copyArgv);
+        const auto temp = m_options.parse(copyArgc, copyArgv.data());
         vecArg = temp.arguments();
     }
     catch (const cxxopts::exceptions::exception& e)
@@ -122,12 +128,6 @@ void ArgumentParser::parse()
             }
         }
     }
-
-    // delete argv copy
-    for (int i = 0; i < m_argc; i++) {
-        delete[] copyArgv[i];
-    }
-    delete[] copyArgv;
 }
 
 void ArgumentParser::showArgs()

Sofa/framework/Helper/src/sofa/helper/io/ImageDDS.cpp

@@ -448,8 +448,8 @@ bool ImageDDS::save(std::string filename, int)
     }
 
     bool isWriteOk = true;
-    isWriteOk = isWriteOk && fwrite(&header, sizeof(DDSHeader), 1, file) == sizeof(DDSHeader);
-    isWriteOk = isWriteOk && fwrite(getPixels(), getImageSize(), 1, file) == getImageSize();
+    isWriteOk = isWriteOk && fwrite(&header, sizeof(DDSHeader), 1, file) == 1;
+    isWriteOk = isWriteOk && fwrite(getPixels(), getImageSize(), 1, file) == 1;
     fclose(file);
     if (!isWriteOk)
     {

Sofa/framework/Helper/src/sofa/helper/system/SetDirectory.cpp

@@ -33,6 +33,7 @@
 #endif
 #include <cstring>
 #include <iostream>
+#include <memory>
 
 #include <sofa/helper/logging/Messaging.h>
 
@@ -192,18 +193,15 @@ std::string SetDirectory::GetProcessFullPath(const char* filename)
 #elif defined (__APPLE__)
     if (!filename || filename[0]!='/')
     {
-        char* path = new char[4096];
-        uint32_t size;
-        if ( _NSGetExecutablePath( path, &size ) != 0)
+        uint32_t size = 4096;
+        std::unique_ptr<char[]> path(new char[size]);
+        if ( _NSGetExecutablePath( path.get(), &size ) != 0)
         {
             //realloc
-            delete [] path;
-            path = new char[size];
-            _NSGetExecutablePath( path, &size );
+            path.reset(new char[size]);
+            _NSGetExecutablePath( path.get(), &size );
         }
-        std::string finalPath(path);
-        delete [] path;
-        return finalPath;
+        return std::string(path.get());
     }
 #endif
 

Sofa/framework/Helper/src/sofa/helper/system/thread/CircularQueue.h

@@ -151,8 +151,8 @@ class SOFA_HELPER_API OneThreadPerEnd
     template<class T, class OutputIterator>
     unsigned pop(T array[], unsigned maxSize, unsigned maxCapacity, OutputIterator out, unsigned outmaxsize, bool clear = true);
 
-    volatile unsigned head;
-    volatile unsigned tail;
+    std::atomic<unsigned> head;
+    std::atomic<unsigned> tail;
 };
 
 /**

Sofa/framework/Helper/src/sofa/helper/system/thread/debug.cpp

@@ -95,6 +95,7 @@ TraceProfile::TraceProfile(const char *name, int index, int size)
 TraceProfile::~TraceProfile()
 {
     delete[] name;
+    delete[] times;
 }
 
 void TraceProfile::addTime(int instant, int time)

Sofa/framework/Simulation/Core/src/sofa/simulation/task/DefaultTaskScheduler.h

@@ -124,7 +124,7 @@ class SOFA_SIMULATION_CORE_API DefaultTaskScheduler : public TaskScheduler
 
     unsigned m_workerThreadCount;
 
-    volatile bool m_workerThreadsIdle;
+    std::atomic<bool> m_workerThreadsIdle;
 
     bool m_isClosing;
 

Sofa/framework/Simulation/Core/src/sofa/simulation/task/Task.h

@@ -45,8 +45,9 @@ class SOFA_SIMULATION_CORE_API Task
     class SOFA_SIMULATION_CORE_API Allocator
     {
     public:
+        virtual ~Allocator() = default;
         virtual void* allocate(std::size_t sz) = 0;
-                
+
         virtual void free(void* ptr, std::size_t sz) = 0;
     };