Re-enabling ASAN tests (#1617)

* SystemSC: size the per-FMU flag arrays in initialize(), not before
  fmus is filled, where they were always zero length (heap-buffer-overflow)
* AlgLoop: own the KinsolSolver, it was never deleted
* Enable ASAN for Clang, error out on compilers that cannot sanitize
* Add testsuite/lsan-suppressions.txt for wrapper, CPython and lxml leaks
* Mark 23 tests asan: no - they leak inside third-party FMU binaries

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: AnHeuermann

.github/workflows/coverage.yml

@@ -25,12 +25,11 @@ jobs:
           submodules: recursive
 
       # Everything else OMSimulator needs is vendored in 3rdParty/. flex is for
-      # omc-diff, which the testsuite compares result files with, and numpy for
-      # the CompositeModels tests that import it.
+      # omc-diff, which the testsuite compares result files with.
       - name: Install dependencies
         run: |
           sudo apt-get update
-          sudo apt-get install -y cmake g++ flex python3 python3-numpy gcovr ccache
+          sudo apt-get install -y cmake g++ flex python3 gcovr ccache
 
       # Only 3rdParty benefits much: the instrumented sources are rebuilt at -O0
       # anyway. It still takes the bulk of a cold build off the critical path.

.gitignore

@@ -15,3 +15,6 @@ imgui.ini
 /OMSimulator.creator.user
 /OMSimulator.files
 /OMSimulator.includes
+
+# Python files
+__pycache__/

Jenkinsfile

@@ -8,7 +8,6 @@ pipeline {
     booleanParam(name: 'MACOS_ARM64', defaultValue: false, description: 'Build with macOS-arm64 (M1 mac)')
     booleanParam(name: 'SUBMODULE_UPDATE', defaultValue: false, description: 'Allow pull request to update submodules (disabled by default due to common user errors)')
     booleanParam(name: 'UPLOAD_BUILD_OPENMODELICA', defaultValue: false, description: 'Upload install artifacts to build.openmodelica.org/omsimulator. Activates MINGW_UCRT64 as well.')
-    booleanParam(name: 'ASAN', defaultValue: false, description: 'Build and test with AddressSanitizer (disabled by default while the reported leaks are unfixed)')
     string(name: 'CTEST_FLAGS', defaultValue: '', description: 'Extra flags passed to ctest, e.g. -R api')
   }
   stages {
@@ -48,12 +47,6 @@ pipeline {
     stage('build-in-parallel') {
       parallel {
         stage('linux64-resolute-asan') {
-          // Disabled until the leaks AddressSanitizer reports have been fixed.
-          // Set the ASAN build parameter to run it in the meantime.
-          when {
-            expression { return params.ASAN }
-            beforeAgent true
-          }
           stages {
             stage('build-asan') {
               agent {

README.md

@@ -1,14 +1,17 @@
-# OMSimulator [![License: OSMC-PL](https://img.shields.io/badge/license-OSMC--PL-lightgrey.svg)](OSMC-License.txt)
+# OMSimulator
+
+[![License: OSMC-PL](https://img.shields.io/badge/license-OSMC--PL-lightgrey.svg)](OSMC-License.txt)
+[![codecov](https://codecov.io/gh/OpenModelica/OMSimulator/branch/master/graph/badge.svg?token=HmYNZuQtNA)](https://codecov.io/gh/OpenModelica/OMSimulator)
 
 The OpenModelica FMI & SSP-based co-simulation environment.
 
-### Branch Overview
+## Branch Overview
 
 - **`master`**: Development branch for active, unstable updates.
 - **`maintenance/v2.1`**: Stable branch for version 2.1 with patch updates (latest release: **v2.1.3**).
 - **`maintenance/v2.0`**: Stable branch for version 2.0 with patch updates (latest release: **v2.0.1**).
 
-### Overview
+## Overview
 
 OMSimulator can be used as:
 
@@ -18,18 +21,18 @@ OMSimulator can be used as:
 
 OMSimulator is also included with the OpenModelica installer, which includes OMEdit, a graphical editor.
 
-* [OpenModelica](https://openmodelica.org/)
-* [Standalone package](https://build.openmodelica.org/omsimulator/)
+- [OpenModelica](https://openmodelica.org/)
+- [Standalone package](https://build.openmodelica.org/omsimulator/)
 
-### Documentation
+## Documentation
 
 Latest documentation:
 
 - [User Guide PDF](https://openmodelica.org/doc/OMSimulator/master/OMSimulator/UsersGuide/pdf/OMSimulator.pdf)
 - [User Guide HTML](https://openmodelica.org/doc/OMSimulator/master/OMSimulator/UsersGuide/html/)
 - [OMSimulatorLib Doxygen](https://openmodelica.org/doc/OMSimulator/master/OMSimulator/doxygen/html/index.html)
 
-### Testing and Coverage
+## Testing and Coverage
 
 OMSimulator's coverage tests provide insights across a wide range of publicly available libraries. For an overview of all tested libraries and their detailed reports, visit the [Coverage Test Overview](https://libraries.openmodelica.org/branches/master-fmi/).
 
@@ -38,10 +41,10 @@ OMSimulator's coverage tests provide insights across a wide range of publicly av
 
 > **Note**: Issues may be attributed to OMSimulator itself or the FMI export in OpenModelica.
 
-### Build Instructions
+## Build Instructions
 
 To compile the project, please follow the steps outlined in the [BUILD.md](BUILD.md) file.
 
-### License
+## License
 
 This project is licensed under the OSMC Public License. See [OSMC-License.txt](OSMC-License.txt) for details.

src/OMSimulatorLib/AlgLoop.cpp

@@ -300,7 +300,9 @@ oms::KinsolSolver* oms::KinsolSolver::NewKinsolSolver(const int algLoopNum, cons
 {
   int flag;
   int printLevel;
-  KinsolSolver* kinsolSolver = new KinsolSolver();
+  /* Held by a unique_ptr until it is complete, so that the error returns below
+   * do not leak the object and the SUNDIALS memory it allocated so far. */
+  std::unique_ptr<KinsolSolver> kinsolSolver(new KinsolSolver());
 
   logDebug("Create new KinsolSolver object for algebraic loop number " + std::to_string(algLoopNum));
 
@@ -392,7 +394,7 @@ oms::KinsolSolver* oms::KinsolSolver::NewKinsolSolver(const int algLoopNum, cons
     fScaleData[i] = 1.0;
   }
 
-  return kinsolSolver;
+  return kinsolSolver.release();
 }
 
 /**
@@ -488,8 +490,8 @@ oms::AlgLoop::AlgLoop(oms_alg_solver_enu_t method, double relativeTolerance, scc
 
   if (method == oms_alg_solver_kinsol)
   {
-    kinsolData = KinsolSolver::NewKinsolSolver(systNumber, SCC.connections.size(), relativeTolerance, useDirectionalDerivative);
-    if (kinsolData==NULL)
+    kinsolData.reset(KinsolSolver::NewKinsolSolver(systNumber, SCC.connections.size(), relativeTolerance, useDirectionalDerivative));
+    if (!kinsolData)
     {
       logError("NewKinsolSolver() failed. Aborting!");
       throw("AlgLoop() failed!");

src/OMSimulatorLib/AlgLoop.h

@@ -36,6 +36,7 @@
 #ifndef _OMS_ALGLOOP_H_
 #define _OMS_ALGLOOP_H_
 
+#include <memory>
 #include <string>
 #include <vector>
 #include "OMSimulator/Types.h"
@@ -65,24 +66,27 @@ namespace oms
     oms_status_enu_t kinsolSolve(System& syst, DirectedGraph& graph);
 
   private:
+    /* All members start out empty, so that the destructor can be run on an
+     * object NewKinsolSolver() gave up on half way through. */
+
     /* tolerances */
-    double fnormtol;        /* function tolerance */
+    double fnormtol = 0.0;  /* function tolerance */
 
     /* work arrays */
-    N_Vector initialGuess;
-    N_Vector uScale;        /* Scaling vector for u */
-    N_Vector fScale;        /* Scaling vector for f(u) */
-    N_Vector fTmp;          /* Vector used for tmp computations */
+    N_Vector initialGuess = nullptr;
+    N_Vector uScale = nullptr;        /* Scaling vector for u */
+    N_Vector fScale = nullptr;        /* Scaling vector for f(u) */
+    N_Vector fTmp = nullptr;          /* Vector used for tmp computations */
 
     /* kinsol internal data */
-    void* kinsolMemory;
-    void* user_data;
-    int size;
+    void* kinsolMemory = nullptr;
+    void* user_data = nullptr;
+    int size = 0;
 
     /* linear solver data */
-    SUNLinearSolver linSol; /* Linear solver object used by KINSOL */
-    N_Vector y;             /* Template for cloning vectors needed inside linear solver */
-    SUNMatrix J;            /* (Non-)Sparse matrix template for cloning matrices needed within linear solver */
+    SUNLinearSolver linSol = nullptr; /* Linear solver object used by KINSOL */
+    N_Vector y = nullptr;             /* Template for cloning vectors needed inside linear solver */
+    SUNMatrix J = nullptr;            /* (Non-)Sparse matrix template for cloning matrices needed within linear solver */
 
     /* member function */
     static int nlsKinsolJac(N_Vector u, N_Vector fu, SUNMatrix J, void *user_data, N_Vector tmp1, N_Vector tmp2);
@@ -105,7 +109,9 @@ namespace oms
     oms_alg_solver_enu_t algSolverMethod;
     oms_status_enu_t fixPointIteration(System& syst, DirectedGraph& graph);
 
-    KinsolSolver* kinsolData;
+    /* Owns the solver: an AlgLoop is created as a temporary and moved into
+     * System::algLoops, so the copy constructor has to stay deleted. */
+    std::unique_ptr<KinsolSolver> kinsolData;
 
     /* Loop data */
     const scc_t SCC;            ///< Strong connected components

src/OMSimulatorLib/CMakeLists.txt

@@ -2,22 +2,45 @@ project(OMSimulatorLib)
 
 find_package(Threads)
 
+# GCC and Clang both work, but they are slightly different
 if (ASAN)
   set(CMAKE_BUILD_TYPE Debug)
-ENDIF ()
 
-IF (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+  if ((CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.1) OR
+      (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.1))
+    message(FATAL_ERROR
+            "ASAN=ON needs GCC 5.1 or newer, or Clang 3.1 or newer, but the selected "
+            "C++ compiler is ${CMAKE_CXX_COMPILER_ID} ${CMAKE_CXX_COMPILER_VERSION} "
+            "(${CMAKE_CXX_COMPILER}). Configure with a supported compiler, e.g. "
+            "-DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++, or build without -DASAN=ON.")
+  endif ()
+
+  message(STATUS "AddressSanitizer enabled")
+  add_compile_options("-fsanitize=address" "-fno-omit-frame-pointer")
+  link_libraries("-fsanitize=address")
+endif ()
+
+# Be pedantic about potential memory issues
+if (MSVC)
+  # MSVC doesn't know -Werror=<name>, its warnings only have numbers.
+  set(OMS_MSVC_MEMORY_WARNINGS
+        4700 # uninitialized local variable used                (-Wuninitialized)
+        4701 # potentially uninitialized local variable used    (-Wmaybe-uninitialized)
+        4703 # potentially uninitialized local pointer used     (-Wmaybe-uninitialized)
+        4715 # not all control paths return a value             (-Wreturn-type)
+        4716 # must return a value                              (-Wreturn-type)
+        4172 # returning address of local variable or temporary (-Wreturn-local-addr)
+     )
+  foreach (warning IN LISTS OMS_MSVC_MEMORY_WARNINGS)
+    # /w1<n> reports the warning at the default warning level (C4701 and C4703
+    # are level 4 and would stay silent otherwise), /we<n> turns it into an error.
+    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /w1${warning} /we${warning}")
+  endforeach ()
+else ()
   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=uninitialized")
-ELSEIF (CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 5.1)
-  SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=uninitialized")
-  SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=return-local-addr")
   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=return-type")
-  if (ASAN)
-    message(STATUS "AddressSanitizer enabled")
-    add_compile_options("-fsanitize=address" "-fno-omit-frame-pointer")
-    link_libraries("-fsanitize=address")
-  ENDIF ()
-ENDIF ()
+  SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror=return-local-addr")
+endif ()
 
 set(OMSIMULATORLIB_SOURCES
       AlgLoop.cpp

src/OMSimulatorLib/SystemSC.cpp

@@ -319,6 +319,11 @@ oms_status_enu_t oms::SystemSC::initialize()
     event_indicators_prev.push_back((double*)calloc(nEventIndicators.back(), sizeof(double)));
    }
 
+  // Now that fmus is filled, the per-FMU flags the integrator steps write to
+  // can be sized. make_unique value-initializes them, i.e. all false.
+  callEventUpdate = std::make_unique<bool[]>(fmus.size());
+  terminateSimulation = std::make_unique<bool[]>(fmus.size());
+
   if (n_states == 0)
     logInfo("model doesn't contain any continuous state");
 
@@ -511,8 +516,8 @@ oms_status_enu_t oms::SystemSC::terminate()
   }
 
   fmus.clear();
-  delete[] callEventUpdate;
-  delete[] terminateSimulation;
+  callEventUpdate.reset();
+  terminateSimulation.reset();
   nStates.clear();
   nEventIndicators.clear();
   states.clear();
@@ -579,6 +584,8 @@ oms_status_enu_t oms::SystemSC::reset()
   for (double* ptr : event_indicators) free(ptr);
   for (double* ptr : event_indicators_prev) free(ptr);
   fmus.clear();
+  callEventUpdate.reset();
+  terminateSimulation.reset();
   nStates.clear();
   nEventIndicators.clear();
   states.clear();

src/OMSimulatorLib/SystemSC.h

@@ -40,6 +40,8 @@
 #include "System.h"
 #include "OMSimulator/Types.h"
 
+#include <memory>
+
 #include <cvode/cvode.h>                  /* prototypes for CVODE fcts., consts. */
 #include <nvector/nvector_serial.h>       /* serial N_Vector types, fcts., macros */
 #include <sunlinsol/sunlinsol_dense.h>    /* Default dense linear solver */
@@ -90,8 +92,9 @@ namespace oms
   private:
     std::vector<Component*> fmus; // use Component Base class to support FMI 2 ME and FMI 3 ME
 
-    bool* callEventUpdate = new bool[fmus.size()](); //initialize with false
-    bool* terminateSimulation = new bool[fmus.size()](); //initialize with false
+    // One entry per FMU, allocated by initialize() once fmus is filled.
+    std::unique_ptr<bool[]> callEventUpdate;     //initialized with false
+    std::unique_ptr<bool[]> terminateSimulation; //initialized with false
 
     std::vector<size_t> nStates;
     std::vector<size_t> nEventIndicators;

testsuite/README.md

@@ -61,7 +61,8 @@ Special directives in the header of a test file:
 * `asan: yes`
 
   Also run the test case in an AddressSanitizer build (`-DOMS_TESTSUITE_ASAN=ON`), where all other
-  tests are skipped.
+  tests are skipped. Such a run leak checks the `OMSimulatorPython3` wrapper and the interpreter
+  as well; the leaks those report on their own are ignored through `lsan-suppressions.txt`.
 * `env: KEY=VALUE ...`
 
   Extra environment variables for the test case.