View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028217CommunityOCCT:Foundation Classespublic2016-12-13 19:352017-10-19 14:04
ReporterIstvan Csanady Assigned Toapn  
PrioritynormalSeveritycrash 
Status closedResolutionfixed 
Target Version7.2.0Fixed in Version7.2.0 
Summary0028217: Error handling is not thread safe and causing memory corruption and sporadic crashes
DescriptionStandard_Failure.cxx:

static Handle(Standard_Failure) RaisedError = NULL;

Should be

thread_local Handle(Standard_Failure) RaisedError = NULL;

Currently this global variable could be set from different threads, causing crashes and possible memory corruption.

Steps To Reproducetest bugs fclasses bug28217
TagsNo tags attached.
Test case numberbugs fclasses bug28217

Relationships

related to 0026937 closedapn Community Eliminate NO_CXX_EXCEPTION macro support 
parent of 0028439 closedapn Open CASCADE Configuration - compilation error when using thread_local within XCode 7 or earlier 
parent of 0029249 closedbugmaster Open CASCADE Configuration - Standard_Failure compilation fails on VS2013 + Intel Compiler due to unavailability of thread_local 

Activities

abv

2016-12-25 19:06

manager   ~0062091

The problem is that not all compilers supported by OCCT do support thread_local keyword. Its use requires VC++ >= 14 of GCC >= 4.8... For CLang, the situation is unclear -- formally it supports this keyword since version 3.3, but Apple variant of the compiler used in XCode is said to not support it (at least till the very recent versions, perhaps 8.0).

Istvan, can you indicate what is your compiler, and confirm that it really does support thread_local?

Istvan Csanady

2016-12-25 19:10

developer   ~0062092

We are using Xcode 8.2, and using the thread_local keyword works, and resolved the issues that were caused by this problem.

git

2016-12-25 21:35

administrator   ~0062095

Branch CR28217 has been created by abv.

SHA-1: 493caea78ed98f69b7c1e1784ef2dc0f49f1466a


Detailed log of new commits:

Author: abv
Date: Sun Dec 25 21:35:02 2016 +0300

    0028217: Error handling is not thread safe and causing memory corruption and sporadic crashes
    
    Static variable holding handle to the last raised exception is made thread-local on compilers that support C++11 keyword thread_local (MCVC++ 14+, GCC 4.8+, ICC 14+, CLang).

git

2016-12-26 09:29

administrator   ~0062099

Branch CR28217 has been updated forcibly by abv.

SHA-1: 1633e77ece69fcf1d07c8314e2efe1158b8c6f2a

abv

2016-12-26 09:31

manager   ~0062100

Fix (working only for VC++ 14 and above, and GCC 4.8+) is pushed to CR28217; please review.

kgv

2016-12-26 11:08

developer   ~0062109 

+// Define Standard_THREADLOCAL modifier as C++11 thread_local keyword
+// where it is available.
+#if (defined(__INTEL_COMPILER) && __INTEL_COMPILER > 1400) || \
+    (defined(__clang__)) /* assume standard CLang > 3.3 or XCode >= 8 */ || \
+    (defined(_MSC_VER) && _MSC_VER >= 1800) /* MSVC++ >= 14 */ || \
+    (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8))) /* GCC >= 4.8 */
+  #define Standard_THREADLOCAL thread_local
+#else
+  #define Standard_THREADLOCAL
+#endif

Why this macros has been buried inside Standard_Failure.cxx, while it might be potentially useful in other places, and we place such attributes in Standard_Macro.hxx?

abv

2016-12-27 08:58

manager   ~0062151

This macro has quite limited application: only the most modern compilers support thread_local variable. I see no other places where this macro could have been useful, hence kept it defined locally. The fix is not really universal but a kind of work-around to improve behavior for modern compilers only; the right fix will be to get rid of use of this static variable.

git

2016-12-28 10:46

administrator   ~0062217

Branch CR28217 has been updated by apv.

SHA-1: 83a7db8847fc584049e03868c1cb25361fa55415


Detailed log of new commits:

Author: apv
Date: Wed Dec 28 10:45:35 2016 +0300

    Test case tuning bugs/fclasses/bug28217

apv

2016-12-28 11:23

tester   ~0062220

Dear BugMaster,

Branch CR28217 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: 1633e77ece69fcf1d07c8314e2efe1158b8c6f2a

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 63
   Windows: 0
   MacOS: 1127

Regressions/Differences:
Not detected

Testing cases:
bugs fclasses bug28217 - BAD (known problem)
http://occt-tests/CR28217-master-OCCT/Debian70-64/bugs/fclasses/bug28217.html
http://occt-tests/CR28217-master-OCCT/Windows-64-VC10/bugs/fclasses/bug28217.html

Testing on Linux:
Total MEMORY difference: 93289164 / 93045435 [+0.26%]
Total CPU difference: 21530.310000000223 / 21520.74000000037 [+0.04%]

Testing on Windows:
Total MEMORY difference: 58627848 / 58650770 [-0.04%]
Total CPU difference: 19406.0875971985 / 19731.318481998722 [-1.65%]

git

2017-02-08 11:32

administrator   ~0063561

Branch CR28217 has been deleted by kgv.

SHA-1: 83a7db8847fc584049e03868c1cb25361fa55415

Related Changesets

occt: master b3d20c7f

2016-12-29 11:37:42

abv


Committer: apn Details Diff		 0028217: Error handling is not thread safe and causing memory corruption and sporadic crashes

Static variable holding handle to the last raised exception is made thread-local on compilers that support C++11 keyword thread_local (MCVC++ 14+, GCC 4.8+, ICC 14+, CLang).
Test bugs fclasses bug28217 is added (BAD on vc < 14 and gcc < 4.8).

Test case tuning bugs/fclasses/bug28217		 Affected Issues
0028217
mod - src/QABugs/QABugs_20.cxx Diff File
mod - src/Standard/Standard_Failure.cxx Diff File
add - tests/bugs/fclasses/bug28217 Diff File

Issue History

Date Modified Username Field Change
2016-12-13 19:35 Istvan Csanady New Issue
2016-12-13 19:35 Istvan Csanady Assigned To => abv
2016-12-25 01:31 abv Relationship added related to 0026937
2016-12-25 19:06 abv Note Added: 0062091
2016-12-25 19:10 Istvan Csanady Note Added: 0062092
2016-12-25 21:35 git Note Added: 0062095
2016-12-26 09:29 git Note Added: 0062099
2016-12-26 09:31 abv Note Added: 0062100
2016-12-26 09:31 abv Status new => resolved
2016-12-26 09:31 abv Steps to Reproduce Updated
2016-12-26 09:31 abv Assigned To abv => kgv
2016-12-26 11:08 kgv Note Added: 0062109
2016-12-26 19:03 kgv Assigned To kgv => abv
2016-12-26 19:03 kgv Status resolved => assigned
2016-12-27 08:58 abv Note Added: 0062151
2016-12-27 08:59 abv Status assigned => resolved
2016-12-27 08:59 abv Assigned To abv => kgv
2016-12-27 09:13 kgv Assigned To kgv => bugmaster
2016-12-27 09:13 kgv Status resolved => reviewed
2016-12-27 14:18 apv Assigned To bugmaster => apv
2016-12-27 14:18 apv Test case number => bugs fclasses bug28217
2016-12-28 10:46 git Note Added: 0062217
2016-12-28 11:23 apv Note Added: 0062220
2016-12-28 11:23 apv Assigned To apv => bugmaster
2016-12-28 11:23 apv Status reviewed => tested
2016-12-30 18:08 apn Changeset attached => occt master b3d20c7f
2016-12-30 18:08 apn Assigned To bugmaster => apn
2016-12-30 18:08 apn Status tested => verified
2016-12-30 18:08 apn Resolution open => fixed
2017-02-03 19:59 kgv Relationship added parent of 0028439
2017-02-08 11:32 git Note Added: 0063561
2017-09-29 16:21 aiv Fixed in Version => 7.2.0
2017-09-29 16:31 aiv Status verified => closed
2017-10-19 14:04 kgv Relationship added parent of 0029249