From de7ffc5906e0ce4117ca91f13c1e3a8f858cefd7 Mon Sep 17 00:00:00 2001 From: Monty Montgomery Date: Sun, 22 May 2022 11:29:10 -0400 Subject: [PATCH] Eliminate a "C++ initialization order fiasco" for geometryregister Current initialization of the global geometryregister suffers from a classic 'initialization order fiasco'. Depending on the order the compilation units are loaded/linked, the initialization of the global geometryregisterarray is not guaranteed to happen (and indeed often does not happen) before it is used. This leads to entries being appended before it's initialized (usually 'suceeding, but potentially causing memory corruption if the segment at that point isn't zeroed), initialization then happening halfway through (wiping the initial entries) and then the last entries being the only ones that show up. The net effect is either a crash at startup, or several geometry types seeming to be missing. Eg, step files will oad, but STL files are just ignored. The bug is actively observed on, eg, Linux. This patch implements a simple 'initialize at first access' convention for the array, eliminating the ordering problem. I've not reviewed the rest of the source for other potential examples of the fiasco pattern; this fixes only the geometryregister, since that was actively biting. --- libsrc/csg/csgeom.cpp | 3 ++- libsrc/csg/csgpkg.cpp | 3 ++- libsrc/geom2d/geom2dpkg.cpp | 3 ++- libsrc/geom2d/geometry2d.cpp | 3 ++- libsrc/interface/nginterface.cpp | 11 +++++++---- libsrc/meshing/basegeom.cpp | 9 ++++++--- libsrc/meshing/basegeom.hpp | 7 ++----- libsrc/meshing/meshclass.cpp | 3 ++- libsrc/meshing/python_mesh.cpp | 3 ++- libsrc/occ/occpkg.cpp | 3 ++- libsrc/stlgeom/stlgeom.cpp | 3 ++- libsrc/stlgeom/stlpkg.cpp | 8 +++++--- libsrc/stlgeom/vsstl.cpp | 6 ++++-- libsrc/visualization/stlmeshing.cpp | 6 ++++-- ng/ngpkg.cpp | 17 ++++++++++------- 15 files changed, 54 insertions(+), 34 deletions(-) diff --git a/libsrc/csg/csgeom.cpp b/libsrc/csg/csgeom.cpp index 45c76170..e22db8b4 100644 --- a/libsrc/csg/csgeom.cpp +++ b/libsrc/csg/csgeom.cpp @@ -1673,7 +1673,8 @@ namespace netgen public: CSGInit() { - geometryregister.Append (new CSGeometryRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new CSGeometryRegister); } }; diff --git a/libsrc/csg/csgpkg.cpp b/libsrc/csg/csgpkg.cpp index ced6b626..346e49ea 100644 --- a/libsrc/csg/csgpkg.cpp +++ b/libsrc/csg/csgpkg.cpp @@ -570,7 +570,8 @@ using namespace netgen; int Ng_CSG_Init (Tcl_Interp * interp) { - geometryregister.Append (new CSGeometryVisRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new CSGeometryVisRegister); if (interp == NULL) return TCL_OK; diff --git a/libsrc/geom2d/geom2dpkg.cpp b/libsrc/geom2d/geom2dpkg.cpp index 87392192..13c43911 100644 --- a/libsrc/geom2d/geom2dpkg.cpp +++ b/libsrc/geom2d/geom2dpkg.cpp @@ -50,6 +50,7 @@ extern "C" int Ng_geom2d_Init (Tcl_Interp * interp); int Ng_geom2d_Init (Tcl_Interp * interp) { - geometryregister.Append (new SplineGeometryVisRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new SplineGeometryVisRegister); return TCL_OK; } diff --git a/libsrc/geom2d/geometry2d.cpp b/libsrc/geom2d/geometry2d.cpp index f3332258..1d35b0e4 100644 --- a/libsrc/geom2d/geometry2d.cpp +++ b/libsrc/geom2d/geometry2d.cpp @@ -1100,7 +1100,8 @@ namespace netgen public: SplineGeoInit() { - geometryregister.Append (new SplineGeometryRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new SplineGeometryRegister); } }; diff --git a/libsrc/interface/nginterface.cpp b/libsrc/interface/nginterface.cpp index d98b797c..0d15b4d0 100644 --- a/libsrc/interface/nginterface.cpp +++ b/libsrc/interface/nginterface.cpp @@ -81,9 +81,10 @@ void Ng_LoadGeometry (const char * filename) return; } - for (int i = 0; i < geometryregister.Size(); i++) + GeometryRegisterArray &gra = FetchGeometryRegisterArray(); + for (int i = 0; i < gra.Size(); i++) { - NetgenGeometry * hgeom = geometryregister[i]->Load (filename); + NetgenGeometry * hgeom = gra[i]->Load (filename); if (hgeom) { ng_geometry.reset (hgeom); @@ -104,7 +105,8 @@ void Ng_LoadMeshFromStream ( istream & input ) mesh -> Load(input); SetGlobalMesh (mesh); - ng_geometry = geometryregister.LoadFromMeshFile (input); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + ng_geometry = gra.LoadFromMeshFile (input); if (!ng_geometry) ng_geometry = make_shared(); @@ -259,7 +261,8 @@ void Ng_LoadMesh (const char * filename, ngcore::NgMPI_Comm comm) shared_ptr geo; if(buf.Size()) { // if we had geom-info in the file, take it istringstream geom_infile(string((const char*)&buf[0], buf.Size())); - geo = geometryregister.LoadFromMeshFile(geom_infile); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + geo = gra.LoadFromMeshFile(geom_infile); } if(geo!=nullptr) { ng_geometry = geo; diff --git a/libsrc/meshing/basegeom.cpp b/libsrc/meshing/basegeom.cpp index 586d351f..09da553a 100644 --- a/libsrc/meshing/basegeom.cpp +++ b/libsrc/meshing/basegeom.cpp @@ -29,9 +29,6 @@ namespace netgen double GetTolerance() { return tree.GetTolerance(); } }; - DLL_HEADER GeometryRegisterArray geometryregister; - //DLL_HEADER NgArray geometryregister; - GeometryRegister :: ~GeometryRegister() { ; } @@ -1165,4 +1162,10 @@ namespace netgen } static RegisterClassForArchive regnggeo; + + GeometryRegisterArray& FetchGeometryRegisterArray (){ + static GeometryRegisterArray *geometryregister = new GeometryRegisterArray(); + return *geometryregister; + } + } diff --git a/libsrc/meshing/basegeom.hpp b/libsrc/meshing/basegeom.hpp index 228ecfad..0f3201b7 100644 --- a/libsrc/meshing/basegeom.hpp +++ b/libsrc/meshing/basegeom.hpp @@ -339,15 +339,12 @@ namespace netgen public: virtual ~GeometryRegisterArray() { - for (int i = 0; i < Size(); i++) - delete (*this)[i]; + DeleteAll(); } - virtual shared_ptr LoadFromMeshFile (istream & ist) const; }; - // extern DLL_HEADER NgArray geometryregister; - extern DLL_HEADER GeometryRegisterArray geometryregister; + DLL_HEADER GeometryRegisterArray& FetchGeometryRegisterArray (); } diff --git a/libsrc/meshing/meshclass.cpp b/libsrc/meshing/meshclass.cpp index b323e87b..d6812cc3 100644 --- a/libsrc/meshing/meshclass.cpp +++ b/libsrc/meshing/meshclass.cpp @@ -1660,7 +1660,8 @@ namespace netgen clusters -> Update(); } - auto geo = geometryregister.LoadFromMeshFile (infile); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + auto geo = gra.LoadFromMeshFile (infile); if(geo) geometry = geo; diff --git a/libsrc/meshing/python_mesh.cpp b/libsrc/meshing/python_mesh.cpp index 5f390832..1b16e3e7 100644 --- a/libsrc/meshing/python_mesh.cpp +++ b/libsrc/meshing/python_mesh.cpp @@ -838,7 +838,8 @@ DLL_HEADER void ExportNetgenMeshing(py::module &m) shared_ptr geo; if(buf.Size()) { // if we had geom-info in the file, take it istringstream geom_infile(string((const char*)buf.Data(), buf.Size())); - geo = geometryregister.LoadFromMeshFile(geom_infile); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + geo = gra.LoadFromMeshFile(geom_infile); } if(geo!=nullptr) mesh->SetGeometry(geo); else if(ng_geometry!=nullptr) mesh->SetGeometry(ng_geometry); diff --git a/libsrc/occ/occpkg.cpp b/libsrc/occ/occpkg.cpp index 80c38e6e..be33bc16 100644 --- a/libsrc/occ/occpkg.cpp +++ b/libsrc/occ/occpkg.cpp @@ -944,7 +944,8 @@ using namespace netgen; int Ng_occ_Init (Tcl_Interp * interp) { - geometryregister.Append (new OCCGeometryRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new OCCGeometryRegister); Tcl_CreateCommand (interp, "Ng_SetOCCVisParameters", diff --git a/libsrc/stlgeom/stlgeom.cpp b/libsrc/stlgeom/stlgeom.cpp index fe5cc4a0..b120a09b 100644 --- a/libsrc/stlgeom/stlgeom.cpp +++ b/libsrc/stlgeom/stlgeom.cpp @@ -3748,7 +3748,8 @@ void STLGeometry :: WriteChartToFile( ChartId chartnumber, filesystem::path file public: STLInit() { - geometryregister.Append (new STLGeometryRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new STLGeometryRegister); } }; diff --git a/libsrc/stlgeom/stlpkg.cpp b/libsrc/stlgeom/stlpkg.cpp index ff0897f0..3d6005b5 100644 --- a/libsrc/stlgeom/stlpkg.cpp +++ b/libsrc/stlgeom/stlpkg.cpp @@ -521,8 +521,9 @@ namespace netgen Tcl_Interp * interp, int argc, tcl_const char *argv[]) { - for (int i = 0; i < geometryregister.Size(); i++) - geometryregister[i] -> SetParameters (interp); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + for (int i = 0; i < gra.Size(); i++) + gra[i]->SetParameters (interp); Ng_SetMeshingParameters (clientData, interp, argc, argv); @@ -564,7 +565,8 @@ using namespace netgen; extern "C" int Ng_stl_Init (Tcl_Interp * interp); int Ng_stl_Init (Tcl_Interp * interp) { - geometryregister.Append (new STLGeometryVisRegister); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + gra.Append (new STLGeometryVisRegister); Tcl_CreateCommand (interp, "Ng_SetSTLParameters", Ng_SetSTLParameters, (ClientData)NULL, diff --git a/libsrc/stlgeom/vsstl.cpp b/libsrc/stlgeom/vsstl.cpp index 854909d8..ef6f7fff 100644 --- a/libsrc/stlgeom/vsstl.cpp +++ b/libsrc/stlgeom/vsstl.cpp @@ -33,8 +33,10 @@ VisualSceneSTLMeshing :: VisualSceneSTLMeshing () { selecttrig = 0; nodeofseltrig = 1; - stlgeometry->SetSelectTrig(selecttrig); - stlgeometry->SetNodeOfSelTrig(nodeofseltrig); + if(stlgeometry){ // don't let the default initializer crash init + stlgeometry->SetSelectTrig(selecttrig); + stlgeometry->SetNodeOfSelTrig(nodeofseltrig); + } } VisualSceneSTLMeshing :: ~VisualSceneSTLMeshing () diff --git a/libsrc/visualization/stlmeshing.cpp b/libsrc/visualization/stlmeshing.cpp index 70699c56..ff3dcba0 100644 --- a/libsrc/visualization/stlmeshing.cpp +++ b/libsrc/visualization/stlmeshing.cpp @@ -33,8 +33,10 @@ VisualSceneSTLMeshing :: VisualSceneSTLMeshing () { selecttrig = 0; nodeofseltrig = 1; - stlgeometry->SetSelectTrig(selecttrig); - stlgeometry->SetNodeOfSelTrig(nodeofseltrig); + if(stlgeometry){ // don't let the default initializer crash init + stlgeometry->SetSelectTrig(selecttrig); + stlgeometry->SetNodeOfSelTrig(nodeofseltrig); + } } VisualSceneSTLMeshing :: ~VisualSceneSTLMeshing () diff --git a/ng/ngpkg.cpp b/ng/ngpkg.cpp index 96ef4ea1..4e79efd9 100644 --- a/ng/ngpkg.cpp +++ b/ng/ngpkg.cpp @@ -465,15 +465,16 @@ namespace netgen try { - for (int i = 0; i < geometryregister.Size(); i++) + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + for (int i = 0; i < gra.Size(); i++) { - NetgenGeometry * hgeom = geometryregister[i]->Load (lgfilename); + NetgenGeometry * hgeom = gra[i]->Load (lgfilename); if (hgeom) { // delete ng_geometry; // ng_geometry = hgeom; ng_geometry = shared_ptr (hgeom); - geometryregister[i]->SetParameters(interp); + gra[i]->SetParameters(interp); mesh.reset(); return TCL_OK; @@ -1473,8 +1474,9 @@ namespace netgen extern void Render(bool blocking); mparam.render_function = &Render; - for (int i = 0; i < geometryregister.Size(); i++) - geometryregister[i] -> SetParameters (interp); + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + for (int i = 0; i < gra.Size(); i++) + gra[i]->SetParameters (interp); Ng_SetMeshingParameters (clientData, interp, 0, argv); @@ -1924,9 +1926,10 @@ namespace netgen { if (strcmp (vismode, "geometry") == 0) { - for (int i = 0; i < geometryregister.Size(); i++) + GeometryRegisterArray& gra = FetchGeometryRegisterArray(); + for (int i = 0; i < gra.Size(); i++) { - VisualScene * hvs = geometryregister[i]->GetVisualScene (ng_geometry.get()); + VisualScene * hvs = gra[i]->GetVisualScene (ng_geometry.get()); if (hvs) { vs = hvs;