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 45c761703..e22db8b42 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 ced6b626c..346e49ea6 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 87392192f..13c439112 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 f3332258b..1d35b0e42 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 d98b797c1..0d15b4d07 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 586d351f8..09da553aa 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 228ecfadc..0f3201b7f 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 b323e87b7..d6812cc34 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 5f3908326..1b16e3e73 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 80c38e6e0..be33bc164 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 fe5cc4a03..b120a09b2 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 ff0897f0a..3d6005b5c 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 854909d8d..ef6f7fff2 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 70699c56a..ff3dcba00 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 96ef4ea12..4e79efd9d 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;