Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iox-#141 enable sanizier(-fsanitize=address) #400

Merged

Conversation

prasannabhat
Copy link
Contributor

@prasannabhat prasannabhat commented Nov 30, 2020

Pre-Review Checklist for the PR Author

  1. Branch follows the naming format (iox-#123-this-is-a-branch)
  2. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  3. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  4. Relevant issues are linked
  5. Add sensible notes for the reviewer
  6. All checks have passed
  7. Assign PR to reviewer

Notes for Reviewer

What is done ?

  • Support AddressSanitizer (all the flags described here, including builtin leak detection)
  • Suppress known errors (blacklist files are stored under iceoryx_meta/sanitizer_blacklist)
  • Support Sanitizer github workflow (Linux & Mac)
    • Known errors are suppressed
    • workflow fails (exits upon first error) upon detection any new errors, if its pattern doesnt match blacklist

Which components are sanitized?

  • Sanitization is enabled for core components (iceoryx_posh & iceoryx_utils)
    However

Runtime interposition allows AddressSanitizer to find bugs in code that is not being recompiled

Next steps

  • Sanization be enabled for components other than iceoryx_posh & iceoryx_utils ?
  • Enable other sanitizers (e.g. ThreadSanitizer , MemorySanitizer)
  • Analyse and fix (if needed) the known issues

More details on the sanitizer can be found here ( Clang , Google)

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

  1. All checkboxes in the PR checklist are checked or crossed out
  2. Merge

References

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, you will make @elfenpiff quite happy with this

.github/workflows/sanitize.yml Outdated Show resolved Hide resolved
iceoryx_meta/CMakeLists.txt Outdated Show resolved Hide resolved
iceoryx_meta/sanitizer_blacklist.txt Outdated Show resolved Hide resolved
iceoryx_meta/CMakeLists.txt Outdated Show resolved Hide resolved
sanitize_test.sh Outdated Show resolved Hide resolved
@dkroenke dkroenke self-requested a review December 2, 2020 09:18
Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For resolving the memleaks in test_waitset.cpp do the following:
In Teardown extend the delete loop:

      for (auto s : m_subscriber)
        {
            delete s->m_portData;
            delete s;
        }

Add delete sut2; to line 115

sanitize_test.sh Outdated Show resolved Hide resolved
@prasannabhat
Copy link
Contributor Author

For resolving the memleaks in test_waitset.cpp do the following:
In Teardown extend the delete loop:

      for (auto s : m_subscriber)
        {
            delete s->m_portData;
            delete s;
        }

Add delete sut2; to line 115

Thanks for the hint @dkroenke.
Fixed this "low hanging fruit" :-)

- Separate compiler flags for lsan & asan
- Organise sanitizer blacklist files
- Support sanitizer for mac

Signed-off-by: Prasanna Bhat <[email protected]>
tools/run_all_tests.sh Show resolved Hide resolved
iceoryx_posh/CMakeLists.txt Outdated Show resolved Hide resolved
iceoryx_meta/sanitizer_blacklist.txt Outdated Show resolved Hide resolved
@prasannabhat prasannabhat requested a review from dkroenke December 7, 2020 04:53
@prasannabhat
Copy link
Contributor Author

nice, you will make @elfenpiff quite happy with this

happiness doubled @elfenpiff 😉 (supported sanitization on macos as well)

iceoryx_posh/CMakeLists.txt Outdated Show resolved Hide resolved
@dkroenke dkroenke self-requested a review December 11, 2020 08:30
Copy link
Member

@dkroenke dkroenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing left: there is one commit which has no iox tag in his name: 2183189

@prasannabhat prasannabhat force-pushed the iox-#141-enable-sanitizer branch from 6073702 to 917f8c2 Compare December 11, 2020 09:38
@prasannabhat
Copy link
Contributor Author

The only thing left: there is one commit which has no iox tag in his name: 2183189

oops ..that got missed !
@dkroenke I have created issue #423 to analyse the errors. (will add the details later)

@dkroenke dkroenke self-requested a review December 11, 2020 11:20
dkroenke
dkroenke previously approved these changes Dec 11, 2020
@dkroenke dkroenke added the tooling All iceoryx related tooling (scripts etc.) label Dec 11, 2020
Comment on lines +54 to +55
# NOTE : This works only when iceoryx is built standalone , in which case CMAKE_SOURCE_DIR point to iceoryx_meta
set(ICEORYX_SANITIZER_BLACKLIST -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/sanitizer_blacklist/asan_compile_time.txt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just putting the blacklist right to this file, then it works all the time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there might be a beside missing right of right

Copy link
Contributor Author

@prasannabhat prasannabhat Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just putting the blacklist right to this file, then it works all the time?

This is the method suggested to suppress errors

there might be a beside missing right of right

I dint quite get that !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, the comment was a bit confusing. I meant why not putting the blacklist files next to the IceoryxPlatform.cmake then it would also work when the components are build separately ... on the other side, then these files would need to be installed. Maybe it's better to just make the files obsolete is the short/mid term by fixing the issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay ..now I understand what you meant.
I am not sure how do I get the path to IceoryxPlatform.cmake (when this file is included in another cmake file)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another possibility would be to use file({WRITE | APPEND} <filename> <content>...) from cmake and create those files on the fly in e.g. CMAKE_BUILD_DIR. It's up to you if you want to do the hassle

iceoryx_utils/cmake/IceoryxPlatform.cmake Show resolved Hide resolved
iceoryx_posh/CMakeLists.txt Outdated Show resolved Hide resolved
.github/workflows/sanitize.yml Outdated Show resolved Hide resolved
Comment on lines +54 to +55
# NOTE : This works only when iceoryx is built standalone , in which case CMAKE_SOURCE_DIR point to iceoryx_meta
set(ICEORYX_SANITIZER_BLACKLIST -fsanitize-blacklist=${CMAKE_SOURCE_DIR}/sanitizer_blacklist/asan_compile_time.txt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another possibility would be to use file({WRITE | APPEND} <filename> <content>...) from cmake and create those files on the fly in e.g. CMAKE_BUILD_DIR. It's up to you if you want to do the hassle

@dkroenke dkroenke merged commit 4073ad0 into eclipse-iceoryx:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Sanitizer in Debug Build and Unit Tests
3 participants