From 0cc285f87b25365b6050753fba76713332185012 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Thu, 22 Aug 2024 22:44:29 -0700 Subject: [PATCH] treewide: fix a bunch of lints Fixes: - Identifiers starting with _ are prohibited - Some driveby header dependency cleaning which wound up with doing some extra fixups. - Fucking C style casts, man. C++ made these 1000% worse by letting you also do memory corruption with them with references. - Remove casts to Expr * where ExprBlackHole is an incomplete type by introducing an explicitly-cast eBlackHoleAddr as Expr *. - An incredibly illegal cast of the text bytes of the StorePath hash into a size_t directly. You can't DO THAT. Replaced with actually parsing the hash so we get 100% of the bits being entropy, then memcpying the start of the hash. If this shows up in a profile we should just make the hash parser faster with a lookup table or something sensible like that. - This horrendous bit of UB which I thankfully slapped a deprecation warning on, built, and it didn't trigger anywhere so it was dead code and I just deleted it. But holy crap you *cannot* do that. inline void mkString(const Symbol & s) { mkString(((const std::string &) s).c_str()); } - Some wrong lints. Lots of wrong macro lints, one wrong suspicious-sizeof lint triggered by the template being instantiated with only pointers, but the calculation being correct for both pointers and not-pointers. - Exceptions in destructors strike again. I tried to catch the exceptions that might actually happen rather than all the exceptions imaginable. We can let the runtime hard-kill it on other exceptions imo. Change-Id: I71761620846cba64d66ee7ca231b20c061e69710 --- src/libcmd/built-path.hh | 16 +++++---- src/libexpr/eval-error.hh | 3 +- src/libexpr/eval-inline.hh | 6 ++-- src/libexpr/gc-alloc.hh | 1 + src/libexpr/nixexpr.cc | 1 + src/libexpr/value.hh | 35 ++++++++----------- src/libstore/common-protocol-impl.hh | 1 + src/libstore/derived-path.hh | 16 +++++---- .../length-prefixed-protocol-helper.hh | 6 ++-- src/libstore/path.cc | 10 ++++++ src/libstore/path.hh | 8 ++--- src/libstore/pathlocks.hh | 8 +++-- src/libstore/serve-protocol-impl.hh | 1 + src/libstore/sqlite.hh | 2 +- src/libstore/worker-protocol-impl.hh | 1 + src/libutil/error.hh | 5 +-- src/libutil/finally.hh | 1 + src/libutil/hash.cc | 3 +- src/libutil/serialise.cc | 1 + src/libutil/serialise.hh | 25 ++++++++----- src/libutil/signals.cc | 1 + src/libutil/thread-pool.hh | 1 + src/libutil/url.hh | 1 + src/libutil/variant-wrapper.hh | 2 ++ tests/unit/libutil/url.cc | 1 + 25 files changed, 94 insertions(+), 62 deletions(-) diff --git a/src/libcmd/built-path.hh b/src/libcmd/built-path.hh index da87a33b0..555adb04f 100644 --- a/src/libcmd/built-path.hh +++ b/src/libcmd/built-path.hh @@ -20,13 +20,15 @@ struct SingleBuiltPathBuilt { DECLARE_CMP(SingleBuiltPathBuilt); }; -using _SingleBuiltPathRaw = std::variant< +namespace built_path::detail { +using SingleBuiltPathRaw = std::variant< DerivedPathOpaque, SingleBuiltPathBuilt >; +} -struct SingleBuiltPath : _SingleBuiltPathRaw { - using Raw = _SingleBuiltPathRaw; +struct SingleBuiltPath : built_path::detail::SingleBuiltPathRaw { + using Raw = built_path::detail::SingleBuiltPathRaw; using Raw::Raw; using Opaque = DerivedPathOpaque; @@ -65,17 +67,19 @@ struct BuiltPathBuilt { DECLARE_CMP(BuiltPathBuilt); }; -using _BuiltPathRaw = std::variant< +namespace built_path::detail { +using BuiltPathRaw = std::variant< DerivedPath::Opaque, BuiltPathBuilt >; +} /** * A built path. Similar to a DerivedPath, but enriched with the corresponding * output path(s). */ -struct BuiltPath : _BuiltPathRaw { - using Raw = _BuiltPathRaw; +struct BuiltPath : built_path::detail::BuiltPathRaw { + using Raw = built_path::detail::BuiltPathRaw; using Raw::Raw; using Opaque = DerivedPathOpaque; diff --git a/src/libexpr/eval-error.hh b/src/libexpr/eval-error.hh index 19540d612..7114d392f 100644 --- a/src/libexpr/eval-error.hh +++ b/src/libexpr/eval-error.hh @@ -1,9 +1,8 @@ #pragma once ///@file -#include - #include "error.hh" +#include "types.hh" #include "pos-idx.hh" namespace nix { diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 4631b71eb..30badb93f 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -31,7 +31,7 @@ Value * EvalState::allocValue() #endif nrValues++; - return (Value *) p; + return static_cast(p); } @@ -54,10 +54,10 @@ Env & EvalState::allocEnv(size_t size) void * p = *env1AllocCache; *env1AllocCache = GC_NEXT(p); GC_NEXT(p) = nullptr; - env = (Env *) p; + env = static_cast(p); } else #endif - env = (Env *) gcAllocBytes(sizeof(Env) + size * sizeof(Value *)); + env = static_cast(gcAllocBytes(sizeof(Env) + size * sizeof(Value *))); /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ diff --git a/src/libexpr/gc-alloc.hh b/src/libexpr/gc-alloc.hh index fc034045f..afdd7eeb0 100644 --- a/src/libexpr/gc-alloc.hh +++ b/src/libexpr/gc-alloc.hh @@ -120,6 +120,7 @@ inline T * gcAllocType(size_t howMany = 1) // However, people can and do request zero sized allocations, so we need // to check that neither of our multiplicands were zero before complaining // about it. + // NOLINTNEXTLINE(bugprone-sizeof-expression): yeah we only seem to alloc pointers with this. the calculation *is* correct though! auto checkedSz = checked::Checked(howMany) * sizeof(T); size_t sz = checkedSz.valueWrapping(); if (checkedSz.overflowed()) { diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 08d4b279b..68da254e2 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -11,6 +11,7 @@ namespace nix { ExprBlackHole eBlackHole; +Expr *eBlackHoleAddr = &eBlackHole; // FIXME: remove, because *symbols* are abstract and do not have a single // textual representation; see printIdentifier() diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 57485aa0a..e38b11cbf 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -136,7 +136,9 @@ class ExternalValueBase std::ostream & operator << (std::ostream & str, const ExternalValueBase & v); -extern ExprBlackHole eBlackHole; +/** This is just the address of eBlackHole. It exists because eBlackHole has an + * incomplete type at usage sites so is not possible to cast. */ +extern Expr *eBlackHoleAddr; struct NewValueAs { @@ -196,6 +198,7 @@ private: public: // Discount `using NewValueAs::*;` +// NOLINTNEXTLINE(bugprone-macro-parentheses) #define USING_VALUETYPE(name) using name = NewValueAs::name USING_VALUETYPE(integer_t); USING_VALUETYPE(floating_t); @@ -473,7 +476,7 @@ public: /// Constructs an evil thunk, whose evaluation represents infinite recursion. explicit Value(blackhole_t) : internalType(tThunk) - , thunk({ .env = nullptr, .expr = reinterpret_cast(&eBlackHole) }) + , thunk({ .env = nullptr, .expr = eBlackHoleAddr }) { } Value(Value const & rhs) = default; @@ -513,7 +516,10 @@ public: // type() == nThunk inline bool isThunk() const { return internalType == tThunk; }; inline bool isApp() const { return internalType == tApp; }; - inline bool isBlackhole() const; + inline bool isBlackhole() const + { + return internalType == tThunk && thunk.expr == eBlackHoleAddr; + } // type() == nFunction inline bool isLambda() const { return internalType == tLambda; }; @@ -669,11 +675,6 @@ public: void mkStringMove(const char * s, const NixStringContext & context); - inline void mkString(const Symbol & s) - { - mkString(((const std::string &) s).c_str()); - } - void mkPath(const SourcePath & path); inline void mkPath(const char * path) @@ -732,7 +733,11 @@ public: lambda.fun = f; } - inline void mkBlackhole(); + inline void mkBlackhole() + { + internalType = tThunk; + thunk.expr = eBlackHoleAddr; + } void mkPrimOp(PrimOp * p); @@ -832,18 +837,6 @@ public: } }; - -bool Value::isBlackhole() const -{ - return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole; -} - -void Value::mkBlackhole() -{ - internalType = tThunk; - thunk.expr = (Expr*) &eBlackHole; -} - using ValueVector = GcVector; using ValueMap = GcMap; using ValueVectorMap = std::map; diff --git a/src/libstore/common-protocol-impl.hh b/src/libstore/common-protocol-impl.hh index fd1387e95..387f848ed 100644 --- a/src/libstore/common-protocol-impl.hh +++ b/src/libstore/common-protocol-impl.hh @@ -20,6 +20,7 @@ namespace nix { { \ return LengthPrefixedProtoHelper::read(store, conn); \ } \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ TEMPLATE [[nodiscard]] WireFormatGenerator CommonProto::Serialise< T >::write(const Store & store, CommonProto::WriteConn conn, const T & t) \ { \ return LengthPrefixedProtoHelper::write(store, conn, t); \ diff --git a/src/libstore/derived-path.hh b/src/libstore/derived-path.hh index c87cf2004..c96e0df67 100644 --- a/src/libstore/derived-path.hh +++ b/src/libstore/derived-path.hh @@ -78,10 +78,12 @@ struct SingleDerivedPathBuilt { DECLARE_CMP(SingleDerivedPathBuilt); }; -using _SingleDerivedPathRaw = std::variant< +namespace derived_path::detail { +using SingleDerivedPathRaw = std::variant< DerivedPathOpaque, SingleDerivedPathBuilt >; +} /** * A "derived path" is a very simple sort of expression (not a Nix @@ -94,8 +96,8 @@ using _SingleDerivedPathRaw = std::variant< * - built, in which case it is a pair of a derivation path and an * output name. */ -struct SingleDerivedPath : _SingleDerivedPathRaw { - using Raw = _SingleDerivedPathRaw; +struct SingleDerivedPath : derived_path::detail::SingleDerivedPathRaw { + using Raw = derived_path::detail::SingleDerivedPathRaw; using Raw::Raw; using Opaque = DerivedPathOpaque; @@ -201,10 +203,12 @@ struct DerivedPathBuilt { DECLARE_CMP(DerivedPathBuilt); }; -using _DerivedPathRaw = std::variant< +namespace derived_path::detail { +using DerivedPathRaw = std::variant< DerivedPathOpaque, DerivedPathBuilt >; +} /** * A "derived path" is a very simple sort of expression that evaluates @@ -216,8 +220,8 @@ using _DerivedPathRaw = std::variant< * - built, in which case it is a pair of a derivation path and some * output names. */ -struct DerivedPath : _DerivedPathRaw { - using Raw = _DerivedPathRaw; +struct DerivedPath : derived_path::detail::DerivedPathRaw { + using Raw = derived_path::detail::DerivedPathRaw; using Raw::Raw; using Opaque = DerivedPathOpaque; diff --git a/src/libstore/length-prefixed-protocol-helper.hh b/src/libstore/length-prefixed-protocol-helper.hh index 1475d2690..423bc77b8 100644 --- a/src/libstore/length-prefixed-protocol-helper.hh +++ b/src/libstore/length-prefixed-protocol-helper.hh @@ -61,9 +61,9 @@ template LENGTH_PREFIXED_PROTO_HELPER(Inner, std::tuple); template -#define _X std::map -LENGTH_PREFIXED_PROTO_HELPER(Inner, _X); -#undef _X +#define DONT_SUBSTITUTE_KV_TYPE std::map +LENGTH_PREFIXED_PROTO_HELPER(Inner, DONT_SUBSTITUTE_KV_TYPE); +#undef DONT_SUBSTITUTE_KV_TYPE template std::vector diff --git a/src/libstore/path.cc b/src/libstore/path.cc index d4b5fc0dc..82ed20285 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -112,3 +112,13 @@ PathSet Store::printStorePathSet(const StorePathSet & paths) const } } + +std::size_t std::hash::operator()(const nix::StorePath & path) const noexcept +{ + // It's already a cryptographic hash of 160 bits (assuming that nobody gives us bogus ones...), so just parse it. + auto h = nix::Hash::parseNonSRIUnprefixed(path.hashPart(), nix::HashType::SHA1); + // This need not be stable across machines, so bit casting the start of it is fine. + size_t r; + memcpy(&r, h.hash, sizeof(r)); + return r; +} diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 4ca6747b3..09b4710c1 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -2,8 +2,9 @@ ///@file #include +#include -#include "types.hh" +#include "types.hh" // IWYU pragma: keep namespace nix { @@ -89,10 +90,7 @@ const std::string drvExtension = ".drv"; namespace std { template<> struct hash { - std::size_t operator()(const nix::StorePath & path) const noexcept - { - return * (std::size_t *) path.to_string().data(); - } + std::size_t operator()(const nix::StorePath & path) const noexcept; }; } diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh index 7fcfa2e40..d06d031b5 100644 --- a/src/libstore/pathlocks.hh +++ b/src/libstore/pathlocks.hh @@ -49,8 +49,12 @@ struct FdLock ~FdLock() { - if (acquired) - lockFile(fd, ltNone, false); + try { + if (acquired) + lockFile(fd, ltNone, false); + } catch (SysError &) { + ignoreException(); + } } }; diff --git a/src/libstore/serve-protocol-impl.hh b/src/libstore/serve-protocol-impl.hh index cfb1dd574..a1d1c797f 100644 --- a/src/libstore/serve-protocol-impl.hh +++ b/src/libstore/serve-protocol-impl.hh @@ -20,6 +20,7 @@ namespace nix { { \ return LengthPrefixedProtoHelper::read(store, conn); \ } \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ TEMPLATE [[nodiscard]] WireFormatGenerator ServeProto::Serialise< T >::write(const Store & store, ServeProto::WriteConn conn, const T & t) \ { \ return LengthPrefixedProtoHelper::write(store, conn, t); \ diff --git a/src/libstore/sqlite.hh b/src/libstore/sqlite.hh index ca021087f..5740c4e45 100644 --- a/src/libstore/sqlite.hh +++ b/src/libstore/sqlite.hh @@ -1,9 +1,9 @@ #pragma once ///@file -#include #include +#include "types.hh" #include "error.hh" struct sqlite3; diff --git a/src/libstore/worker-protocol-impl.hh b/src/libstore/worker-protocol-impl.hh index b2603b954..d99c59f84 100644 --- a/src/libstore/worker-protocol-impl.hh +++ b/src/libstore/worker-protocol-impl.hh @@ -20,6 +20,7 @@ namespace nix { { \ return LengthPrefixedProtoHelper::read(store, conn); \ } \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ TEMPLATE [[nodiscard]] WireFormatGenerator WorkerProto::Serialise< T >::write(const Store & store, WorkerProto::WriteConn conn, const T & t) \ { \ return LengthPrefixedProtoHelper::write(store, conn, t); \ diff --git a/src/libutil/error.hh b/src/libutil/error.hh index 06dfba0df..73c1ccadd 100644 --- a/src/libutil/error.hh +++ b/src/libutil/error.hh @@ -16,16 +16,12 @@ */ #include "suggestions.hh" -#include "ref.hh" -#include "types.hh" #include "fmt.hh" #include #include #include -#include #include -#include #include #include @@ -173,6 +169,7 @@ public: }; #define MakeError(newClass, superClass) \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ class newClass : public superClass \ { \ public: \ diff --git a/src/libutil/finally.hh b/src/libutil/finally.hh index dc51d7b1e..5e62dfa3b 100644 --- a/src/libutil/finally.hh +++ b/src/libutil/finally.hh @@ -22,6 +22,7 @@ public: Finally(Finally &&other) : fun(std::move(other.fun)) { other.movedFrom = true; } + // NOLINTNEXTLINE(bugprone-exception-escape): the noexcept is declared properly here, the analysis seems broken ~Finally() noexcept(noexcept(fun())) { try { diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 925f71f80..f05d4aa98 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -7,6 +7,7 @@ #include "args.hh" #include "hash.hh" #include "archive.hh" +#include "charptr-cast.hh" #include "logging.hh" #include "split.hh" @@ -129,7 +130,7 @@ std::string Hash::to_string(Base base, bool includeType) const break; case Base::Base64: case Base::SRI: - s += base64Encode(std::string_view(reinterpret_cast(hash), hashSize)); + s += base64Encode(std::string_view(charptr_cast(hash), hashSize)); break; } return s; diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index f1db05b0b..a6dd7e200 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -1,4 +1,5 @@ #include "serialise.hh" +#include "charptr-cast.hh" #include "signals.hh" #include diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 8218db440..d6a22b3e9 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -4,6 +4,7 @@ #include #include +#include "charptr-cast.hh" #include "generator.hh" #include "strings.hh" #include "types.hh" @@ -385,7 +386,7 @@ struct SerializingTransform buf[5] = (n >> 40) & 0xff; buf[6] = (n >> 48) & 0xff; buf[7] = (unsigned char) (n >> 56) & 0xff; - return {reinterpret_cast(buf.begin()), 8}; + return {charptr_cast(buf.begin()), 8}; } static Bytes padding(size_t unpadded) @@ -417,6 +418,9 @@ struct SerializingTransform void writePadding(size_t len, Sink & sink); +// NOLINTBEGIN(cppcoreguidelines-avoid-capturing-lambda-coroutines): +// These coroutines do their entire job before the semicolon and are not +// retained, so they live long enough. inline Sink & operator<<(Sink & sink, uint64_t u) { return sink << [&]() -> WireFormatGenerator { co_yield u; }(); @@ -441,6 +445,7 @@ inline Sink & operator<<(Sink & sink, const Error & ex) { return sink << [&]() -> WireFormatGenerator { co_yield ex; }(); } +// NOLINTEND(cppcoreguidelines-avoid-capturing-lambda-coroutines) MakeError(SerialisationError, Error); @@ -448,7 +453,7 @@ template T readNum(Source & source) { unsigned char buf[8]; - source((char *) buf, sizeof(buf)); + source(charptr_cast(buf), sizeof(buf)); auto n = readLittleEndian(buf); @@ -540,13 +545,17 @@ struct FramedSource : Source ~FramedSource() { - if (!eof) { - while (true) { - auto n = readInt(from); - if (!n) break; - std::vector data(n); - from(data.data(), n); + try { + if (!eof) { + while (true) { + auto n = readInt(from); + if (!n) break; + std::vector data(n); + from(data.data(), n); + } } + } catch (...) { + ignoreException(); } } diff --git a/src/libutil/signals.cc b/src/libutil/signals.cc index a94c2802a..04a697d01 100644 --- a/src/libutil/signals.cc +++ b/src/libutil/signals.cc @@ -3,6 +3,7 @@ #include "sync.hh" #include "terminal.hh" +#include #include namespace nix { diff --git a/src/libutil/thread-pool.hh b/src/libutil/thread-pool.hh index 3db7ce88f..380e1a2d2 100644 --- a/src/libutil/thread-pool.hh +++ b/src/libutil/thread-pool.hh @@ -4,6 +4,7 @@ #include "error.hh" #include "sync.hh" +#include #include #include #include diff --git a/src/libutil/url.hh b/src/libutil/url.hh index d2413ec0e..a821301ba 100644 --- a/src/libutil/url.hh +++ b/src/libutil/url.hh @@ -2,6 +2,7 @@ ///@file #include "error.hh" +#include namespace nix { diff --git a/src/libutil/variant-wrapper.hh b/src/libutil/variant-wrapper.hh index cedcb999c..a809cd2a4 100644 --- a/src/libutil/variant-wrapper.hh +++ b/src/libutil/variant-wrapper.hh @@ -8,6 +8,7 @@ * Force the default versions of all constructors (copy, move, copy * assignment). */ +// NOLINTBEGIN(bugprone-macro-parentheses) #define FORCE_DEFAULT_CONSTRUCTORS(CLASS_NAME) \ CLASS_NAME(const CLASS_NAME &) = default; \ CLASS_NAME(CLASS_NAME &) = default; \ @@ -15,6 +16,7 @@ \ CLASS_NAME & operator =(const CLASS_NAME &) = default; \ CLASS_NAME & operator =(CLASS_NAME &) = default; +// NOLINTEND(bugprone-macro-parentheses) /** * Make a wrapper constructor. All args are forwarded to the diff --git a/tests/unit/libutil/url.cc b/tests/unit/libutil/url.cc index a908631e6..bfd9a228a 100644 --- a/tests/unit/libutil/url.cc +++ b/tests/unit/libutil/url.cc @@ -1,4 +1,5 @@ #include "url.hh" +#include "types.hh" #include namespace nix {