core: refactor config parser; fix ktls configuration; fix crashes/ub#2222
Conversation
6254b22 to
d5e7442
Compare
There was a problem hiding this comment.
Great work!
I think there are some really nice improvements to the config parser code.
I have some small remarks and some larger ones, since this PR is already really big and implementing some of this might require quite some time, feel free to write whether you want to invest the effort or not.
florian-at-bareos
left a comment
There was a problem hiding this comment.
Great work! I especially like the ResourceItem config system you wrote!
If you want you can replace the const char* with std::optional<std::string_view> (see above discussion) but since this was a const char* before we can also just leave it as is for now.
In case you haven't noticed:
Some of your commit titles are longer than 50 characters.
|
maybe rename to "refactor config parser" since there are more changes than in parse_conf? |
good idea! |
e8babeb to
0bab41b
Compare
0bab41b to
4e6bdca
Compare
If we read a string that does not match a resource name when trying to parse a resource, then we would do out of bounds reads for !function! pointers. This is obviously not great!
A lot of configuration code expects that e.g. ResourceTable has R_NUM elements, which causes oob reads because R_JOB/R_STORAGE arent actually resource types. R_STORAGE seems to be unused and R_JOB is only used once for formatting a string.
Obviously we want to always use the `EnableKtls` setting of the daemon and not from the "target", as otherwise it would be impossible to use ktls between fd and sd.
Also removes direct dependency on ConfigurationParser from ConfigResourceContainer.
Each configresourcecontainer now keeps its successors alive. This ensures that if something (i.e. a job) keeps alive its configuration, and accidentally accesses the global configuration afterwards (without saving it as well), then the daemon will not crash because of use after free, as the newer configurations are now also kept alive implicitly as well.
Now that resource items are only accessed via const ptr/ref, we do not need this anymore.
4e6bdca to
718de2c
Compare
Thank you for contributing to the Bareos Project!
This pr fixes a lot of bugs in our configuration & config parser. It also changes how we define our configuration to make sure that these mistakes cannot happen anymore. The PR also contains the following fixes:
The ktls configuration of each daemon will now determine whether that particular daemon tries to use it.
Now each configuration gets kept alive by their predecessors, which means that jobs which incorrectly try to access out of date configurations, will not create use-after-free situations.
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)
Make sure you check/merge the PR using
devtools/pr-toolto have some simple automated checks run and a proper changelog record added.General
Source code quality