From c1291fd102ed88ba8e1df63e5b2ae6fa94c73188 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sun, 4 Aug 2024 20:22:04 -0700 Subject: [PATCH 1/3] clang-tidy: enforce the new rules Fixes: https://git.lix.systems/lix-project/lix/issues/241 Change-Id: Idd096dc9ca92ffd4be8c22d293ba5bf2ec48a85f --- .clang-tidy | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.clang-tidy b/.clang-tidy index 0cc1f2520..ccfdf9e7d 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -16,6 +16,13 @@ Checks: - -bugprone-unchecked-optional-access # many warnings, seems like a questionable lint - -bugprone-branch-clone + # all thrown exceptions must derive from std::exception + - hicpp-exception-baseclass + # capturing async lambdas are dangerous + - cppcoreguidelines-avoid-capturing-lambda-coroutines + # crimes must be appropriately declared as crimes + - cppcoreguidelines-pro-type-cstyle-cast + CheckOptions: bugprone-reserved-identifier.AllowedIdentifiers: '__asan_default_options' From a85c4ce535c940bd2f48c34ab823fb3a8f5be0cc Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sun, 4 Aug 2024 22:03:35 -0700 Subject: [PATCH 2/3] tree-wide: automated migration to charptr_cast The lint did it :3 Change-Id: I2d9f276b01ebbf14101de4257ea13e44ff6fe0a0 --- src/libstore/build/worker.cc | 3 ++- src/libstore/crypto.cc | 13 +++++++------ src/libstore/sqlite.cc | 3 ++- src/libutil/charptr-cast.hh | 1 + src/libutil/compression.cc | 7 ++++--- src/libutil/file-descriptor.cc | 3 ++- src/libutil/tarfile.cc | 3 ++- 7 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/libstore/build/worker.cc b/src/libstore/build/worker.cc index 135cfecf5..30c4ed944 100644 --- a/src/libstore/build/worker.cc +++ b/src/libstore/build/worker.cc @@ -1,3 +1,4 @@ +#include "charptr-cast.hh" #include "machines.hh" #include "worker.hh" #include "substitution-goal.hh" @@ -529,7 +530,7 @@ void Worker::waitForInput() } else { printMsg(lvlVomit, "%1%: read %2% bytes", goal->getName(), rd); - std::string_view data(reinterpret_cast(buffer.data()), rd); + std::string_view data(charptr_cast(buffer.data()), rd); j->lastOutput = after; handleWorkResult(goal, goal->handleChildOutput(k, data)); } diff --git a/src/libstore/crypto.cc b/src/libstore/crypto.cc index e8ab15537..6f4a36735 100644 --- a/src/libstore/crypto.cc +++ b/src/libstore/crypto.cc @@ -1,3 +1,4 @@ +#include "charptr-cast.hh" #include "crypto.hh" #include "file-system.hh" #include "globals.hh" @@ -44,15 +45,15 @@ std::string SecretKey::signDetached(std::string_view data) const { unsigned char sig[crypto_sign_BYTES]; unsigned long long sigLen; - crypto_sign_detached(sig, &sigLen, reinterpret_cast(data.data()), data.size(), - reinterpret_cast(key.data())); + crypto_sign_detached(sig, &sigLen, charptr_cast(data.data()), data.size(), + charptr_cast(key.data())); return name + ":" + base64Encode(std::string(reinterpret_cast(sig), sigLen)); } PublicKey SecretKey::toPublicKey() const { unsigned char pk[crypto_sign_PUBLICKEYBYTES]; - crypto_sign_ed25519_sk_to_pk(pk, reinterpret_cast(key.data())); + crypto_sign_ed25519_sk_to_pk(pk, charptr_cast(key.data())); return PublicKey(name, std::string(reinterpret_cast(pk), crypto_sign_PUBLICKEYBYTES)); } @@ -85,9 +86,9 @@ bool verifyDetached(const std::string & data, const std::string & sig, if (sig2.size() != crypto_sign_BYTES) throw Error("signature is not valid"); - return crypto_sign_verify_detached(reinterpret_cast(sig2.data()), - reinterpret_cast(data.data()), data.size(), - reinterpret_cast(key->second.key.data())) == 0; + return crypto_sign_verify_detached(charptr_cast(sig2.data()), + charptr_cast(data.data()), data.size(), + charptr_cast(key->second.key.data())) == 0; } PublicKeys getDefaultPublicKeys() diff --git a/src/libstore/sqlite.cc b/src/libstore/sqlite.cc index 3114aad48..8d0bfcb11 100644 --- a/src/libstore/sqlite.cc +++ b/src/libstore/sqlite.cc @@ -1,3 +1,4 @@ +#include "charptr-cast.hh" #include "sqlite.hh" #include "globals.hh" #include "logging.hh" @@ -201,7 +202,7 @@ bool SQLiteStmt::Use::next() std::optional SQLiteStmt::Use::getStrNullable(int col) { - auto s = reinterpret_cast(sqlite3_column_text(stmt, col)); + auto s = charptr_cast(sqlite3_column_text(stmt, col)); return s != nullptr ? std::make_optional((s)) : std::nullopt; } diff --git a/src/libutil/charptr-cast.hh b/src/libutil/charptr-cast.hh index 990f2ec55..ec53d8924 100644 --- a/src/libutil/charptr-cast.hh +++ b/src/libutil/charptr-cast.hh @@ -134,6 +134,7 @@ template requires charptr_cast_detail::IsCharCastable inline To charptr_cast(From p) { + // NOLINTNEXTLINE(lix-charptrcast): stop the linter ever getting too clever and causing funny recursion return reinterpret_cast(p); } diff --git a/src/libutil/compression.cc b/src/libutil/compression.cc index 6b0fa9d15..5152a2146 100644 --- a/src/libutil/compression.cc +++ b/src/libutil/compression.cc @@ -1,3 +1,4 @@ +#include "charptr-cast.hh" #include "compression.hh" #include "tarfile.hh" #include "signals.hh" @@ -160,7 +161,7 @@ struct BrotliDecompressionSource : Source size_t read(char * data, size_t len) override { - uint8_t * out = reinterpret_cast(data); + uint8_t * out = charptr_cast(data); const auto * begin = out; while (len && !BrotliDecoderIsFinished(state.get())) { @@ -172,7 +173,7 @@ struct BrotliDecompressionSource : Source } catch (EndOfFile &) { break; } - next_in = reinterpret_cast(buf.get()); + next_in = charptr_cast(buf.get()); } if (!BrotliDecoderDecompressStream( @@ -238,7 +239,7 @@ struct BrotliCompressionSink : ChunkedCompressionSink void writeInternal(std::string_view data) override { - auto next_in = reinterpret_cast(data.data()); + auto next_in = charptr_cast(data.data()); size_t avail_in = data.size(); uint8_t * next_out = outbuf; size_t avail_out = sizeof(outbuf); diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 7c82988b3..be9f8c889 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -1,3 +1,4 @@ +#include "charptr-cast.hh" #include "file-system.hh" #include "finally.hh" #include "logging.hh" @@ -115,7 +116,7 @@ Generator drainFDSource(int fd, bool block) throw SysError("reading from file"); } else if (rd == 0) break; - else co_yield std::span{reinterpret_cast(buf.data()), (size_t) rd}; + else co_yield std::span{charptr_cast(buf.data()), (size_t) rd}; } } diff --git a/src/libutil/tarfile.cc b/src/libutil/tarfile.cc index f024149ec..316751533 100644 --- a/src/libutil/tarfile.cc +++ b/src/libutil/tarfile.cc @@ -1,6 +1,7 @@ #include #include +#include "charptr-cast.hh" #include "file-system.hh" #include "logging.hh" #include "serialise.hh" @@ -19,7 +20,7 @@ static ssize_t callback_read(struct archive * archive, void * _self, const void *buffer = self->buffer.data(); try { - return self->source->read(reinterpret_cast(self->buffer.data()), self->buffer.size()); + return self->source->read(charptr_cast(self->buffer.data()), self->buffer.size()); } catch (EndOfFile &) { return 0; } catch (std::exception & err) { From a5f0954c290157875b4dfb79edcf651f55742dc2 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Sun, 4 Aug 2024 22:02:53 -0700 Subject: [PATCH 3/3] clang-tidy: write a lint for charptr_cast This lets us ensure that nobody is putting in new reinterpret_cast instances where they could safely use charptr_cast instead. Change-Id: I6358a3934c8133c7150042635843bdbb6b9218d4 --- .clang-tidy | 7 +++ subprojects/lix-clang-tidy/CharPtrCast.cc | 45 +++++++++++++++++++ subprojects/lix-clang-tidy/CharPtrCast.hh | 32 +++++++++++++ .../lix-clang-tidy/LixClangTidyChecks.cc | 2 + subprojects/lix-clang-tidy/meson.build | 3 +- 5 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 subprojects/lix-clang-tidy/CharPtrCast.cc create mode 100644 subprojects/lix-clang-tidy/CharPtrCast.hh diff --git a/.clang-tidy b/.clang-tidy index ccfdf9e7d..87f6d0404 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -22,6 +22,13 @@ Checks: - cppcoreguidelines-avoid-capturing-lambda-coroutines # crimes must be appropriately declared as crimes - cppcoreguidelines-pro-type-cstyle-cast + - lix-* + # This can not yet be applied to Lix itself since we need to do source + # reorganization so that lix/ include paths work. + - -lix-fixincludes + # This lint is included as an example, but the lib function it replaces is + # already gone. + - -lix-hasprefixsuffix CheckOptions: diff --git a/subprojects/lix-clang-tidy/CharPtrCast.cc b/subprojects/lix-clang-tidy/CharPtrCast.cc new file mode 100644 index 000000000..e76797f5d --- /dev/null +++ b/subprojects/lix-clang-tidy/CharPtrCast.cc @@ -0,0 +1,45 @@ +#include "CharPtrCast.hh" +#include +#include +#include + +namespace nix::clang_tidy { +using namespace clang::ast_matchers; +using namespace clang; + +void CharPtrCastCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + Finder->addMatcher( + traverse(clang::TK_IgnoreUnlessSpelledInSource, + cxxReinterpretCastExpr(allOf( + hasDestinationType(qualType(pointsTo(isAnyCharacter()))), + has(expr(hasType(qualType(pointsTo(isAnyCharacter())))))))) + .bind("reinterpret-cast-expr"), + this); +} + +void CharPtrCastCheck::check( + const ast_matchers::MatchFinder::MatchResult &Result) { + const auto ReinterpretCastExpr = + Result.Nodes.getNodeAs("reinterpret-cast-expr"); + const auto ToTypeSpan = ReinterpretCastExpr->getAngleBrackets(); + const auto & SM = Result.Context->getSourceManager(); + + auto Diag = + diag(ReinterpretCastExpr->getExprLoc(), + "reinterpret_cast used for trivially safe character pointer cast"); + Diag << ReinterpretCastExpr->getSourceRange(); + + auto Inside = tooling::getText(*ReinterpretCastExpr->getSubExprAsWritten(), + *Result.Context); + + Diag << Inserter.createIncludeInsertion(SM.getFileID(ReinterpretCastExpr->getExprLoc()), "charptr-cast.hh"); + + llvm::Twine Replacement = + "charptr_cast" + + tooling::getText(CharSourceRange(ToTypeSpan, true), *Result.Context) + + "(" + Inside + ")"; + Diag << FixItHint::CreateReplacement(ReinterpretCastExpr->getSourceRange(), + Replacement.str()); +} + +} // namespace nix::clang_tidy diff --git a/subprojects/lix-clang-tidy/CharPtrCast.hh b/subprojects/lix-clang-tidy/CharPtrCast.hh new file mode 100644 index 000000000..66883d055 --- /dev/null +++ b/subprojects/lix-clang-tidy/CharPtrCast.hh @@ -0,0 +1,32 @@ +#pragma once +///@file + +#include +#include +#include +#include + +namespace nix::clang_tidy { + +using namespace clang; +using namespace clang::tidy; + +class CharPtrCastCheck : public ClangTidyCheck { + tidy::utils::IncludeInserter Inserter{ + Options.getLocalOrGlobal("IncludeStyle", + tidy::utils::IncludeSorter::IS_Google), + false}; + +public: + CharPtrCastCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + + void registerPPCallbacks(const SourceManager &, Preprocessor *PP, + Preprocessor *) override { + Inserter.registerPreprocessor(PP); + } + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; +} // namespace nix::clang_tidy diff --git a/subprojects/lix-clang-tidy/LixClangTidyChecks.cc b/subprojects/lix-clang-tidy/LixClangTidyChecks.cc index b3503dd3a..283e0e7c8 100644 --- a/subprojects/lix-clang-tidy/LixClangTidyChecks.cc +++ b/subprojects/lix-clang-tidy/LixClangTidyChecks.cc @@ -2,6 +2,7 @@ #include #include "FixIncludes.hh" #include "HasPrefixSuffix.hh" +#include "CharPtrCast.hh" namespace nix::clang_tidy { using namespace clang; @@ -12,6 +13,7 @@ class NixClangTidyChecks : public ClangTidyModule { void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck("lix-hasprefixsuffix"); CheckFactories.registerCheck("lix-fixincludes"); + CheckFactories.registerCheck("lix-charptrcast"); } }; diff --git a/subprojects/lix-clang-tidy/meson.build b/subprojects/lix-clang-tidy/meson.build index ef0226420..43648a1c8 100644 --- a/subprojects/lix-clang-tidy/meson.build +++ b/subprojects/lix-clang-tidy/meson.build @@ -5,9 +5,10 @@ project('lix-clang-tidy', ['cpp', 'c'], llvm = dependency('Clang', version: '>= 17', modules: ['libclang']) sources = files( + 'CharPtrCast.cc', + 'FixIncludes.cc', 'HasPrefixSuffix.cc', 'LixClangTidyChecks.cc', - 'FixIncludes.cc', ) lix_clang_tidy = shared_module('lix-clang-tidy', sources,