View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0030557||Open CASCADE||OCCT:Coding||public||2019-03-12 04:33||2021-08-24 14:59|
|Summary||0030557: Coding - eliminate errors reported by -fsanitize|
|Description||Compile OCCT 7.3.0 with -fsanitize=undefined, -fsanitize=address and run the test suite.|
|Tags||No tags attached.|
|Test case number|
|parent of||0030556||closed||apn||Community||Coding - IGESData_GlobalSection missing member initialization in default constructor|
|parent of||0030553||closed||apn||Community||Coding - TopOpeBRepDS_Surface missing default initalizations|
|parent of||0030554||closed||apn||Community||Coding - ChFiDS_CommonPoint uninitialized member traarc|
|parent of||0030552||feedback||abv||Community||Foundation Classes - Stack overflow due to math_SingleTab static array size|
|parent of||0030551||closed||bugmaster||Community||Foundation Classes - Integer overflow in NCollection_CellFilter HashCode|
|parent of||0030562||closed||apn||Community||Coding - TopOpeBRepBuild_Builder use of null pointer|
|parent of||0030564||closed||apn||Community||Coding - math_Gauss uninitialized 'Singular' member variable|
|parent of||0030565||closed||bugmaster||Community||Coding - Approx_SweepApproximation call on null pointer|
|parent of||0030518||closed||bugmaster||Community||Foundation Classes - NCollection_IndexedDataMap array out of bounds|
|parent of||0030550||closed||bugmaster||Community||Coding - Integer overflow in Standard_CString HashCodes|
|parent of||0030603||closed||bugmaster||Community||Memory Leaks in Geom_BSplineCurve|
|parent of||0030563||closed||abv||Community||Coding - Standard_Type vptr error|
|parent of||0030978||closed||bugmaster||Open CASCADE||Visualization - stack-use-after-scope reported by Clang address sanitizer in OpenGl_Text.cxx|
|parent of||0030980||closed||bugmaster||Open CASCADE||Data Exchange - global-buffer-overflow reported by Clang address sanitizer in iges_newchar()|
|parent of||0030981||closed||bugmaster||Open CASCADE||Foundation Classes - heap-buffer-overflow reported by Clang address sanitizer in TCollection_ExtendedString|
|parent of||0030985||closed||bugmaster||Open CASCADE||Modeling Algorithms - heap-use-after-free reported by Clang address sanitizer in TopOpeBRepTool_REGUW::InitBlock()|
|parent of||0030986||new||msv||Open CASCADE||Modeling Algorithms - heap-buffer-overflow reported by Clang address sanitizer in HLRBRep_Data.cxx|
|parent of||0030989||new||kgv||Open CASCADE||Visualization - heap-use-after-free reported by Clang address sanitizer in OpenGl_Structure::IsRaytracable()|
|parent of||0030516||closed||bugmaster||Community||Visualization - Pointer to an OpenGl_Structure is deleted and accessed later after PrsMgr_Presentation::Highlight()|
|parent of||0030992||closed||abv||Open CASCADE||Foundation Classes - heap-buffer-overflow reported by Clang address sanitizer in BSplCLib::BuildKnots()|
|parent of||0030993||closed||abv||Open CASCADE||Modeling Algorithms - heap-use-after-free reported by Clang address sanitizer in BRepFeat_MakeRevolutionForm::Perform()|
|parent of||0031008||closed||abv||Open CASCADE||Application Framework - memcpy-param-overlap reported by Clang address sanitizer in LDOM_XmlReader::ReadRecord()|
|parent of||0031009||verified||azv||Open CASCADE||Modeling Algorithms - alloc-dealloc-mismatch reported by Clang address sanitizer in IntCurvesFace_ShapeIntersector|
|parent of||0031010||closed||abv||Open CASCADE||Foundation Classes - heap-buffer-overflow reported by Clang address sanitizer in OSD_Path::IsUncExtendedPath()|
|parent of||0031024||closed||abv||Community||Coding - invalid left shift in BVH_RadixSorter::Perform() using -fsanitize=undefined|
|parent of||0031034||closed||abv||Open CASCADE||Visualization - stack-use-after-scope reported by Clang address sanitizer in AIS_FixRelation::Compute()|
|parent of||0031038||closed||bugmaster||Open CASCADE||Draw - adaptations for running tests with CLang address sanitizer|
|parent of||0031048||closed||abv||Open CASCADE||Visualization - runtime error reported by Clang undefined behavior sanitizer in Image_AlienPixMap::Save()|
|Not all the children of this issue are yet resolved or closed.|
||Issues 0030552 and 0030551 should also be added here.|
||Since -fsanitize=address was added, issue 0030518 should also be a child here.|
To use address sanitizers on my computer (Ubuntu 16.04) with CLang 6.0 I had to do the following:
1. In CMake configuration:
- set CMAKE_CXX_COMPILER to clang++-6.0 and CMAKE_C_COMPILER to clang-6.0; when using CMake GUI, this is done by choice of oprion "Specify native compilers" when you configure for the first time
- set CMAKE_LINKER to clang++-6.0; this is important to avoid linking problems
- set CMAKE_CXX_FLAGS and CMAKE_C_FLAGS to "-fsanitize=address -fno-omit-frame-pointer -fno-optimize-sibling-calls"
- set CMAKE_BUILD_TYPE to RelWithDebInfo; this is in the hope to get more informative messages
2. Build as usual (make), but times slower and more disk space consuming
3. Before running executable, make sure that "llvm-symbolizer" is in PATH; this is necessary to get human-readable stack traces. The tool must have exaclty that name. I did not had it installed with that name, so had to create a symlink with:
> sudo ln -s /usr/bin/llvm-symbolizer-6.0 /usr/bin/llvm-symbolizer
4. Set environment variable to disable memory leaks detection (they seem to be reported for every global variable, like MFC used to do in the past):
> export ASAN_OPTIONS=detect_leaks=0
5. Run DRAW (./draw.sh relwithdeb) and enjoy
So far I have been able to see the address sanitizer reporting an out-of-bound access in TCollection_ExtendedString (a problem known for long time) in XDE.
Another sanitizer I tried was "undefined", its reports are difficult to understand (perhaps will try more).
Sanitizer "memcheck" is mostly useless as it requires all libraries used by the executable to be instrumented, this requires enormous effort and time.
All this is extremely slow and takes a lot of time to build (every change is a rebuild).
From this experience I conclude that use of Valgrind is much easier and practical.
Use of compiler sanitizers requires a lot of effort (you have to build a whole environment for each sanitizer) and is worth only if there is a real strong need in making code 100% clean.
I have a few comments.
1. Did you also try the gnu compiler? g++ as of 4.9 also has the sanitizer. There is a bug in Ubuntu that requires you to use the flag -fuse-ld=gold as a link flag.
2. Using the sanitizer does make the build slower and it does take up more disk space.
3. I don't seem to need the llvm-symbolizer when using the gnu compiler.
4. The address sanitizer will report all memory that is not cleaned up before the code exits. valgrind will do the same if you use the flag --show-leak-kinds=all. You completely disabled all memory leak detection with export ASAN_OPTIONS=detect_leaks=0. I would recommend you instead suppress individual known problems with a suppression file: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions
valgrind is only able to look for array out of bounds on heap allocated memory. The address sanitizer can also detect array out of bound errors on statically sized arrays on the stack. Bugs relate to the latter are extremely difficult to find otherwise. However, the address sanitizer cannot detect use of uninitialized data. So for our testing we use both valgrind and the sanitizer to detect the different types of errors.
While the compile time is longer with the sanitizer, the runtime of the test suite is significantly faster. Our set of tests takes about 3 hours to run with the address sanitizer, and the same suite of tests takes about 3 days to run with valgrind. We can run the sanitizer nightly to look for errors, but we only run valgrind once a month.
You do not need to instrument all libraries used by the executable, but you might need to add suppressions for libraries that you do not instrument. If the primary executable isn't instrumented you need to have
(the last number on the shared library will depend on the compiler version).
5. I would encourage you to look closely at the undefined errors. They take a little getting used to deciphering, but are quite informative once you understand them.
6. I would hope that you are striving towards 100% clean code. I will continue reporting errors from the sanitizer as they show up with our testing.
Please let me know if there is anything I can help with.
Thank you for comments!
Your points are correct indeed, and sure we do aim at having 100% clean code... Yet systematic usage of tools like sanitizers is behind our current capacities due to amount of effort and resources needed for that.
Besides, I have been able to run OCCT tests with address sanitizers, and so far (1/5 passed during the night) only one error has been reported, thus now its usage looks more feasible.
Regarding errors reported by "undefined" sanitizer: I agree that errors like uninitialized fields are very important, but static code analyzers should be able to report them as well, and they should be much easier to use. The errors I have seen trying to run with "undefined" sanitizer looked more like in 0030563, and so far I could not understand what they mean. I will be grateful to you if you could explain the report in 0030563 - this may help us to understand other similar reports.
I appreciate that OCC is a very large code base, and I have seen a lot of improvements over the past couple revisions.
What static analyzers do you use? I'm using the clang analyzer at the moment and it catches some things, but not all the same things running with the undefined sanitizer catches.
The report 0030563 is probably the most cryptic message I have gotten so far from the undefined sanitizer, and it has me a little stumped as well. Part of the issue here is I don't completely understand the process of how aRegistry is used in OCC. The one clue is that I only get this report with running anything that does STEP file IO. I suspect there is some invalid casting that is going on in the STEP IO that the sanitizer is only catching when everything is getting cleaned up. I'm wondering if something was incorrectly cast to an NCollection_DataMap. It's either that, or there is some mismatch in the tracking of how many things are in the registry, which is why my kludge suppressed the problem.
Do you have any other undefined messages I could take a stab at?
For the static code analysis of OCCT code we (including some external contributors) have used (from what I know) CAST, cppcheck, PVS Studio, Visual Studio Code Analyser. All of them do their job and a few issues have been identified and solved, however we do not use them in a systematic way (i.e. in an automated build environment), due to a lots of false positives that need to be sorted out and filtered. This task is still pending.
I still have tests with address sanitizer running on my computer, apparently it will take a few days more to complete. Thus further experiments with undefined sanitizer are postponed, and I have no other messages to investigate.
I shall coment separately in 0030563 on the usage of registry of types in OCCT.
Yeah we also use cppcheck, and yes the false positives are a pain. It does not help that things change with versions of the static analyzers as well. We do our best to deal with them though.
Feel free to ask me if anything else pops up with the address or undefined sanitizers.
I will be running our suite of tests when the next version of OCCT comes out and I'll report anything that pops up (hopefully you have already got them all though!).
||Finally I have been able to run the whole OCCT test suite under Address sanitizer, all the issues have been reported as children of this one. The total time of execution was about 8 days (on Ubuntu 16.04 running under Virtual Box VM on Windows 10 computer with i5-3590 CPU, in one thread).|
||Just out of curiosity, how long does the test suite toke to run without the sanitizer on Ubuntu 16.04 on the virtual box?|
|2019-03-12 04:33||kgv||New Issue|
|2019-03-12 04:33||kgv||Assigned To||=> kgv|
|2019-03-12 04:33||kgv||Relationship added||parent of 0030556|
|2019-03-12 04:34||kgv||Relationship added||parent of 0030553|
|2019-03-12 04:34||kgv||Relationship added||parent of 0030554|
|2019-03-12 04:42||galbramc||Note Added: 0082854|
|2019-03-12 05:00||kgv||Relationship added||parent of 0030552|
|2019-03-12 05:00||kgv||Summary||Coding - eliminate errors reported by -fsanitize=undefined => Coding - eliminate errors reported by -fsanitize|
|2019-03-12 05:00||kgv||Description Updated|
|2019-03-12 14:05||kgv||Relationship added||parent of 0030551|
|2019-03-12 19:47||kgv||Relationship added||parent of 0030562|
|2019-03-13 07:43||kgv||Relationship added||parent of 0030564|
|2019-03-13 07:43||kgv||Relationship added||parent of 0030565|
|2019-03-13 13:53||galbramc||Note Added: 0082909|
|2019-03-13 13:54||kgv||Relationship added||parent of 0030518|
|2019-03-20 08:40||kgv||Relationship added||parent of 0030550|
|2019-03-25 14:09||kgv||Relationship added||parent of 0030603|
|2019-03-25 14:20||kgv||Relationship added||parent of 0030563|
|2019-09-17 08:22||abv||Note Added: 0087181|
|2019-09-17 13:55||galbramc||Note Added: 0087201|
|2019-09-18 03:38||abv||Relationship added||parent of 0030978|
|2019-09-18 08:21||abv||Note Added: 0087240|
|2019-09-18 08:22||abv||Target Version||=> 7.5.0|
|2019-09-18 15:50||galbramc||Note Added: 0087259|
|2019-09-19 05:59||abv||Note Added: 0087282|
|2019-09-19 07:15||abv||Relationship added||parent of 0030980|
|2019-09-19 08:34||abv||Relationship added||parent of 0030981|
|2019-09-19 23:35||galbramc||Note Added: 0087333|
|2019-09-20 08:09||abv||Relationship added||parent of 0030985|
|2019-09-20 08:09||abv||Relationship added||parent of 0030986|
|2019-09-21 08:04||abv||Relationship added||parent of 0030989|
|2019-09-21 10:04||kgv||Relationship added||parent of 0030516|
|2019-09-24 08:28||abv||Relationship added||parent of 0030992|
|2019-09-24 08:32||abv||Relationship added||parent of 0030993|
|2019-09-28 09:13||abv||Relationship added||parent of 0031008|
|2019-09-28 09:27||abv||Relationship added||parent of 0031009|
|2019-09-28 09:37||abv||Relationship added||parent of 0031010|
|2019-10-03 06:25||kgv||Relationship added||parent of 0031024|
|2019-10-04 20:35||abv||Relationship added||parent of 0031034|
|2019-10-06 09:33||abv||Relationship added||parent of 0031038|
|2019-10-08 23:01||abv||Relationship added||parent of 0031048|
|2019-10-09 23:16||abv||Note Added: 0087995|
|2019-10-09 23:24||galbramc||Note Added: 0087996|
|2020-09-11 15:34||utverdov||Target Version||7.5.0 => 7.6.0|
|2021-08-24 14:59||kgv||Target Version||7.6.0 => 7.7.0|