From 2b379b9f08b25ef5fc909397f607ac29bc849f46 Mon Sep 17 00:00:00 2001 From: Pasukhin Dmitry Date: Sun, 14 Jun 2026 12:39:10 +0100 Subject: [PATCH] Foundation Classes - Fix thread-safety of Strtod by making Bigint pool thread-local (#1309) - The Strtod() function used a shared static Bigint memory pool (private_mem, pmem_next, TI0) without synchronization. - When multiple threads called Strtod() concurrently (e.g. parallel BRep reading), the pool was corrupted causing random mis-parses of numeric values. - Fix: declare private_mem, pmem_next and TI0 as thread_local so each thread gets its own independent memory pool and freelist. - Added Standard_Strtod_Test.cxx with parallel correctness tests using OSD_Parallel::For. - Added BRepTools_Test.cxx with integration tests for parallel BRep file reading. --- .../TKernel/GTests/FILES.cmake | 1 + .../TKernel/GTests/Standard_Strtod_Test.cxx | 160 ++++++++++++++++++ .../TKernel/Standard/Standard_Strtod.cxx | 5 +- .../TKBRep/GTests/BRepTools_Test.cxx | 142 ++++++++++++++++ src/ModelingData/TKBRep/GTests/FILES.cmake | 1 + 5 files changed, 307 insertions(+), 2 deletions(-) create mode 100644 src/FoundationClasses/TKernel/GTests/Standard_Strtod_Test.cxx create mode 100644 src/ModelingData/TKBRep/GTests/BRepTools_Test.cxx diff --git a/src/FoundationClasses/TKernel/GTests/FILES.cmake b/src/FoundationClasses/TKernel/GTests/FILES.cmake index 32bd462d56..ff3d876202 100644 --- a/src/FoundationClasses/TKernel/GTests/FILES.cmake +++ b/src/FoundationClasses/TKernel/GTests/FILES.cmake @@ -46,6 +46,7 @@ set(OCCT_TKernel_GTests_FILES Standard_Failure_Test.cxx Standard_GUID_Test.cxx Standard_Handle_Test.cxx + Standard_Strtod_Test.cxx TCollection_AsciiString_Test.cxx TCollection_ExtendedString_Test.cxx TopLoc_Location_Test.cxx diff --git a/src/FoundationClasses/TKernel/GTests/Standard_Strtod_Test.cxx b/src/FoundationClasses/TKernel/GTests/Standard_Strtod_Test.cxx new file mode 100644 index 0000000000..ed0fa87720 --- /dev/null +++ b/src/FoundationClasses/TKernel/GTests/Standard_Strtod_Test.cxx @@ -0,0 +1,160 @@ +// Copyright (c) 2026 OPEN CASCADE SAS +// +// This file is part of Open CASCADE Technology software library. +// +// This library is free software; you can redistribute it and/or modify it under +// the terms of the GNU Lesser General Public License version 2.1 as published +// by the Free Software Foundation, with special exception defined in the file +// OCCT_LGPL_EXCEPTION.txt. Consult the file LICENSE_LGPL_21.txt included in OCCT +// distribution for complete text of the license and disclaimer of any warranty. +// +// Alternatively, this file may be used under the terms of Open CASCADE +// commercial license or contractual agreement. + +#include +#include +#include + +#include + +#include +#include + +namespace +{ + +struct TestCase +{ + const char* Input; + double Expected; +}; + +// A representative set of numeric strings that exercise different code paths +// inside Strtod: simple integers, fractions, exponents, large/small values, +// and edge cases that trigger the Bigint memory pool (Balloc/Bfree). +static const TestCase THE_TEST_CASES[] = { + {"0.0", 0.0}, + {"1.0", 1.0}, + {"3.141592653589793", 3.141592653589793}, + {"-273.15", -273.15}, + {"1e10", 1e10}, + {"1.23456789e-15", 1.23456789e-15}, + {"123456789.987654321", 123456789.987654321}, + {"1.0e20", 1.0e20}, + {"1.0e-20", 1.0e-20}, + {"9.999999999999999e300", 9.999999999999999e300}, + {"5e-324", 5e-324}, + {"2.2250738585072014e-308", 2.2250738585072014e-308}, + {"17976931348623157.0", 17976931348623157.0}, + {"0.000001", 0.000001}, + {"999999999999999.0", 999999999999999.0}, +}; + +static const int THE_NB_CASES = sizeof(THE_TEST_CASES) / sizeof(THE_TEST_CASES[0]); + +} // namespace + +// Basic sanity: Strtod parses known values correctly in a single thread. +TEST(Standard_StrtodTest, SingleThread_Correctness) +{ + for (int i = 0; i < THE_NB_CASES; ++i) + { + char* anEnd = nullptr; + double aVal = Strtod(THE_TEST_CASES[i].Input, &anEnd); + EXPECT_TRUE(anEnd != THE_TEST_CASES[i].Input) + << "No digits parsed for: " << THE_TEST_CASES[i].Input; + EXPECT_DOUBLE_EQ(aVal, THE_TEST_CASES[i].Expected) + << "Mismatch for input: " << THE_TEST_CASES[i].Input; + } +} + +// The core regression test: concurrent Strtod calls must not corrupt each +// other's results. Prior to the fix, the Bigint memory pool (private_mem, +// pmem_next, TI0) was a single global static shared across threads without +// synchronization, causing random mis-parses. +TEST(Standard_StrtodTest, Parallel_ConsistentResults) +{ + constexpr int aNbThreads = 200; + + NCollection_Array1 aResults(0, aNbThreads - 1); + for (int i = 0; i < aNbThreads; ++i) + { + aResults(i) = -1.0; + } + + // Use a string long enough to force use of the Bigint memory pool + // (values with many significant digits exercise Balloc paths). + const char* aTestInput = "3.14159265358979323846264338327950288419716939937510"; + + OSD_Parallel::For(0, aNbThreads, [&](int theIndex) { + char* anEnd = nullptr; + double aVal = Strtod(aTestInput, &anEnd); + aResults(theIndex) = aVal; + }); + + // All results must be identical to the single-threaded reference. + char* aRefEnd = nullptr; + double aRef = Strtod(aTestInput, &aRefEnd); + + for (int i = 0; i < aNbThreads; ++i) + { + EXPECT_DOUBLE_EQ(aResults(i), aRef) + << "Thread " << i << " produced a different result: " << aResults(i); + } +} + +// Run many iterations to increase the chance of detecting intermittent races. +TEST(Standard_StrtodTest, Parallel_RepeatedRuns) +{ + constexpr int aNbThreads = 100; + constexpr int aNbRuns = 10; + const char* aTestInput = "2.71828182845904523536028747135266249775724709369995"; + + char* aRefEnd = nullptr; + double aRef = Strtod(aTestInput, &aRefEnd); + + for (int aRun = 0; aRun < aNbRuns; ++aRun) + { + NCollection_Array1 aResults(0, aNbThreads - 1); + + OSD_Parallel::For(0, aNbThreads, [&](int theIndex) { + char* anEnd = nullptr; + double aVal = Strtod(aTestInput, &anEnd); + aResults(theIndex) = aVal; + }); + + for (int i = 0; i < aNbThreads; ++i) + { + EXPECT_DOUBLE_EQ(aResults(i), aRef) << "Run " << aRun << ", thread " << i << " mismatch"; + } + } +} + +// Test with many distinct inputs processed in parallel to exercise different +// code paths (small values, large values, many-digit values) concurrently. +TEST(Standard_StrtodTest, Parallel_ManyDistinctInputs) +{ + constexpr int aNbThreads = 100; + + NCollection_Array1 aResults(0, aNbThreads - 1); + NCollection_Array1 aExpected(0, aNbThreads - 1); + + // Compute expected values single-threaded first. + for (int i = 0; i < aNbThreads; ++i) + { + const char* anInput = THE_TEST_CASES[i % THE_NB_CASES].Input; + aExpected(i) = Strtod(anInput, nullptr); + } + + // Now compute in parallel. + OSD_Parallel::For(0, aNbThreads, [&](int theIndex) { + const char* anInput = THE_TEST_CASES[theIndex % THE_NB_CASES].Input; + aResults(theIndex) = Strtod(anInput, nullptr); + }); + + for (int i = 0; i < aNbThreads; ++i) + { + EXPECT_DOUBLE_EQ(aResults(i), aExpected(i)) + << "Thread " << i << " mismatch for input: " << THE_TEST_CASES[i % THE_NB_CASES].Input; + } +} diff --git a/src/FoundationClasses/TKernel/Standard/Standard_Strtod.cxx b/src/FoundationClasses/TKernel/Standard/Standard_Strtod.cxx index 44ec12a9cd..3e36400ec4 100644 --- a/src/FoundationClasses/TKernel/Standard/Standard_Strtod.cxx +++ b/src/FoundationClasses/TKernel/Standard/Standard_Strtod.cxx @@ -294,7 +294,8 @@ extern "C" #define PRIVATE_MEM 2304 #endif #define PRIVATE_mem ((PRIVATE_MEM + sizeof(double) - 1) / sizeof(double)) -static double private_mem[PRIVATE_mem], *pmem_next = private_mem; +static thread_local double private_mem[PRIVATE_mem] = {}; +static thread_local double* pmem_next = private_mem; #endif #undef IEEE_Arith @@ -1145,7 +1146,7 @@ typedef struct ThInfo Bigint* P5s; } ThInfo; -static ThInfo TI0; +static thread_local ThInfo TI0; #ifdef MULTIPLE_THREADS static ThInfo* TI1; diff --git a/src/ModelingData/TKBRep/GTests/BRepTools_Test.cxx b/src/ModelingData/TKBRep/GTests/BRepTools_Test.cxx new file mode 100644 index 0000000000..09ed351fcc --- /dev/null +++ b/src/ModelingData/TKBRep/GTests/BRepTools_Test.cxx @@ -0,0 +1,142 @@ +// Copyright (c) 2026 OPEN CASCADE SAS +// +// This file is part of Open CASCADE Technology software library. +// +// This library is free software; you can redistribute it and/or modify it under +// the terms of the GNU Lesser General Public License version 2.1 as published +// by the Free Software Foundation, with special exception defined in the file +// OCCT_LGPL_EXCEPTION.txt. Consult the file LICENSE_LGPL_21.txt included in OCCT +// distribution for complete text of the license and disclaimer of any warranty. +// +// Alternatively, this file may be used under the terms of Open CASCADE +// commercial license or contractual agreement. + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace +{ + +// Write a box to a BRep string once, to be reused by all threads. +std::string MakeBoxBrepString() +{ + BRepPrimAPI_MakeBox aBox(10.0, 20.0, 30.0); + EXPECT_FALSE(aBox.Shape().IsNull()); + + std::ostringstream anOss; + BRepTools::Write(aBox.Shape(), anOss); + EXPECT_FALSE(anOss.str().empty()); + return anOss.str(); +} + +// Read a shape from a BRep buffer. Each call uses its own stream. +bool ReadFromBuffer(const std::string& theData, TopoDS_Shape& theShape) +{ + std::istringstream anIss(theData); + BRep_Builder aBuilder; + BRepTools::Read(theShape, anIss, aBuilder); + return !theShape.IsNull(); +} + +} // namespace + +// Parallel BRep reading must produce shapes with identical volume properties. +// The underlying bug was that Strtod() shared a single Bigint memory pool +// across all threads, causing random mis-parses of numeric values in .brep +// files when read concurrently. +TEST(BRepTools_Test, ParallelRead_IdenticalMass) +{ + const std::string aBrepData = MakeBoxBrepString(); + + // Single-threaded reference + TopoDS_Shape aRefShape; + ASSERT_TRUE(ReadFromBuffer(aBrepData, aRefShape)); + GProp_GProps aRefProps; + BRepGProp::VolumeProperties(aRefShape, aRefProps); + const double aRefMass = std::abs(aRefProps.Mass()); + ASSERT_GT(aRefMass, 0.0); + + constexpr int aNbReads = 200; + + NCollection_Array1 aShapes(0, aNbReads - 1); + std::atomic aFailCount{0}; + + OSD_Parallel::For(0, aNbReads, [&](int theIndex) { + TopoDS_Shape aShape; + if (!ReadFromBuffer(aBrepData, aShape)) + { + aFailCount.fetch_add(1, std::memory_order_relaxed); + return; + } + aShapes(theIndex) = aShape; + }); + + EXPECT_EQ(aFailCount.load(), 0) << "Some parallel reads failed"; + + constexpr double aEps = 1.0e-6; + for (int i = 0; i < aNbReads; ++i) + { + ASSERT_FALSE(aShapes(i).IsNull()) << "Shape " << i << " is null"; + GProp_GProps aProps; + BRepGProp::VolumeProperties(aShapes(i), aProps); + const double aMass = std::abs(aProps.Mass()); + EXPECT_NEAR(aMass, aRefMass, aEps) + << "Shape " << i << " mass " << aMass << " != reference " << aRefMass; + } +} + +// Repeated parallel runs to catch intermittent failures. +TEST(BRepTools_Test, ParallelRead_RepeatedRuns) +{ + const std::string aBrepData = MakeBoxBrepString(); + + TopoDS_Shape aRefShape; + ASSERT_TRUE(ReadFromBuffer(aBrepData, aRefShape)); + GProp_GProps aRefProps; + BRepGProp::VolumeProperties(aRefShape, aRefProps); + const double aRefMass = std::abs(aRefProps.Mass()); + + constexpr int aNbReads = 100; + constexpr int aNbRuns = 5; + constexpr double aEps = 1.0e-6; + + for (int aRun = 0; aRun < aNbRuns; ++aRun) + { + NCollection_Array1 aShapes(0, aNbReads - 1); + std::atomic aFailCount{0}; + + OSD_Parallel::For(0, aNbReads, [&](int theIndex) { + TopoDS_Shape aShape; + if (!ReadFromBuffer(aBrepData, aShape)) + { + aFailCount.fetch_add(1, std::memory_order_relaxed); + return; + } + aShapes(theIndex) = aShape; + }); + + EXPECT_EQ(aFailCount.load(), 0) << "Run " << aRun << ": some reads failed"; + + for (int i = 0; i < aNbReads; ++i) + { + ASSERT_FALSE(aShapes(i).IsNull()) << "Run " << aRun << ", shape " << i << " is null"; + GProp_GProps aProps; + BRepGProp::VolumeProperties(aShapes(i), aProps); + const double aMass = std::abs(aProps.Mass()); + EXPECT_NEAR(aMass, aRefMass, aEps) << "Run " << aRun << ", shape " << i << " mass mismatch"; + } + } +} diff --git a/src/ModelingData/TKBRep/GTests/FILES.cmake b/src/ModelingData/TKBRep/GTests/FILES.cmake index 3471deb349..c7f485e56d 100644 --- a/src/ModelingData/TKBRep/GTests/FILES.cmake +++ b/src/ModelingData/TKBRep/GTests/FILES.cmake @@ -54,6 +54,7 @@ set(OCCT_TKBRep_GTests_FILES BRepGraph_SparseModel_Test.cxx BRepGraph_Deduplicate_Test.cxx BRepTools_ReShape_Test.cxx + BRepTools_Test.cxx TopExp_Test.cxx TopoDS_Builder_Test.cxx TopoDS_Edge_Test.cxx