Fix warnings found by clang-tidy #540
Merged
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
The new Docker image has newer versions or Clang and associated tools so let's use them to get all the latest analysis and warnings.
We've been using not that that much checks. Let's enable whatever available. Note, however, that code base has quite a few legacy warts so we can't enable everything and have to disable some of the warnings. We still support C++03 so we do not enable "modernize-*" group. This hides an amazing amount of warnings from JsThemis code (which ended up with its own clang-tidy configuration). Some other tweaks: - readability-implicit-bool-conversion - because we have quite a few places where we check integers for zero with "!" and all of them trigger this warning - readability-isolate-declaration - "sput" (our testing framework) macros cause most of these warnings, hopefully we can enable this back after we migrate to a newer framework for unit tests - *-magic-numbers - unfortunately, clang-tidy gives a warning for *every individual integer* in data arrays (like in CRC32 code) which is idiotic, sorry but this goes out of the window - cppcoreguidelines-pro-type-vararg - we use printf() functions quite often (due to sput and SOTER_IF macros), don't want this to be triggered. Maybe we can disable this later
Macro expansion should use parentheses to avoid unexpected precendence rule application. Wrap all uses of parameters in parentheses. At some places clang-format thinks that binary operation is actually unary type cast. Fix spacing these. This does not affect the semantics after the macros are expanded.
There is a warning for not checking memcmp() result (effectively checking for "not equal"). Write "!= 0" explicitly to fix this.
BN_num_bytes() and RSA_size() return "unsigned int" in BoringSSL while in OpenSSL they use regular (signed) "int". Update the code to expect correct types from these functions.
There are a couple of warnings about unused labels. They are caused by the old compatibility code not using the labels because it does not have failure conditions that we present in the new code. Instead of using ifdefs in the code base, introduce compatibility shims for newer functions like RSA_get0_key() that we not available in older OpenSSL versions. This forces us to handle possible error conditions and gets rid of the warnings. Additionally, all compatibility code is now compactly located in one place and can be easily removed later.
For some historical reasons signature checking code needs access to private data structures of cryptographic backend. We achieve that be cleverly applying advanced preprocessor macrology. Unfortunately, it confuses clang-tidy which emits a lint here. Suppress the lint and avoid code duplication. These changes are limited to "soter_t.h" which is a private header file. They are not visible to the users, but we should review the code later and try to avoid dependency on the implementation details like the choice of cryptographic backend.
Static analyzer gets confused by all possible code paths through this enormous function, and produces a bunch of warnings about possible NULL pointer dereference and double-free errors. Let's help it a little by making sure that we handle NULL values in the helper functions and by zeroing out recently freed pointers. Yeah, it would be better to refactor this complex functions, but let's keep it as is for now. We'll update it later, together with a major unit-test overhaul. (Still, I'm amazed at Clang being able to see a possible error condition which requires around a dozen of other conditions to hold in order for it to have a chance at triggering.)
clang-tidy static analyzer sometimes gets confused by string literals being used in multi-level macros and thinks that they are actually character arrays which get implicitly casted to pointers. Let's silence these warnings by explicitly casting strings to "const char*".
Non-inline and non-static functions in header files may trigger weird issues with one-definition rule (ODR) if the definitions are different. It's obviously not true in our case but you tell that to clang-tidy. Mark private functions as static and exported functions as inline to avoid these warnings. Ideally, we'd put the actual code to *.cpp files. We'll do that later together with proper refactoring of unit tests.
First of all, add explicit virtual destructors to classes that declare virtual functions. This is important for deallocation to work properly, especially for the callback interface of Secure Session. (Other classes do not really need inheritance, but they currently use it and thus need the destructors as well.) After that, bring back copy constructor to secure_message_t. There iso no reason for these objects to not be copyable or moveable. Finally, implement a move constructor for secure_comparator_t. These objects can be moved, but cannot be copied. Add a move constructor and move-assignment operator for them. Delete the copy constructor and copy-assignment operator. Note that we keep compatibility with C++03 and provide explicit empty functions and the 'private constructor' hack there. With C++11 we can use default and deleted implementations directly.
Here we test that our objects have reasonable behavior in this state so we need to trigger this questionable code path.
Various code snipptes like
std::vector<uint8_t> pass1(password1.c_str(),
password1.c_str() + password1.length());
cause clang-tidy warnings about pointer arithmetic being dangerous and
discouraged. ThemisPP interfaces work with std::vector<uint8_t>, so
instead of using std::string, let's use std::vector directly.
Introduce a helper function to convert a string into a vector,
replace all inline instances of such conversion with just references
to a global constant with test data. This fixes most of the warnings.
There is more interesting story with Secure Session tests. First of
all, it requires private keys which are obvioulsy not strings. Currently
we use C arrays which get two warnings: one about using C arrays
(in C++11 there is std::array which is safer) and another one for
inevitable use of pointer arithmetic.
In C++11 we'd use initialization lists to construct a vector from
C array syntax, but C++03 does not have that option so we have to
keep the C arrays and convert them into vectors.
Another thing is replacing std::string with std::vector as a key for
std::unordered_map that we use to keep client IDs. The thing is that
std::vector does not have a built-in implementation of std::hash
so we have to provide our own for that code to compile.
clang-tidy produces a warning about unhandled exceptions in main(). We don't really care for std::terminate being called in tests since that's a failure as well, but let's add a catch clause to make the analyzer happy.
Exactly what it says on the tin: if we can use C++ constants in C++ code then we should use that instead of preprocessor macros. Technically, this changes the public API, but it's unlikely that anyone used this macro and it's an implementation detail anyway.
There is another warning for C-style pointer casts. In this case we know exactly what kind of pointer is there under the disguise of void* so we can use static_cast instead. Also, mark pointer arithmetic when constructing vectors with NOLINT since we really can't get around that: that's C callback interface.
We keep compatibility with C++03 here and cannot use std::unique_ptr. Since std::auto_ptr is broken and should not be used, we continue using carefully placed raw pointers. Mark these code sites as NOLINT to suppress clang-tidy warnings.
Use of protected data fields is discouraged, and actually there is little reason for Secure Cell to use inheritance. However, current code is like that and we'd like to keep it that way before a major refactoring. Silence the warnings and move on.
This method is missing a reference symbol, most likely due to a typo. Make it accept a reference which in turns makes the warning about useless "const" specifier to go away. Also, this technically changes the ABI, but since C++ does not have a stable ABI, it should be okay. Previously written code will be okay with this change.
clang-tidy produces a warning about this case. There is really little use to return a const vector by copy here. Drop that. Unfortunately, this changes the API and ABI of the code and user applications may need to be updated to compile against the newer version. It is not possible to degrade this gracefully. Though, the change is minor and should not cause issues when upgrading.
There is no real reason for this field to be public. It's been like that since the beginning. Instead of exposing the field as public, declare the function that needs access to it as private static method of SecureSession class. This provides access to private members and we can still use this function as a callback.
ilammy
added
next release
core
ilammy
requested review from
Lagovas,
storojs72 and
vixentael
as code owners
vixentael
reviewed
Sep 26, 2019
Lagovas
reviewed
Sep 26, 2019
| template <asym_algs alg_t_p, size_t max_key_length_t_p = MAX_KEY_LENGTH> | ||
| static const size_t max_key_length = 10 * 1024; | ||
|
|
||
| template <asym_algs alg_t_p, size_t max_key_length_t_p = max_key_length> |
There was a problem hiding this comment.
did you check that it works on c++03?
There was a problem hiding this comment.
It should work with C++03. Here we check that ThemisPP tests pass in C++03 mode:
Lines 118 to 119 in 8c07b5b
and they indeed seem to pass.
It’s a constant, not constexpr which requires C++11.
| // Allow usage of non-owning Secure Session constructor for testing | ||
| #if defined(__GNUC__) || defined(__clang__) | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wdeprecated-declarations" | ||
| #endif | ||
|
|
||
| // Thank you, STL, for providing this out-of-the-box. It's really convenient. |
There was a problem hiding this comment.
std::unordered_map used in secure_session_moved() test requires the key type to be hashable. We use std::vectors as keys and they are not hashable, unfortunately.
There was a problem hiding this comment.
actually I don't like this comment because we look like kids that crying due to needing write a small piece of code. especially when STL provides a way to extend base structures with custom types and vector which supports a lot of other types may be implemented in different ways. but it is up to you leave or remove :)
There was a problem hiding this comment.
Yes, it does piss me off personally a bit because every other language that I known which has built-in 'vectors' and 'hash maps' allows to use vectors as map keys right away, without having to specialize anything in their standard library.
I'm not even 100% sure that it's legal to do it the way I do here. I remember that one is not allowed to overload any function in namespace std, but it's okay to provide specializations for templates there. However, I'm not sure that it's okay to provide specialization for all vectors – maybe std::vector<bool> is special again.
Since you've brought this up I guess I'll put my serious, professional hat on and replace this comment with something more neutral.
Well, there are breaking changes in ThemisPP, strictly speaking (as in "C++ allows to write code which will not compile after an update"), but they are otherwise trivial to correct. Since ThemisPP is header-only, the users will have to recompile anyway. |
Lagovas
approved these changes
Sep 27, 2019
ilammy
added
W-JsThemis 🍭
1 task
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.
We have been using clang-tidy for a while (added in #395). However, this isn't even its final form. Most of the warnings were disabled and we were really unaware of them.
Let’s update the Clang toolchain to the newer version that got recently released and then enable more available checks. Note, however, that code base has quite a few legacy warts so we can't enable everything and have to disable some of the warnings.
We still support C++03 so we do not enable
modernize-*group. This hides an amazing amount of warnings from JsThemis code (which ended up with its own clang-tidy configuration).Some other explicitly disabled checks:
readability-implicit-bool-conversion– because we have quite a few places where we check integers for zero with!and all of them trigger this warning.readability-isolate-declaration– sput (our testing framework) macros cause most of these warnings, hopefully we can enable this back after we migrate to a newer framework for unit tests.*-magic-numbers– unfortunately, clang-tidy gives a warning for every individual integer in data arrays (like in CRC32 code) which isstupidless than ideal. Sorry, but this goes out of the window.cppcoreguidelines-pro-type-vararg– we use printf() functions quite often (due to sput and SOTER_IF macros), don't want this to be triggered. Maybe we can disable this later.Most of the changes are trivial, but there are a lot of them, and some might have caused trouble in case of misuse or sloppy coding. Well, our code is not that bad! I’m a tiiiiny bit relieved.
Some changes in ThemisPP are significant and may require changes in the application code. We will have to announce them in the changelog:
secure_message_tis now copyablesecure_comparator_tis now non-copyable, but moveablesecure_session_callback_interface_tnow has to return non-const vectorAs usual, read individual commit messages for details. I made all changes atomic so it will be tedious, but straightforward to review if you look at them separately.