-
Notifications
You must be signed in to change notification settings - Fork 377
Add Sanitizers documentation #4199
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
Conversation
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
| Modeling and applying sanitizers using settings | ||
| ----------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good, before this section, to add a bit of explanation about how sanitizers influence binary compatibility and then, as a result, explain how you can model it for these cases.
Also, it would be great to do some research about if all the sanitizers will make binaries to break compatibility or there are cases were you can apply the sanitizers just as flags without the risk of producing incompatible binaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an explanation about what each sanitizers does in the library/application and their abi compatibility as well. Please, review it again.
| .. code-block:: bash | ||
|
|
||
| git clone https://github.com/conan-io/examples2.git | ||
| cd examples2/examples/dev_flow/sanitizers/compiler_sanitizers | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d suggest restructuring the page so readers get the why/what before the how. Right now the doc asks to clone the repo in the middle of conceptual explanations, which interrupts the flow. It would be clearer to start with the important concepts and only then move to the hands-on example.
Depending on how extensive we want the theoretical part to be, we might even split it out into a separate page (so this page stays more tutorial-oriented, and the other more reference-style).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved to the end of the document:
Present everything about Sanitizers -> How to configure Sanitizer with Conan -> Example -> Final considerations.
|
|
||
| Sanitizers are powerful tools for detecting runtime bugs like buffer overflows, data races, memory leaks, | ||
| dangling pointers, use-of-uninitialized memory, and various types of undefined behavior. Compilers such as | ||
| GCC, Clang, and MSVC support these tools through specific compiler and linker flags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to talk about the difference in sanitizer support for each compiler, maybe do a comparative table or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be huge, considering the number of sanitizers supported by llvm. I pointed the sanitizers pages as a link in the documentation.
Let me add a table, in case it gets too big, we can still summarize to most important sanitizers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just for consideration, if you think that it would add value...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added some tables, indeed it is much clearer and visible now. Thank you for that suggestion! Please, take a look.
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
|
@czoido @memsharded Hello! Thank you for another round of review. I just addressed your comments in my last commits. You can take a quick preview of how the documentation is on the commit 74acf05 here:
|
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
memsharded
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some further minor comments
security/sanitizers.rst
Outdated
| @@ -0,0 +1,352 @@ | |||
| .. _security_sanitizers: | |||
|
|
|||
| Sanitizers | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For discoverability, search purposes, something like C, C++ compiler sanitizers or something in that line, a bit more complete, could be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Title updated by cd416bf
security/sanitizers.rst
Outdated
| .. code-block:: ini | ||
| :caption: compiler_sanitizers/profiles/asan | ||
|
|
||
| include(default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a bit confusing/not reproducible? Is this a gcc based or a clang profile?
Because the settings_user.yml above only added the sanitizer model to clang, not gcc, but the default profile in Linux systems is generally gcc.
Maybe it is better to fully explicitly define the profile? Or if not, maybe a comment saying the default profile is a clang one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by 0779b86
security/sanitizers.rst
Outdated
| .. code-block:: ini | ||
| :caption: ~/.conan/profiles/asan | ||
|
|
||
| include(default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, at least a note that this is compiler=msvc and not others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by 25403f6
security/sanitizers.rst
Outdated
| For Visual Studio (MSVC) we can obtain an equivalent profile for AddressSanitizer: | ||
|
|
||
| .. code-block:: ini | ||
| :caption: ~/.conan/profiles/asan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this profile is here ~/.conan/profiles/asan and the previous one is compiler_sanitizers/profiles/asan? That is a big confusing, also both are named the same which seems pretty conflicting, maybe one should be msvc_asan and the other clang_asan in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by commit d239adc
|
|
||
| Besides using Conan profiles to manage sanitizer settings, you can also use other approaches. | ||
|
|
||
| If you already have a :ref:`custom CMake toolchain file <conan_cmake_user_toolchain>` to manage compiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the section can be kept, but it would be necessary to at least clearly clarify that it only works when all dependencies are using CMake as build system and CMakeToolchain integration, it will not work for dependencies with Meson, autotools or others.
examples/security/sanitizers.rst
Outdated
| .. toctree:: | ||
| :maxdepth: 2 | ||
|
|
||
| sanitizers/compiler_sanitizers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this extra intermediate level adds little.
Is there any other potential or future nested level under sanitizers that is not compiler_sanitizers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to sanitizer folder by the commit d596bc2
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Added a note on the commit 763e8e3 to make it explicit |
|
Current preview based on the commit 763e8e3:
|
memsharded
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me, just a note about clang compiler profiles in Linux
examples/security/sanitizers.rst
Outdated
| arch=x86_64 | ||
| os=Linux | ||
| build_type=Debug | ||
| compiler=clang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this profile really uses clang in Linux? Isn't it necessary to at least define CC/CXX env-vars or compiler_executables confs?
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It's fine keeping gcc for these few examples, as it covers asan and ubsan.
I updated the settings_user as well on the commit 2decba9
examples/security/sanitizers.rst
Outdated
| arch=x86_64 | ||
| os=Linux | ||
| build_type=Debug | ||
| compiler=clang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this might use gcc compiler instead, the default one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated on the commit c5544ce
security/sanitizers.rst
Outdated
| arch=x86_64 | ||
| os=Linux | ||
| build_type=Debug | ||
| compiler=clang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated on the commit c5544ce4
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
Signed-off-by: Uilian Ries <uilianr@jfrog.com>
* Fix a typo (#4300) * Add Sanitizers documentation (#4199) * Add Sanitizers documentation Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add Sanitizers Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add more sections Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Sanitizer setting is optional Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add toolchain and hook section Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve description Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Remove unsed header. Co-authored-by: Carlos Zoido <mrgalleta@gmail.com> * Update sanitizers warning * Improve document description Co-authored-by: Carlos Zoido <mrgalleta@gmail.com> * Remove Sanitizers with Conan hooks section Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add Sanitizers documentation Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add Sanitizers Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add more sections Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Sanitizer setting is optional Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add toolchain and hook section Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve description Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Remove unsed header. Co-authored-by: Carlos Zoido <mrgalleta@gmail.com> * Update sanitizers warning * Improve document description Co-authored-by: Carlos Zoido <mrgalleta@gmail.com> * Remove Sanitizers with Conan hooks section Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add tables Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Fix warning message Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Reorganize reader flow 1 Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Reestructured page Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add ABI compatibility section Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Restore back sanitizers page Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add note about having multiple sanitizers in settings Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use Conan build command instead of install --build * Move sanitizers page to security folder Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Update folder name for examples Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Simplify index out of bounds commands Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Simplify signed integer overflow commands Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Be more consistent about mixing sanitizers Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve environment variables descriptioN Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Windows support Asan for both x86 and x64 Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Separare sanitizers examples Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve example title page Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve example title page Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Improve sanitizers title Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Make explicit profile Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Use explicit profiles Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Fix profile names Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Move sanitizer example to parent folder Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Update example link reference Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Explicit note about using cmake toolchain Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Change clang profiles to gcc Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Add GCC and MSVC to user_settings Signed-off-by: Uilian Ries <uilianr@jfrog.com> * Give back leak sanitizer to gcc Signed-off-by: Uilian Ries <uilianr@jfrog.com> --------- Signed-off-by: Uilian Ries <uilianr@jfrog.com> Co-authored-by: Carlos Zoido <mrgalleta@gmail.com> * Use words instead of unicode chars (#4304) Signed-off-by: Uilian Ries <uilianr@jfrog.com> --------- Signed-off-by: Uilian Ries <uilianr@jfrog.com> Co-authored-by: Lukas Müller <hello@lukasmu.com> Co-authored-by: Uilian Ries <uilianr@jfrog.com>




Related to conan-io/examples2#192
This is a new documentation page and spiritual successor of https://docs.conan.io/1/howtos/sanitizers.html
Also related to Conan issues:
Close conan-io/conan#19191