Fix several cases of undefined behaviour, memory corruption and memory leaks #1060
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arogge
force-pushed
the
dev/arogge/master/fix-ub
branch
5 times, most recently
from
2e374eb
to
8d5c3e6
Compare
arogge
changed the title
16 tasks
arogge
force-pushed
the
dev/arogge/master/fix-ub
branch
3 times, most recently
from
65abc28
to
222a46d
Compare
alaaeddineelamri
force-pushed
the
dev/arogge/master/fix-ub
branch
from
222a46d
to
428c79b
Compare
alaaeddineelamri
approved these changes
Feb 22, 2022
arogge
force-pushed
the
dev/arogge/master/fix-ub
branch
2 times, most recently
from
b66c94a
to
b655f2a
Compare
the Debian Project will EOL the LTS version of Debian 9 on 2022-06-20 which is way before the planned release of 22.0.0. As we will not build Bareos 22 for Debian 9 we can disable it now. As Debian 9 was the last system that did not have a compiler with full C++17 support, the immediate benefit is that we can now finally use C++17. We will also disable Univention 4.4, which is built on Debian 9.
Support for using offsetof() on non-standard-layout types is conditionally-supported[1]. All compiler we're using seem to support this, but are required to "emit a diagnostic" (i.e. warning). This patch disables this warning to allow that non-portable usage. [1] https://stackoverflow.com/q/47518542
Previously ITEM(c,m) was called with `c` being a NULL-pointer with the type of one of BareosResource's children. It then took the address of `c->m` as the offset of m within the type of `c`. This is undefined behaviour. It also relies on `c == NULL`, which was never checked. This patch replaces the existing method by using offsetof() with the non-pointer type of `c` and its member `m`, removing all NULL-pointer involvement and allowing to pass any pointer or non-pointer value of the desired type.
When building with ubsan linking would fail, as RTTI for JobResource was not available. With this patch all required objects are linked.
Looks like underflowing a pointer is undefined behaviour, so we just initialize with UINTPTR_MAX directly. Also replaced macro-based conditional compile with constexpr-if.
and also stop calling dlist::init().
Instead of copy-initializing, we now call placement-new. Also contained lists are default initialized by the constructor, so there is no need to do this expliticly. This will also allow us to remove dlist<T>::init() as this is the very last use.
This patch extracts dlistString and implements some minor changes to make it comply to the duck-typing that dlist<T> will expect in the future (i.e. have a public member of type `dlist<T> link`).
In preparation of changes to dlink<T> we make the link member public, so it can be accessed by dlink<T>'s constructor in a clean fashion.
prompts in `UaContext`s are created using `strdup()`, but are not freed when destroying the `UaContext` they are in. refactoring the multicolumn_prompts test
Backtracing doesn't seem to be compatible to address sanitizing. So we disable the backtrace test if asan is enabled.
previously, StoreRun() allocated a new RunResource in pass 1, but didn't free it. This patch converts StoreRun to use value semantics (i.e. just use a local RunResource object) and copy that into a heap object if needed during pass 2.
previously fmtstr() only supported correctly null-terminated strings when formatting %s, no matter what size specification was given. With this patch, fmtstr() will no longer read past the specified bytes required for the output that should be formatted.
The debug logging for xattr did not limit the length of name even though that value might not be null-terminated.
* free director's name when freeing a console resource * remove `password` in favor of base class' `password_` to avoid confusion
Fixes bareos#1412: Unable to build it, because of the default compiler flags on Fedora and unclean code
This patch configures ASAN/LSAN to work correctly with python code. As the embedded python interpreter will leak memory, we add a suppression to ignore the problematic parts. We also disable leak-detection and asan checking in the python module tests, as these will not give reasonable results.
This option enables Sanitizers for Addresses, Leaks and Undefinied Behaviour.
use only one configuration making use of test fixtures
arogge
force-pushed
the
dev/arogge/master/fix-ub
branch
from
b655f2a
to
41639b5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a lot (hopefully most) of the issues that you will uncover when running ctest after building with
-fsanitize=addressand-fsanitize=undefined.Besides a lot of smaller changes fixing undefined behaviour and memory leaks, this PR fixes a long-standing bug in Bvsnprintf() and refactors dlist and htable to be templatized and type-safe.
We also disable warnings for invalid uses of offsetof (see specific commit for details) and stop building packages for Debian 9 and Univention 4.4.
Thank you for contributing to the Bareos Project!
Please check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
General
Source code quality
bareos-check-sources --since-mergedoes not report any problemsgit statusshould not report modifications in the source tree after building and testing