Specify sanitizers using IGN_SANITIZERS cmake variable #210

j-rivero · 2022-03-11T18:20:55Z

🎉 New feature

Part of gazebo-tooling/release-tools#404

Summary

The PR integrates the sanitizers compilers flags into ignition-cmake consumers to facilitate how to run them. It adds a CMake option of IGN_SANITIZERS that can be populated with different kind of sanitizers.

The work mostly integrated the code from https://github.com/StableCoder/cmake-scripts#sanitizer-builds-sanitizerscmake.

I added a test to run against the FAKE_INSTALL installation that checks that the compiler flag is being added to the compiler when using the CMake parameter.

Test it

It can be used directly without colcon by passing -DIGN_SANITIZERS=Address or using colcon together with for example ignition-transport11:

colcon build --merge-install --executor sequential --cmake-args -DIGN_SANITIZER=Address -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON --event-handlers console_direct+
colcon test --merge-install

Some tests are failing due to problems with the sanitizer:

33: [ RUN      ] ignTest.ServiceRequest                                                                                                                                                                                           
33: /home/jrivero/code/ignition/ws_asan/src/ign-transport/src/cmd/ign_TEST.cc:390: Failure                                                                                                                                        
33: Expected equality of these values:                                                                                                                                                                                            
33:   output                                                                                                                                                                                                                      
33:     Which is: "Library error: libignition-tools-backward.so not found. Improved backtrace generation will be disabled\ndata: 10\n\n\n=================================================================\n==109340==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 64 byte(s
33:   "data: " + value + "\n\n"                                                                                                                                                                                                   
33:     Which is: "data: 10\n\n"                                                                                                                                                                                                  
33: With diff:                                                                                                                                                                                                                    
33: @@ -1,20 +1,2 @@                                                                                                                                                                                                              
33: -Library error: libignition-tools-backward.so not found. Improved backtrace generation will be disabled                                                                                                                       
33:  data: 10                                                                                                                                                                                                                     
33: -                                                                                                                                                                                                                             
33: -                                                                                                                                                                                                                             
33: -=================================================================                                                                                                                                                            
33: -==109340==ERROR: LeakSanitizer: detected memory leaks                                                                                                                                                                        
33: -                                                                                                                                                                                                                             
33: -Direct leak of 64 byte(s) in 2 object(s) allocated from:                                                                                                                                                                     
33: -    #0 0x7f632490a587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104                                                                                                                
33: -    #1 0x7f63243a7ee9 in ignition::msgs::Int32* google::protobuf::Arena::CreateMaybeMessage<ignition::msgs::Int32>(google::protobuf::Arena*) (/lib/x86_64-linux-gnu/libignition-msgs8.so.8+0x226ee9)                         
33: -    #2 0x5591192b87b6 in cmdServiceReq /home/jrivero/code/ignition/ws_asan/src/ign-transport/src/cmd/ign.cc:240                                                                                                              
33: -    #3 0x5591191db204 in std::function<void ()>::operator()() const /usr/include/c++/9/bits/std_function.h:688                                                                                                               
33: -    #4 0x5591191db204 in CLI::App::run_callback(bool) /usr/include/ignition/utils1/ignition/utils/cli/App.hpp:1918                                                                                                           
33: -    #5 0x5591191db204 in CLI::App::run_callback(bool) /usr/include/ignition/utils1/ignition/utils/cli/App.hpp:1898                                                                                                           
33: -    #6 0x55911929435e in CLI::App::parse(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&&) /usr/include/ignition/utils1/ignition/utils/
33: -    #7 0x55911929435e in CLI::App::parse(int, char const* const*) /usr/include/ignition/utils1/ignition/utils/cli/App.hpp:1233                                                                                               
33: -    #8 0x5591191c6eeb in main /home/jrivero/code/ignition/ws_asan/src/ign-transport/src/cmd/service_main.cc:134                                                                                                              
33: -    #9 0x7f63237730b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2)                                                                                                                                         
33: -                                                                                                                                                                                                                             
33: -SUMMARY: AddressSanitizer: 64 byte(s) leaked in 2 allocation(s).\n                                                                                                                                                           
33: +\n                                                                                                                                                                                                                           
33:·                                                                                                                                                                                                                              
33: [  FAILED  ] ignTest.ServiceRequest (494 ms)

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial: I might need some feedback from @chapulina about where is the best place to host the tutorial. In this same repository under examples? I place it under tutorials c1bc190
Updated documentation (as needed) I also welcome feedback about where to place a copy of https://github.com/StableCoder/cmake-scripts#sanitizer-builds-sanitizerscmake that is great. c1bc190
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

azeey

Looks great! Just a couple minor comments. I tried it on sdformat and it worked great except the tests that use ign which gave the error:

ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

I think this is because the libraries are loaded through ruby when using ign.

When setting up CI, we'll have to find a way to ignore tests that run ign.

cmake/IgnSanitizers.cmake

azeey · 2022-03-15T00:11:55Z

cmake/IgnSanitizers.cmake

+        endif()
+        append("${SANITIZER_MEM_FLAG}" SANITIZER_SELECTED_FLAGS)
+
+        if(AFL)


AFL is not being used here, so we can remove this if block. Here and elsewhere below.

True. I'm in favor of leave it in place just in the case that someone (or even us in the future) is using that support now that we have it implemented.

Sure. I'm okay with that, but I haven't tested it. Not sure if you've had a chance.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero · 2022-03-29T15:03:07Z

I think this is because the libraries are loaded through ruby when using ign.
When setting up CI, we'll have to find a way to ignore tests that run ign.

Correct, we would need to exclude them somehow when executing the test suite with the sanitizers. Not sure right now that we can do anything for them in this ign-cmake support.

j-rivero · 2022-03-29T15:04:23Z

Made a little change from option to set in f169762. option is designed to work mostly for ON/OFF, using it for a string support can deal to unexpected behaviors.

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero · 2022-03-29T16:49:24Z

Found the place to update the documentation and place a copy of the original docs about how to use the new parameter c1bc190

azeey

Looking good. I think we should clarify which CMake variable to use after the latest changes. Also Windows CI is failing.

azeey · 2022-03-29T18:39:41Z

cmake/IgnSanitizers.cmake

+        endif()
+        append("${SANITIZER_MEM_FLAG}" SANITIZER_SELECTED_FLAGS)
+
+        if(AFL)


Sure. I'm okay with that, but I haven't tested it. Not sure if you've had a chance.

azeey · 2022-03-29T18:40:47Z

tutorials/ign_cmake_sanitizers.md

+- [MemorySanitizer](https://clang.llvm.org/docs/MemorySanitizer.html) detects uninitialized reads.
+- [Control Flow Integrity](https://clang.llvm.org/docs/ControlFlowIntegrity.html) is designed to detect certain forms of undefined behaviour that can potentially allow attackers to subvert the program's control flow.
+
+These are used by declaring the `IGN_USE_SANITIZER` CMake variable as string containing any of:


Not sure what the correct CMake variable is: I see USE_SANITIZER and IGN_SANITIZER in the cmake code, but not IGN_USE_SANITIZER.

examples/CMakeLists.txt

cmake/IgnSanitizers.cmake

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero · 2022-03-29T22:25:03Z

Looking good. I think we should clarify which CMake variable to use after the latest changes. Also Windows CI is failing.

ok, it is not a good to make changes without caffeine. Sorry for the mess, IGN_SANITIZER is the right one, I think I fixed all the occurrences in 86f2091

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

azeey

LGTM!

azeey · 2022-03-30T19:49:06Z

tutorials/developing_with_ign-cmake.md

-
-Note: Address sanitizer may have an impact on the performance of execution.
+`IGN_SANITIZER` CMake parameter can be used with different compilers to support the detection of different problems in the code.
+[Check the documentation for `IGN_SANITIZER` flag](ign_cmake_sanitizers.md)


We need to add ign_cmake_sanitizers.md to tutorials.md.in

The problem we have is that the ign_cmake_sanitizers is not really a tutorial but more a documentation on how to use the feature. Not sure if we can consider it as a "tutorial". It is referenced from the "Developing with Ignition CMake" and place inside the tutorials since I did not find a better place for it.

We've been using the term "tutorial" to refer to all kinds of documentation, guides, instructions, etc. So I wouldn't worry too much about it. It's good to give the document more visibility by placing it on the main page.

In any case, note that we're not publishing tutorials for ign-cmake on the main site yet:

Missing libs on docs page docs#53

thanks @azeey and @chapulina for the information. I added the entry in c55d115

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Support for running compiler sanitizers Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org> Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org> Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>

osrf-triage · 2022-04-27T15:06:00Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-releases-2022-04-27-fortress-citadel/1389/1

j-rivero added 2 commits March 11, 2022 17:35

Support for running compiler sanitizers

e6eea98

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Fix problems with linters

c80c29c

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero requested review from mxgrey and adlarkin as code owners March 11, 2022 18:20

github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels Mar 11, 2022

chapulina requested a review from azeey March 14, 2022 18:38

azeey approved these changes Mar 15, 2022

View reviewed changes

j-rivero added 2 commits March 29, 2022 16:58

Do not mess with CMAKE_BUILD_TYPE

443e015

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Prefer to use set when dealing with strings

f169762

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Include documentation

c1bc190

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

azeey reviewed Mar 29, 2022

View reviewed changes

Fix variable name: IGN_SANITIZER

86f2091

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero mentioned this pull request Mar 29, 2022

ASAN CI builds for ignition and sdformat gazebo-tooling/release-tools#678

Merged

Restrict sanitizer test to UNIX

05e5e6d

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

scpeters changed the title ~~Ign sanitizers~~ Specify sanitizers using IGN_SANITIZERS cmake variable Mar 30, 2022

scpeters mentioned this pull request Mar 30, 2022

Fix unsafe globals in Interval and Region3 gazebosim/gz-math#396

Merged

6 tasks

azeey approved these changes Mar 30, 2022

View reviewed changes

j-rivero and others added 2 commits March 31, 2022 14:19

Include an entry in the tutorial page

c55d115

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Merge branch 'ign-cmake2' into ign_sanitizers

6bb8683

j-rivero merged commit ca66305 into ign-cmake2 Apr 1, 2022

j-rivero deleted the ign_sanitizers branch April 1, 2022 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify sanitizers using IGN_SANITIZERS cmake variable #210

Specify sanitizers using IGN_SANITIZERS cmake variable #210

j-rivero commented Mar 11, 2022 •

edited

Loading

azeey left a comment

azeey Mar 15, 2022

j-rivero Mar 29, 2022

azeey Mar 29, 2022

j-rivero commented Mar 29, 2022

j-rivero commented Mar 29, 2022

j-rivero commented Mar 29, 2022

azeey left a comment

azeey Mar 29, 2022

azeey Mar 29, 2022

j-rivero commented Mar 29, 2022

azeey left a comment

azeey Mar 30, 2022

j-rivero Mar 30, 2022

chapulina Mar 30, 2022

j-rivero Mar 31, 2022

osrf-triage commented Apr 27, 2022

Specify sanitizers using IGN_SANITIZERS cmake variable #210

Specify sanitizers using IGN_SANITIZERS cmake variable #210

Conversation

j-rivero commented Mar 11, 2022 • edited Loading

🎉 New feature

Summary

Test it

Checklist

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-rivero commented Mar 29, 2022

j-rivero commented Mar 29, 2022

j-rivero commented Mar 29, 2022

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-rivero commented Mar 29, 2022

azeey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osrf-triage commented Apr 27, 2022

j-rivero commented Mar 11, 2022 •

edited

Loading