-
Notifications
You must be signed in to change notification settings - Fork 8
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
[#390] Add UBSAN #369
[#390] Add UBSAN #369
Conversation
There are more options that can be used (e.g., for unsigned integer overflow and implicit conversions), but they were left out as the logs became very noisy. Docs for clang 13: https://releases.llvm.org/13.0.0/tools/clang/docs/UndefinedBehaviorSanitizer.html |
Is noisy... bad? Are they real UB? Isn't that what we want to see? All the noise, aka 'signal'? |
Yes. If it found things, we need to open issues for them and address them. We should review the results together first though. |
Here's what I have saved. Context for the first part, running non OIDC tests. Context for second part, stopping a server with ctrl^c after a successful start. $ cat /tmp/run.txt.4174
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:70:19: runtime error: unsigned integer overflow: 1701012321 * 5 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:70:19 in
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:62:20: runtime error: unsigned integer overflow: 1701012321 * 5 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:62:20 in
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:62:24: runtime error: unsigned integer overflow: 4210094309 + 1630368880 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/http/impl/field.ipp:62:24 in
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/core/impl/string.ipp:33:12: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/core/impl/string.ipp:33:12 in
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/core/detail/string.hpp:37:39: runtime error: unsigned integer overflow: 45 - 65 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/beast/core/detail/string.hpp:37:39 in
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/asio/detail/impl/signal_set_service.ipp:142:12: runtime error: implicit conversion from type 'ssize_t' (aka 'long') of value -1 (64-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/asio/detail/impl/signal_set_service.ipp:142:12 in
$ cat /tmp/run.txt.4368
/opt/irods-externals/boost-libcxx1.81.0-1/include/boost/asio/detail/impl/signal_set_service.ipp:142:12: runtime error: implicit conversion from type 'ssize_t' (aka 'long') of value -1 (64-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/irods-externals/boost-libcxx1.81.0-1/include/boost/asio/detail/impl/signal_set_service.ipp:142:12 in |
Looks like it's complaining about Boost.Beast, not our code. Can it be configured to ignore Boost.Beast code? |
Took a look at the listed files that have unsigned overflow. They all seem to be intentionally designed to do so. Just so it's not lost, following is an example of one unsigned integer overflow, using trap to produce the following core dump:
|
It does seem we can create an ignore file to not sanitize certain things: https://releases.llvm.org/13.0.1/tools/clang/docs/SanitizerSpecialCaseList.html I'll try using it shortly. This can be used for asan too. Not sure if that might be of interest for you @korydraughn, since I think you've used it the most. |
I'm always open to learning. Make it good for everyone. |
2b066b4
to
49aecb2
Compare
Added more checks. Of note:
|
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.
Excellent.
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 like us to eventually move away from having all of this stuff in our CMake, but I don't think we're quite ready for a workflow that leans on setting a bunch of stuff via arguments to cmake.
Aside from Kory's comments and my ignorelist suggestions, this looks good to me.
That sounds like a push towards the cmake presets feature. I like the idea of a smaller CMake. Demonstrate and prove to the team this is the way. |
What's the status of this PR? Seems we're very close. |
Need to address open comments is all, I think. |
Good. Let's get this merged today as well. |
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.
Looks alright to me. I defer.
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.
Squash it.
Please open an issue to add ubsan to irods/irods.
Created irods/irods#8090. |
d97ddc8
to
fa7afb2
Compare
Squashed. |
Slightly tweaked commit message. Top line:
Commit message body: |
fa7afb2
to
1d5b897
Compare
Message tweaked. |
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.
Pound it.
This commit adds UBSAN to the CMake options while also providing defaults for UBSAN and an ignore list.
1d5b897
to
26bb02f
Compare
Pounded. |
Add UBSan to CMake options.