diff --git a/.clang-tidy b/.clang-tidy index ba265cc3f6..aefd43816f 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,5 +1,26 @@ -Checks: '-*, +Checks: [-*, bugprone-*, + + # Skipping these temporarily because they are very noisy + -bugprone-narrowing-conversions, + -bugprone-unchecked-optional-access, + + # The following cause either lots of pointless or advisory warnings -bugprone-easily-swappable-parameters, - clang-analyzer-*, - performance-*' + -bugprone-nondeterministic-pointer-iteration-order, + + # bifcl generates a lot of code with double underscores in their name. + # ZAM uses a few identifiers that start with underscores or have + # double-underscores in the name. + -bugprone-reserved-identifier, + + # bifcl generates almost every switch statement without a default case + # and so this one generates a lot of warnings. + -bugprone-switch-missing-default-case, + + # These report warnings that are rather difficult to fix. + -bugprone-undefined-memory-manipulation, + -bugprone-pointer-arithmetic-on-polymorphic-object, + -bugprone-empty-catch, + -bugprone-exception-escape +] diff --git a/CHANGES b/CHANGES index 40a9aa3027..f37f157c7a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +8.0.0-dev.252 | 2025-05-27 11:59:00 -0700 + + * Update .clang-tidy to have bugprone-* enabled with some exclusions (Tim Wojtulewicz, Corelight) + + * Fix clang-tidy bugprone-* warnings (Tim Wojtulewicz, Corelight) + 8.0.0-dev.229 | 2025-05-27 17:46:35 +0100 * Add explicit TLS support for FTP (Johanna Amann, Corelight) diff --git a/VERSION b/VERSION index 22162a0a57..7cb952d9fd 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -8.0.0-dev.229 +8.0.0-dev.252 diff --git a/auxil/binpac b/auxil/binpac index f7c94581f1..e1bb430912 160000 --- a/auxil/binpac +++ b/auxil/binpac @@ -1 +1 @@ -Subproject commit f7c94581f1f4d1bef1537824c27503d58000933e +Subproject commit e1bb4309129abc475250d36ca6822624cd3036bb diff --git a/src/Anon.cc b/src/Anon.cc index 022778c9c0..facc5dbbb2 100644 --- a/src/Anon.cc +++ b/src/Anon.cc @@ -54,7 +54,7 @@ static int bi_ffs(uint32_t value) { return add + bvals[value & 0xf]; } -#define first_n_bit_mask(n) (~(0xFFFFFFFFU >> n)) +static inline uint64_t first_n_bit_mask(int n) { return ~(0xFFFFFFFFU >> n); } ipaddr32_t AnonymizeIPAddr::Anonymize(ipaddr32_t addr) { std::map::iterator p = mapping.find(addr); diff --git a/src/CompHash.cc b/src/CompHash.cc index bd366e276c..c9cb324cdb 100644 --- a/src/CompHash.cc +++ b/src/CompHash.cc @@ -232,16 +232,8 @@ bool CompositeHash::RecoverOneVal(const HashKey& hk, Type* t, ValPtr* pval, bool if ( ! pvt ) reporter->InternalError("bad aggregate Val in CompositeHash::RecoverOneVal()"); - else if ( t->Tag() != TYPE_FUNC && ! same_type(pvt, t) ) - // ### Maybe fix later, but may be fundamentally un-checkable --US - { - reporter->InternalError("inconsistent aggregate Val in CompositeHash::RecoverOneVal()"); - *pval = nullptr; - return false; - } - - // ### A crude approximation for now. - else if ( t->Tag() == TYPE_FUNC && pvt->Tag() != TYPE_FUNC ) { + else if ( (t->Tag() != TYPE_FUNC && ! same_type(pvt, t)) || + (t->Tag() == TYPE_FUNC && pvt->Tag() != TYPE_FUNC) ) { reporter->InternalError("inconsistent aggregate Val in CompositeHash::RecoverOneVal()"); *pval = nullptr; return false; diff --git a/src/DNS_Mapping.cc b/src/DNS_Mapping.cc index 2500551606..a1299974ec 100644 --- a/src/DNS_Mapping.cc +++ b/src/DNS_Mapping.cc @@ -300,17 +300,17 @@ TEST_CASE("dns_mapping save reload") { // Try loading from the file at EOF. This should cause a mapping failure. DNS_Mapping mapping(tmpfile); CHECK(mapping.NoMapping()); - rewind(tmpfile); + fseek(tmpfile, 0, SEEK_SET); // Try reading from the empty file. This should cause an init failure. DNS_Mapping mapping2(tmpfile); CHECK(mapping2.InitFailed()); - rewind(tmpfile); + fseek(tmpfile, 0, SEEK_SET); // Save a valid mapping into the file and rewind to the start. DNS_Mapping mapping3(addr, &he, 123); mapping3.Save(tmpfile); - rewind(tmpfile); + fseek(tmpfile, 0, SEEK_SET); // Test loading the mapping back out of the file DNS_Mapping mapping4(tmpfile); diff --git a/src/DebugCmds.cc b/src/DebugCmds.cc index 11b7c18849..47136e299c 100644 --- a/src/DebugCmds.cc +++ b/src/DebugCmds.cc @@ -12,7 +12,7 @@ #include "zeek/DbgBreakpoint.h" #include "zeek/Debug.h" -#include "zeek/DebugCmdInfoConstants.cc" +#include "zeek/DebugCmdInfoConstants.cc" // NOLINT(bugprone-suspicious-include) #include "zeek/Desc.h" #include "zeek/Frame.h" #include "zeek/Func.h" @@ -147,7 +147,7 @@ int find_all_matching_cmds(const string& prefix, const char* array_of_matches[]) for ( int j = 0; j < g_DebugCmdInfos[i]->NumNames(); ++j ) { const char* curr_name = g_DebugCmdInfos[i]->Names()[j]; - if ( strncmp(curr_name, prefix.c_str(), arglen) ) + if ( strncmp(curr_name, prefix.c_str(), arglen) != 0 ) continue; // If exact match, then only return that one. diff --git a/src/Expr.cc b/src/Expr.cc index cddc9d800a..a0acb01b5e 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -892,11 +892,13 @@ ValPtr BinaryExpr::StringFold(Val* v1, Val* v2) const { switch ( tag ) { #undef DO_FOLD +// NOLINTBEGIN(bugprone-macro-parentheses) #define DO_FOLD(sense) \ { \ result = Bstr_cmp(s1, s2) sense 0; \ break; \ } + // NOLINTEND(bugprone-macro-parentheses) case EXPR_LT: DO_FOLD(<) case EXPR_LE: DO_FOLD(<=) @@ -1383,8 +1385,8 @@ AddExpr::AddExpr(ExprPtr arg_op1, ExprPtr arg_op2) : BinaryExpr(EXPR_ADD, std::m TypePtr base_result_type; - if ( bt2 == TYPE_INTERVAL && (bt1 == TYPE_TIME || bt1 == TYPE_INTERVAL) ) - base_result_type = base_type(bt1); + if ( (bt2 == TYPE_INTERVAL && (bt1 == TYPE_TIME || bt1 == TYPE_INTERVAL)) ) + base_result_type = base_type(bt1); // NOLINT(bugprone-branch-clone) else if ( bt2 == TYPE_TIME && bt1 == TYPE_INTERVAL ) base_result_type = base_type(bt2); else if ( BothArithmetic(bt1, bt2) ) @@ -1925,7 +1927,7 @@ CmpExpr::CmpExpr(ExprTag tag, ExprPtr _op1, ExprPtr _op2) : BinaryExpr(tag, std: void CmpExpr::Canonicalize() { if ( tag == EXPR_EQ || tag == EXPR_NE ) { if ( op2->GetType()->Tag() == TYPE_PATTERN ) - SwapOps(); + SwapOps(); //NOLINT(bugprone-branch-clone) else if ( op1->GetType()->Tag() == TYPE_PATTERN ) ; @@ -4082,10 +4084,13 @@ CallExpr::CallExpr(ExprPtr arg_func, ListExprPtr arg_args, bool in_hook, bool _i util::streq(((NameExpr*)func.get())->Id()->Name(), "fmt") && // The following is needed because fmt might not yet // be bound as a name. - did_builtin_init && (func_val = func->Eval(nullptr)) ) { - zeek::Func* f = func_val->AsFunc(); - if ( f->GetKind() == Func::BUILTIN_FUNC && ! check_built_in_call((BuiltinFunc*)f, this) ) - SetError(); + did_builtin_init ) { + func_val = func->Eval(nullptr); + if ( func_val ) { + zeek::Func* f = func_val->AsFunc(); + if ( f->GetKind() == Func::BUILTIN_FUNC && ! check_built_in_call((BuiltinFunc*)f, this) ) + SetError(); + } } } } diff --git a/src/Frag.cc b/src/Frag.cc index 8666e24b5c..9c2adbfaeb 100644 --- a/src/Frag.cc +++ b/src/Frag.cc @@ -158,7 +158,7 @@ void FragReassembler::Weird(const char* name) const { } void FragReassembler::Overlap(const u_char* b1, const u_char* b2, uint64_t n) { - if ( memcmp((const void*)b1, (const void*)b2, n) ) + if ( memcmp((const void*)b1, (const void*)b2, n) != 0 ) Weird("fragment_inconsistency"); else Weird("fragment_overlap"); diff --git a/src/Func.cc b/src/Func.cc index b484d1ca55..6aa53eefff 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -915,7 +915,7 @@ zeek::RecordValPtr make_backtrace_element(std::string_view name, const VectorVal static auto line_location_idx = elem_type->FieldOffset("line_location"); auto elem = make_intrusive(elem_type); - elem->Assign(function_name_idx, name.data()); + elem->Assign(function_name_idx, name); elem->Assign(function_args_idx, args); if ( loc ) { diff --git a/src/MMDB.cc b/src/MMDB.cc index 406ceee513..1160409a3e 100644 --- a/src/MMDB.cc +++ b/src/MMDB.cc @@ -118,7 +118,8 @@ bool MMDB::EnsureLoaded() { if ( ! res && ! reported_error ) { reported_error = true; - zeek::emit_builtin_error(zeek::util::fmt("Failed to open %s", Description().data())); + zeek::emit_builtin_error( + zeek::util::fmt("Failed to open %.*s", static_cast(Description().size()), Description().data())); } return res; diff --git a/src/PrefixTable.cc b/src/PrefixTable.cc index bc975c8fa4..a30dcc1568 100644 --- a/src/PrefixTable.cc +++ b/src/PrefixTable.cc @@ -69,7 +69,12 @@ std::list> PrefixTable::FindAll(const IPAddr& addr, out.emplace_back(PrefixToIPPrefix(list[i]->prefix), list[i]->data); Deref_Prefix(prefix); - free(list); + + // clang-tidy reports a bugprone-multi-level-implicit-pointer-conversion warning here + // because the double-pointer is implicitly converted to a void* for the call to + // free(). The double-pointer was calloc'd in patricia_search_all as an array of + // pointers, so it's safe to free. Explicitly cast it to void* to silence the warning. + free(static_cast(list)); return out; } diff --git a/src/RandTest.cc b/src/RandTest.cc index 3e94dd5556..9d53ddefe3 100644 --- a/src/RandTest.cc +++ b/src/RandTest.cc @@ -60,7 +60,7 @@ void RandTest::add(const void* buf, int bufl) { montey = 0; for ( int mj = 0; mj < RT_MONTEN / 2; mj++ ) { montex = (montex * 256.0) + monte[mj]; - montey = (montey * 256.0) + monte[(RT_MONTEN / 2) + mj]; + montey = (montey * 256.0) + monte[static_cast((RT_MONTEN / 2) + mj)]; } if ( montex * montex + montey * montey <= RT_INCIRC ) { inmont++; diff --git a/src/RandTest.h b/src/RandTest.h index e1f0682fe5..f422cdbbe3 100644 --- a/src/RandTest.h +++ b/src/RandTest.h @@ -4,11 +4,9 @@ #include -#define RT_MONTEN \ - 6 /* Bytes used as Monte Carlo \ - co-ordinates. This should be no more \ - bits than the mantissa of your "double" \ - floating point type. */ +// Bytes used as Monte Carlo co-ordinates. This should be no more bits than the mantissa +// of your "double" floating point type. +constexpr int RT_MONTEN = 6; namespace zeek { diff --git a/src/Reporter.cc b/src/Reporter.cc index 0cb93b8755..88a1502972 100644 --- a/src/Reporter.cc +++ b/src/Reporter.cc @@ -467,7 +467,7 @@ void Reporter::Deprecation(std::string_view msg, const detail::Location* loc1, c if ( loc1 || loc2 ) PushLocation(loc1, loc2); - Warning("%s", msg.data()); + Warning("%.*s", static_cast(msg.size()), msg.data()); if ( loc1 || loc2 ) PopLocation(); @@ -475,7 +475,8 @@ void Reporter::Deprecation(std::string_view msg, const detail::Location* loc1, c void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Connection* conn, ValPList* addl, bool location, bool time, const char* postfix, const char* fmt, va_list ap) { - static char tmp[512]; + constexpr size_t DEFAULT_BUFFER_SIZE = 512; + static char tmp[DEFAULT_BUFFER_SIZE]; int size = sizeof(tmp); char* buffer = tmp; @@ -484,7 +485,6 @@ void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Conne std::string loc_str; if ( location ) { - std::string loc_file = ""; int loc_line = 0; if ( locations.size() ) { @@ -522,27 +522,38 @@ void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Conne } } - while ( true ) { - va_list aq; - va_copy(aq, ap); - int n = vsnprintf(buffer, size, fmt, aq); - va_end(aq); + // Figure out how big of a buffer is needed here. + va_list ap_copy; + va_copy(ap_copy, ap); + size_t req_buffer_size = vsnprintf(nullptr, 0, fmt, ap_copy); - if ( postfix ) - n += strlen(postfix) + 10; // Add a bit of slack. - - if ( n > -1 && n < size ) - // We had enough space; - break; - - // Enlarge buffer; - size *= 2; - buffer = allocated = (char*)realloc(allocated, size); - - if ( ! buffer ) - FatalError("out of memory in Reporter"); + if ( req_buffer_size < 0 ) { + va_end(ap_copy); + FatalError("out of memory in Reporter"); } + if ( postfix && *postfix ) + req_buffer_size += strlen(postfix) + 10; + + // Add one byte for a null terminator. + req_buffer_size++; + + if ( req_buffer_size > DEFAULT_BUFFER_SIZE ) { + buffer = (char*)malloc(req_buffer_size); + if ( ! buffer ) { + va_end(ap_copy); + FatalError("out of memory in Reporter"); + } + + allocated = buffer; + } + + va_end(ap_copy); + + req_buffer_size = vsnprintf(buffer, req_buffer_size, fmt, ap); + if ( req_buffer_size < 0 ) + FatalError("out of memory in Reporter"); + if ( postfix && *postfix ) // Note, if you change this fmt string, adjust the additional // buffer size above. @@ -641,6 +652,7 @@ void Reporter::DoLog(const char* prefix, EventHandlerPtr event, FILE* out, Conne #endif } + // If the buffer got reallocated because it was too small, free the reallocated one. if ( allocated ) free(allocated); } diff --git a/src/SmithWaterman.cc b/src/SmithWaterman.cc index 30bece0476..1f6248bfc0 100644 --- a/src/SmithWaterman.cc +++ b/src/SmithWaterman.cc @@ -187,9 +187,9 @@ public: SWNode* operator()(int row, int col) { // Make sure access is in allowed range. - if ( row < 0 || row >= _rows ) + if ( row < 0 || static_cast(row) >= _rows ) return nullptr; - if ( col < 0 || col >= _cols ) + if ( col < 0 || static_cast(col) >= _cols ) return nullptr; return &(_nodes[row * _cols + col]); @@ -215,7 +215,7 @@ private: const String* _s1; const String* _s2; - int _rows, _cols; + size_t _rows, _cols; SWNode* _nodes; }; diff --git a/src/Stmt.cc b/src/Stmt.cc index 9d93992b97..b72ee06ade 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -1226,7 +1226,7 @@ ValPtr ForStmt::DoExec(Frame* f, Val* v, StmtFlowType& flow) { bool ForStmt::IsPure() const { return e->IsPure() && body->IsPure(); } void ForStmt::StmtDescribe(ODesc* d) const { - Stmt::StmtDescribe(d); + Stmt::StmtDescribe(d); // NOLINT(bugprone-parent-virtual-call) if ( d->IsReadable() ) d->Add("("); @@ -1398,7 +1398,7 @@ ValPtr ReturnStmt::Exec(Frame* f, StmtFlowType& flow) { } void ReturnStmt::StmtDescribe(ODesc* d) const { - Stmt::StmtDescribe(d); + Stmt::StmtDescribe(d); // NOLINT(bugprone-parent-virtual-call) if ( ! d->IsReadable() ) d->Add(e != nullptr); @@ -1607,7 +1607,7 @@ ValPtr AssertStmt::Exec(Frame* f, StmtFlowType& flow) { } void AssertStmt::StmtDescribe(ODesc* d) const { - Stmt::StmtDescribe(d); + Stmt::StmtDescribe(d); // NOLINT(bugprone-parent-virtual-call) // Quoting strings looks better when describing assert // statements. So turn it on explicitly. diff --git a/src/TraverseTypes.h b/src/TraverseTypes.h index 5a2cd4a613..abe157346a 100644 --- a/src/TraverseTypes.h +++ b/src/TraverseTypes.h @@ -14,20 +14,22 @@ enum TraversalCode { #define HANDLE_TC_STMT_PRE(code) \ { \ - if ( (code) == zeek::detail::TC_ABORTALL ) \ - return (code); \ - else if ( (code) == zeek::detail::TC_ABORTSTMT ) \ - return zeek::detail::TC_CONTINUE; \ + switch ( code ) { \ + case zeek::detail::TC_ABORTALL: return (code); \ + case zeek::detail::TC_ABORTSTMT: return zeek::detail::TC_CONTINUE; \ + case zeek::detail::TC_CONTINUE: \ + default: break; \ + } \ } #define HANDLE_TC_STMT_POST(code) \ { \ - if ( (code) == zeek::detail::TC_ABORTALL ) \ - return (code); \ - else if ( (code) == zeek::detail::TC_ABORTSTMT ) \ - return zeek::detail::TC_CONTINUE; \ - else \ - return (code); \ + switch ( code ) { \ + case zeek::detail::TC_ABORTSTMT: return zeek::detail::TC_CONTINUE; \ + case zeek::detail::TC_ABORTALL: \ + case zeek::detail::TC_CONTINUE: \ + default: return (code); \ + } \ } #define HANDLE_TC_EXPR_PRE(code) HANDLE_TC_STMT_PRE(code) diff --git a/src/Type.cc b/src/Type.cc index 5a5c62b627..a9a072a20e 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -422,8 +422,7 @@ static bool is_supported_index_type(const TypePtr& t, const char** tname, std::u switch ( tag ) { // Allow functions, since they can be compared for Func* pointer equality. - case TYPE_FUNC: return true; - + case TYPE_FUNC: case TYPE_PATTERN: return true; case TYPE_RECORD: { @@ -1843,7 +1842,7 @@ bool same_type(const Type& arg_t1, const Type& arg_t2, bool is_init, bool match_ // First do all checks that don't require any recursion. switch ( t1->Tag() ) { - case TYPE_VOID: + case TYPE_VOID: // NOLINT(bugprone-branch-clone) case TYPE_BOOL: case TYPE_INT: case TYPE_COUNT: @@ -2143,8 +2142,7 @@ bool is_assignable(TypeTag t) { case TYPE_FUNC: case TYPE_ANY: case TYPE_ERROR: - case TYPE_LIST: return true; - + case TYPE_LIST: case TYPE_VECTOR: case TYPE_FILE: case TYPE_OPAQUE: @@ -2157,10 +2155,6 @@ bool is_assignable(TypeTag t) { return false; } -#define CHECK_TYPE(t) \ - if ( t1 == t || t2 == t ) \ - return t; - TypeTag max_type(TypeTag t1, TypeTag t2) { if ( t1 == TYPE_INTERVAL || t1 == TYPE_TIME ) t1 = TYPE_DOUBLE; @@ -2168,9 +2162,12 @@ TypeTag max_type(TypeTag t1, TypeTag t2) { t2 = TYPE_DOUBLE; if ( BothArithmetic(t1, t2) ) { - CHECK_TYPE(TYPE_DOUBLE); - CHECK_TYPE(TYPE_INT); - CHECK_TYPE(TYPE_COUNT); + if ( t1 == TYPE_DOUBLE || t2 == TYPE_DOUBLE ) + return TYPE_DOUBLE; + else if ( t1 == TYPE_INT || t2 == TYPE_INT ) + return TYPE_INT; + else if ( t1 == TYPE_COUNT || t2 == TYPE_COUNT ) + return TYPE_COUNT; return TYPE_COUNT; } diff --git a/src/UID.h b/src/UID.h index 8c746e6cfe..35e49bc5fa 100644 --- a/src/UID.h +++ b/src/UID.h @@ -9,7 +9,7 @@ namespace zeek { -constexpr int UID_LEN = 2; +constexpr size_t UID_LEN = 2; /** * A class for creating/managing UIDs of arbitrary bit-length and converting diff --git a/src/Val.cc b/src/Val.cc index 451c9f6428..f98e6e708b 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -981,8 +981,8 @@ static zeek::expected BuildVal(const rapidjson::Value& j, c else if ( unit == "usec" || unit == "usecs" ) interval_secs += (value * Microseconds); else - return zeek::unexpected( - util::fmt("wrong interval format, invalid unit type %s", unit.data())); + return zeek::unexpected(util::fmt("wrong interval format, invalid unit type %.*s", + static_cast(unit.size()), unit.data())); } return make_intrusive(interval_secs, Seconds); diff --git a/src/Val.h b/src/Val.h index 6e2f4d7b60..cc606d70d5 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1186,6 +1186,7 @@ public: } void Assign(int field, const char* new_val) { Assign(field, new StringVal(new_val)); } void Assign(int field, const std::string& new_val) { Assign(field, new StringVal(new_val)); } + void Assign(int field, std::string_view new_val) { Assign(field, new StringVal(new_val)); } void Assign(int field, String* new_val) { Assign(field, new StringVal(new_val)); } /** diff --git a/src/Var.cc b/src/Var.cc index a93cdd1c75..e1bf55c67d 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -203,6 +203,7 @@ static void make_var(const IDPtr& id, TypePtr t, InitClass c, ExprPtr init, std: } if ( id->GetType() && id->GetType()->Tag() != TYPE_ERROR && ! id->IsBlank() ) { + // NOLINTNEXTLINE(bugprone-assignment-in-if-condition) if ( dt != VAR_REDEF && (! init || ! do_init || (! t && ! (t = init_type(init)))) ) { id->Error("already defined", init.get()); return; @@ -749,6 +750,7 @@ void end_func(StmtPtr body, const char* module_name, bool free_of_conditionals) // Note: ideally, something would take ownership of this memory until the // end of script execution, but that's essentially the same as the // lifetime of the process at the moment, so ok to "leak" it. + // NOLINTNEXTLINE(bugprone-unused-return-value) ingredients.release(); } diff --git a/src/analyzer/protocol/bittorrent/bittorrent-analyzer.pac b/src/analyzer/protocol/bittorrent/bittorrent-analyzer.pac index 36a6c50117..1d389ac806 100644 --- a/src/analyzer/protocol/bittorrent/bittorrent-analyzer.pac +++ b/src/analyzer/protocol/bittorrent/bittorrent-analyzer.pac @@ -38,7 +38,7 @@ flow BitTorrent_Flow(is_orig: bool) { function validate_handshake(pstrlen: uint8, pstr: const_bytestring): bool %{ if ( pstrlen != 19 || - memcmp("BitTorrent protocol", pstr.begin(), 19) ) + memcmp("BitTorrent protocol", pstr.begin(), 19) != 0 ) { throw Exception("invalid handshake"); } diff --git a/src/analyzer/protocol/dns/DNS.cc b/src/analyzer/protocol/dns/DNS.cc index 87b3f7fbd4..dc0132bbfd 100644 --- a/src/analyzer/protocol/dns/DNS.cc +++ b/src/analyzer/protocol/dns/DNS.cc @@ -924,18 +924,18 @@ bool DNS_Interpreter::ParseRR_RRSIG(detail::DNS_MsgInfo* msg, const u_char*& dat switch ( dsa ) { case detail::RSA_MD5: analyzer->Weird("DNSSEC_RRSIG_NotRecommended_ZoneSignAlgo", util::fmt("%d", algo)); break; - case detail::Diffie_Hellman: break; - case detail::DSA_SHA1: break; - case detail::Elliptic_Curve: break; - case detail::RSA_SHA1: break; - case detail::DSA_NSEC3_SHA1: break; - case detail::RSA_SHA1_NSEC3_SHA1: break; - case detail::RSA_SHA256: break; - case detail::RSA_SHA512: break; - case detail::GOST_R_34_10_2001: break; - case detail::ECDSA_curveP256withSHA256: break; - case detail::ECDSA_curveP384withSHA384: break; - case detail::Ed25519: break; + case detail::Diffie_Hellman: + case detail::DSA_SHA1: + case detail::Elliptic_Curve: + case detail::RSA_SHA1: + case detail::DSA_NSEC3_SHA1: + case detail::RSA_SHA1_NSEC3_SHA1: + case detail::RSA_SHA256: + case detail::RSA_SHA512: + case detail::GOST_R_34_10_2001: + case detail::ECDSA_curveP256withSHA256: + case detail::ECDSA_curveP384withSHA384: + case detail::Ed25519: case detail::Ed448: break; case detail::Indirect: analyzer->Weird("DNSSEC_RRSIG_Indirect_ZoneSignAlgo", util::fmt("%d", algo)); break; case detail::PrivateDNS: analyzer->Weird("DNSSEC_RRSIG_PrivateDNS_ZoneSignAlgo", util::fmt("%d", algo)); break; @@ -999,18 +999,18 @@ bool DNS_Interpreter::ParseRR_DNSKEY(detail::DNS_MsgInfo* msg, const u_char*& da case detail::RSA_MD5: analyzer->Weird("DNSSEC_DNSKEY_NotRecommended_ZoneSignAlgo", util::fmt("%d", dalgorithm)); break; - case detail::Diffie_Hellman: break; - case detail::DSA_SHA1: break; - case detail::Elliptic_Curve: break; - case detail::RSA_SHA1: break; - case detail::DSA_NSEC3_SHA1: break; - case detail::RSA_SHA1_NSEC3_SHA1: break; - case detail::RSA_SHA256: break; - case detail::RSA_SHA512: break; - case detail::GOST_R_34_10_2001: break; - case detail::ECDSA_curveP256withSHA256: break; - case detail::ECDSA_curveP384withSHA384: break; - case detail::Ed25519: break; + case detail::Diffie_Hellman: + case detail::DSA_SHA1: + case detail::Elliptic_Curve: + case detail::RSA_SHA1: + case detail::DSA_NSEC3_SHA1: + case detail::RSA_SHA1_NSEC3_SHA1: + case detail::RSA_SHA256: + case detail::RSA_SHA512: + case detail::GOST_R_34_10_2001: + case detail::ECDSA_curveP256withSHA256: + case detail::ECDSA_curveP384withSHA384: + case detail::Ed25519: case detail::Ed448: break; case detail::Indirect: analyzer->Weird("DNSSEC_DNSKEY_Indirect_ZoneSignAlgo", util::fmt("%d", dalgorithm)); @@ -1216,9 +1216,9 @@ bool DNS_Interpreter::ParseRR_DS(detail::DNS_MsgInfo* msg, const u_char*& data, String* ds_digest = ExtractStream(data, len, rdlength - 4); switch ( ds_digest_type ) { - case detail::SHA1: break; - case detail::SHA256: break; - case detail::GOST_R_34_11_94: break; + case detail::SHA1: + case detail::SHA256: + case detail::GOST_R_34_11_94: case detail::SHA384: break; case detail::reserved: analyzer->Weird("DNSSEC_DS_ReservedDigestType", util::fmt("%d", ds_dtype)); break; default: analyzer->Weird("DNSSEC_DS_unknown_DigestType", util::fmt("%d", ds_dtype)); break; diff --git a/src/analyzer/protocol/ftp/FTP.cc b/src/analyzer/protocol/ftp/FTP.cc index 368162005b..53be3f0452 100644 --- a/src/analyzer/protocol/ftp/FTP.cc +++ b/src/analyzer/protocol/ftp/FTP.cc @@ -309,25 +309,21 @@ void FTP_ADAT_Analyzer::DeliverStream(int len, const u_char* data, bool orig) { break; + // Server isn't going to accept named security mechanism. + // Client has to restart back at the AUTH. case 421: case 431: case 500: case 501: case 503: case 535: - // Server isn't going to accept named security mechanism. - // Client has to restart back at the AUTH. - done = true; - break; + // If the server is sending protected replies, the security + // data exchange must have already succeeded. It does have + // encoded data in the reply, but 632 and 633 are also encrypted. case 631: case 632: - case 633: - // If the server is sending protected replies, the security - // data exchange must have already succeeded. It does have - // encoded data in the reply, but 632 and 633 are also encrypted. - done = true; - break; + case 633: done = true; break; default: break; } diff --git a/src/analyzer/protocol/ftp/functions.bif b/src/analyzer/protocol/ftp/functions.bif index 5af6346b93..62cd318098 100644 --- a/src/analyzer/protocol/ftp/functions.bif +++ b/src/analyzer/protocol/ftp/functions.bif @@ -4,7 +4,7 @@ type ftp_port: record; %%{ #include "zeek/Reporter.h" -static zeek::RecordValPtr parse_port(std::string_view line) +static zeek::RecordValPtr parse_port(const std::string& line) { auto r = zeek::make_intrusive(zeek::BifType::Record::ftp_port); @@ -13,7 +13,7 @@ static zeek::RecordValPtr parse_port(std::string_view line) uint32_t addr = 0; int32_t bytes[6]; - if ( line.size() >= 11 && sscanf(line.data(), + if ( line.size() >= 11 && sscanf(line.c_str(), "%" SCNd32 ",%" SCNd32 ",%" SCNd32 ",%" SCNd32 ",%" SCNd32 ",%" SCNd32, &bytes[0], &bytes[1], &bytes[2], &bytes[3], &bytes[4], &bytes[5]) == 6 ) @@ -125,7 +125,7 @@ static zeek::RecordValPtr parse_eftp(const char* line) ## .. zeek:see:: parse_eftp_port parse_ftp_pasv parse_ftp_epsv fmt_ftp_port function parse_ftp_port%(s: string%): ftp_port %{ - return parse_port(s->ToStdStringView()); + return parse_port(s->ToStdString()); %} ## Converts a string representation of the FTP EPRT command (see :rfc:`2428`) @@ -160,20 +160,28 @@ function parse_ftp_pasv%(str: string%): ftp_port const char* line = strchr(s, '('); if ( line ) ++line; // move past '(' - else if ( (line = strstr(s, "PORT")) ) + else if ( line = strstr(s, "PORT"); line != nullptr ) line += 5; // Skip over - else if ( (line = strchr(s, ',')) ) + else if ( line = strchr(s, ','); line != nullptr ) { // Look for comma-separated list. - while ( --line >= s && isdigit(*line) ) - ; // Back up over preceding digits. - ++line; // now points to first digit, or beginning of s + do + { + // Back up over preceding digits. We'll end on the + // first digit or the beginning of s. + --line; + } + while ( line >= s && isdigit(*line) ); + + // Scoot forward one to point at the first digit or at the + // beginning of s. + ++line; } // Make sure we didn't step past the end of the string. if ( ! line || ( line - s ) > str->Len() ) return parse_port(""); else - return parse_port(std::string_view{line}); + return parse_port(std::string{line}); %} ## Converts the result of the FTP EPSV command (see :rfc:`2428`) to an diff --git a/src/analyzer/protocol/gnutella/Gnutella.cc b/src/analyzer/protocol/gnutella/Gnutella.cc index 9c68376300..6f0a7b3691 100644 --- a/src/analyzer/protocol/gnutella/Gnutella.cc +++ b/src/analyzer/protocol/gnutella/Gnutella.cc @@ -117,11 +117,11 @@ bool Gnutella_Analyzer::IsHTTP(std::string header) { } bool Gnutella_Analyzer::GnutellaOK(std::string header) { - if ( strncmp("GNUTELLA", header.data(), 8) ) + if ( strncmp("GNUTELLA", header.data(), 8) != 0 ) return false; int codepos = header.find(' ') + 1; - if ( ! strncmp("200", header.data() + codepos, 3) ) + if ( strncmp("200", header.data() + codepos, 3) == 0 ) return true; return false; diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index a5b80997ad..4aefbea232 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -103,7 +103,7 @@ void HTTP_Entity::Deliver(int len, const char* data, bool trailing_CRLF) { // Entity body. if ( content_type == analyzer::mime::CONTENT_TYPE_MULTIPART || content_type == analyzer::mime::CONTENT_TYPE_MESSAGE ) - DeliverBody(len, data, trailing_CRLF); + DeliverBody(len, data, trailing_CRLF); // NOLINT(bugprone-branch-clone) else if ( chunked_transfer_state != NON_CHUNKED_TRANSFER ) { switch ( chunked_transfer_state ) { @@ -1111,7 +1111,7 @@ const char* HTTP_Analyzer::PrefixMatch(const char* line, const char* end_of_line const char* HTTP_Analyzer::PrefixWordMatch(const char* line, const char* end_of_line, const char* prefix, bool ignore_case) { - if ( (line = PrefixMatch(line, end_of_line, prefix, ignore_case)) == nullptr ) + if ( line = PrefixMatch(line, end_of_line, prefix, ignore_case); line == nullptr ) return nullptr; const char* orig_line = line; diff --git a/src/analyzer/protocol/ident/Ident.cc b/src/analyzer/protocol/ident/Ident.cc index 378f9dafb7..1b6d11e2ba 100644 --- a/src/analyzer/protocol/ident/Ident.cc +++ b/src/analyzer/protocol/ident/Ident.cc @@ -142,7 +142,7 @@ void Ident_Analyzer::DeliverStream(int length, const u_char* data, bool is_orig) const char* sys_end = (comma && comma < colon) ? comma : colon; - while ( --sys_end > sys_type && isspace(*sys_end) ) + for ( ; sys_end > sys_type && isspace(*sys_end); --sys_end ) ; String* sys_type_s = new String((const u_char*)sys_type, sys_end - sys_type + 1, true); diff --git a/src/analyzer/protocol/imap/imap-protocol.pac b/src/analyzer/protocol/imap/imap-protocol.pac index b1964b2bb8..0088dbe06f 100644 --- a/src/analyzer/protocol/imap/imap-protocol.pac +++ b/src/analyzer/protocol/imap/imap-protocol.pac @@ -53,7 +53,6 @@ refine connection IMAP_Conn += { %{ string cmdstr = std_str(command); std::transform(cmdstr.begin(), cmdstr.end(), cmdstr.begin(), ::tolower); - string tagstr = std_str(tag); if ( !is_orig && cmdstr == "capability" && tag == "*" ) { return CMD_CAPABILITY; diff --git a/src/analyzer/protocol/irc/IRC.cc b/src/analyzer/protocol/irc/IRC.cc index 9981895cc1..a7f79ba5c2 100644 --- a/src/analyzer/protocol/irc/IRC.cc +++ b/src/analyzer/protocol/irc/IRC.cc @@ -280,9 +280,7 @@ void IRC_Analyzer::DeliverStream(int length, const u_char* line, bool orig) { int servers = 0; for ( size_t i = 1; i < parts.size(); ++i ) { - if ( parts[i] == "users," ) - users = atoi(parts[i - 1].c_str()); - else if ( parts[i] == "clients" ) + if ( parts[i] == "users," || parts[i] == "clients" ) users = atoi(parts[i - 1].c_str()); else if ( parts[i] == "services" ) services = atoi(parts[i - 1].c_str()); diff --git a/src/analyzer/protocol/login/NVT.cc b/src/analyzer/protocol/login/NVT.cc index 23eb513972..1543bdd79d 100644 --- a/src/analyzer/protocol/login/NVT.cc +++ b/src/analyzer/protocol/login/NVT.cc @@ -9,7 +9,7 @@ #include "zeek/analyzer/protocol/login/events.bif.h" #include "zeek/analyzer/protocol/tcp/TCP.h" -#define IS_3_BYTE_OPTION(c) (c >= 251 && c <= 254) +#define IS_3_BYTE_OPTION(c) ((c) >= 251 && (c) <= 254) #define TELNET_OPT_SB 250 #define TELNET_OPT_SE 240 diff --git a/src/analyzer/protocol/mime/MIME.cc b/src/analyzer/protocol/mime/MIME.cc index 71a36e9d8f..5965f70242 100644 --- a/src/analyzer/protocol/mime/MIME.cc +++ b/src/analyzer/protocol/mime/MIME.cc @@ -952,10 +952,7 @@ void MIME_Entity::DecodeQuotedPrintable(int len, const char* data) { else if ( (data[i] >= 33 && data[i] <= 60) || // except controls, whitespace and '=' - (data[i] >= 62 && data[i] <= 126) ) - DataOctet(data[i]); - - else if ( data[i] == HT || data[i] == SP ) + (data[i] >= 62 && data[i] <= 126) || (data[i] == HT || data[i] == SP) ) DataOctet(data[i]); else { diff --git a/src/analyzer/protocol/modbus/modbus-analyzer.pac b/src/analyzer/protocol/modbus/modbus-analyzer.pac index d53e2aa16e..03a124045f 100644 --- a/src/analyzer/protocol/modbus/modbus-analyzer.pac +++ b/src/analyzer/protocol/modbus/modbus-analyzer.pac @@ -114,7 +114,7 @@ refine flow ModbusTCP_Flow += { %} # EXCEPTION - function deliver_Exception(header: ModbusTCP_TransportHeader, message: Exception): bool + function deliver_Exception(header: ModbusTCP_TransportHeader, message: ModbusTCP_ExceptResponse): bool %{ if ( ::modbus_exception ) { diff --git a/src/analyzer/protocol/modbus/modbus-protocol.pac b/src/analyzer/protocol/modbus/modbus-protocol.pac index 4edf458ecf..d811806bbf 100644 --- a/src/analyzer/protocol/modbus/modbus-protocol.pac +++ b/src/analyzer/protocol/modbus/modbus-protocol.pac @@ -112,7 +112,7 @@ type ModbusTCP_Request(header: ModbusTCP_TransportHeader) = case header.fc of { # type ModbusTCP_Response(header: ModbusTCP_TransportHeader) = case header.fc & 0x80 of { 0 -> normal_response : ModbusTCP_NormalResponse(header); - default -> exception_response : Exception(header); + default -> exception_response : ModbusTCP_ExceptResponse(header); }; type ModbusTCP_NormalResponse(header: ModbusTCP_TransportHeader) = case header.fc of { @@ -140,7 +140,7 @@ type ModbusTCP_NormalResponse(header: ModbusTCP_TransportHeader) = case header.f default -> unknown: bytestring &restofdata; }; -type Exception(header: ModbusTCP_TransportHeader) = record { +type ModbusTCP_ExceptResponse(header: ModbusTCP_TransportHeader) = record { code: uint8; } &let { deliver: bool = $context.flow.deliver_Exception(header, this); diff --git a/src/analyzer/protocol/mysql/mysql-protocol.pac b/src/analyzer/protocol/mysql/mysql-protocol.pac index 41fbff5808..60ea7e240c 100644 --- a/src/analyzer/protocol/mysql/mysql-protocol.pac +++ b/src/analyzer/protocol/mysql/mysql-protocol.pac @@ -861,101 +861,47 @@ refine connection MySQL_Conn += { %{ switch ( cmd ) { case COM_SLEEP: - expected_ = EXPECT_STATUS; - break; case COM_QUIT: - expected_ = EXPECT_STATUS; - break; case COM_INIT_DB: + case COM_CREATE_DB: + case COM_DROP_DB: + case COM_REFRESH: + case COM_SHUTDOWN: + case COM_CONNECT: + case COM_PROCESS_KILL: + case COM_DEBUG: + case COM_PING: + case COM_TIME: + case COM_DELAYED_INSERT: + case COM_DAEMON: + case COM_RESET_CONNECTION: expected_ = EXPECT_STATUS; break; case COM_QUERY: + case COM_PROCESS_INFO: expected_ = EXPECT_COLUMN_COUNT; break; case COM_FIELD_LIST: expected_ = EXPECT_COLUMN_DEFINITION_OR_EOF; break; - case COM_CREATE_DB: - expected_ = EXPECT_STATUS; - break; - case COM_DROP_DB: - expected_ = EXPECT_STATUS; - break; - case COM_REFRESH: - expected_ = EXPECT_STATUS; - break; - case COM_SHUTDOWN: - expected_ = EXPECT_STATUS; - break; case COM_STATISTICS: expected_ = EXPECT_REST_OF_PACKET; break; - case COM_PROCESS_INFO: - expected_ = EXPECT_COLUMN_COUNT; - break; - case COM_CONNECT: - expected_ = EXPECT_STATUS; - break; - case COM_PROCESS_KILL: - expected_ = EXPECT_STATUS; - break; - case COM_DEBUG: - expected_ = EXPECT_STATUS; - break; - case COM_PING: - expected_ = EXPECT_STATUS; - break; - case COM_TIME: - expected_ = EXPECT_STATUS; - break; - case COM_DELAYED_INSERT: - expected_ = EXPECT_STATUS; - break; case COM_CHANGE_USER: update_state(CONNECTION_PHASE); break; case COM_BINLOG_DUMP: - expected_ = NO_EXPECTATION; - break; case COM_TABLE_DUMP: - expected_ = NO_EXPECTATION; - break; case COM_CONNECT_OUT: - expected_ = NO_EXPECTATION; - break; case COM_REGISTER_SLAVE: - expected_ = NO_EXPECTATION; - break; case COM_STMT_PREPARE: - expected_ = NO_EXPECTATION; - break; case COM_STMT_EXECUTE: - expected_ = NO_EXPECTATION; - break; case COM_STMT_SEND_LONG_DATA: - expected_ = NO_EXPECTATION; - break; case COM_STMT_CLOSE: - expected_ = NO_EXPECTATION; - break; case COM_STMT_RESET: - expected_ = NO_EXPECTATION; - break; case COM_SET_OPTION: - expected_ = NO_EXPECTATION; - break; case COM_STMT_FETCH: - expected_ = NO_EXPECTATION; - break; - case COM_DAEMON: - expected_ = EXPECT_STATUS; - break; case COM_BINLOG_DUMP_GTID: - expected_ = NO_EXPECTATION; - break; - case COM_RESET_CONNECTION: - expected_ = EXPECT_STATUS; - break; default: expected_ = NO_EXPECTATION; break; diff --git a/src/analyzer/protocol/netbios/NetbiosSSN.cc b/src/analyzer/protocol/netbios/NetbiosSSN.cc index 9218a7c2cb..c9106537b2 100644 --- a/src/analyzer/protocol/netbios/NetbiosSSN.cc +++ b/src/analyzer/protocol/netbios/NetbiosSSN.cc @@ -13,11 +13,11 @@ constexpr double netbios_ssn_session_timeout = 15.0; #define MAKE_INT16(dest, src) \ - dest = *src; \ - dest <<= 8; \ - src++; \ - dest |= *src; \ - src++; + (dest) = *(src); \ + (dest) <<= 8; \ + (src)++; \ + (dest) |= *(src); \ + (src)++; namespace zeek::analyzer::netbios_ssn { namespace detail { diff --git a/src/analyzer/protocol/rpc/MOUNT.cc b/src/analyzer/protocol/rpc/MOUNT.cc index c4b391fbad..8d39d07632 100644 --- a/src/analyzer/protocol/rpc/MOUNT.cc +++ b/src/analyzer/protocol/rpc/MOUNT.cc @@ -22,10 +22,8 @@ bool MOUNT_Interp::RPC_BuildCall(RPC_CallInfo* c, const u_char*& buf, int& n) { switch ( proc ) { case BifEnum::MOUNT3::PROC_NULL: break; - case BifEnum::MOUNT3::PROC_MNT: callarg = mount3_dirmntargs(buf, n); break; - - case BifEnum::MOUNT3::PROC_UMNT: callarg = mount3_dirmntargs(buf, n); break; - + case BifEnum::MOUNT3::PROC_MNT: + case BifEnum::MOUNT3::PROC_UMNT: case BifEnum::MOUNT3::PROC_UMNT_ALL: callarg = mount3_dirmntargs(buf, n); break; default: @@ -88,11 +86,6 @@ bool MOUNT_Interp::RPC_BuildReply(RPC_CallInfo* c, BifEnum::rpc_status rpc_statu break; case BifEnum::MOUNT3::PROC_UMNT: - n = 0; - mount_status = BifEnum::MOUNT3::MNT3_OK; - event = mount_proc_umnt; - break; - case BifEnum::MOUNT3::PROC_UMNT_ALL: n = 0; mount_status = BifEnum::MOUNT3::MNT3_OK; diff --git a/src/analyzer/protocol/rpc/NFS.cc b/src/analyzer/protocol/rpc/NFS.cc index de9feefe5c..c16ab6b231 100644 --- a/src/analyzer/protocol/rpc/NFS.cc +++ b/src/analyzer/protocol/rpc/NFS.cc @@ -27,7 +27,9 @@ bool NFS_Interp::RPC_BuildCall(RPC_CallInfo* c, const u_char*& buf, int& n) { case BifEnum::NFS3::PROC_SETATTR: callarg = nfs3_sattrargs(buf, n); break; - case BifEnum::NFS3::PROC_LOOKUP: callarg = nfs3_diropargs(buf, n); break; + case BifEnum::NFS3::PROC_LOOKUP: + case BifEnum::NFS3::PROC_REMOVE: + case BifEnum::NFS3::PROC_RMDIR: callarg = nfs3_diropargs(buf, n); break; case BifEnum::NFS3::PROC_READ: callarg = nfs3_readargs(buf, n); break; @@ -40,23 +42,13 @@ bool NFS_Interp::RPC_BuildCall(RPC_CallInfo* c, const u_char*& buf, int& n) { case BifEnum::NFS3::PROC_WRITE: callarg = nfs3_writeargs(buf, n); break; case BifEnum::NFS3::PROC_CREATE: - callarg = nfs3_diropargs(buf, n); - // TODO: implement create attributes. For now we just skip - // over them. - n = 0; - break; - case BifEnum::NFS3::PROC_MKDIR: callarg = nfs3_diropargs(buf, n); - // TODO: implement mkdir attributes. For now we just skip + // TODO: implement create and mkdir attributes. For now we just skip // over them. n = 0; break; - case BifEnum::NFS3::PROC_REMOVE: callarg = nfs3_diropargs(buf, n); break; - - case BifEnum::NFS3::PROC_RMDIR: callarg = nfs3_diropargs(buf, n); break; - case BifEnum::NFS3::PROC_RENAME: callarg = nfs3_renameopargs(buf, n); break; case BifEnum::NFS3::PROC_READDIR: callarg = nfs3_readdirargs(false, buf, n); break; diff --git a/src/analyzer/protocol/smtp/SMTP.cc b/src/analyzer/protocol/smtp/SMTP.cc index 616ebecd8b..8edc14b286 100644 --- a/src/analyzer/protocol/smtp/SMTP.cc +++ b/src/analyzer/protocol/smtp/SMTP.cc @@ -20,7 +20,7 @@ static const char* smtp_cmd_word[] = { static const char* unknown_cmd = "(UNKNOWN)"; -#define SMTP_CMD_WORD(code) ((code >= 0) ? smtp_cmd_word[code] : unknown_cmd) +#define SMTP_CMD_WORD(code) (((code) >= 0) ? smtp_cmd_word[code] : unknown_cmd) namespace zeek::analyzer::smtp { @@ -523,7 +523,7 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { state = detail::SMTP_RCPT_OK; break; - case 250: + case 250: // NOLINT(bugprone-branch-clone) case 251: // ?? Shall we catch 251? (RFC 2821) break; @@ -590,8 +590,7 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { state = detail::SMTP_IN_BDAT; break; - case 250: break; // server accepted BDAT transfer. - + case 250: // server accepted BDAT transfer. case 421: case 500: case 501: @@ -620,8 +619,7 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { state = detail::SMTP_AFTER_DATA; break; - case 250: break; - + case 250: case 421: case 451: case 452: @@ -668,8 +666,7 @@ void SMTP_Analyzer::UpdateState(int cmd_code, int reply_code, bool orig) { case 334: state = detail::SMTP_IN_AUTH; break; - case 235: state = detail::SMTP_INITIATED; break; - + case 235: case 432: case 454: case 501: diff --git a/src/analyzer/protocol/ssh/ssh-protocol.pac b/src/analyzer/protocol/ssh/ssh-protocol.pac index f0f1f37d79..73022b21e4 100644 --- a/src/analyzer/protocol/ssh/ssh-protocol.pac +++ b/src/analyzer/protocol/ssh/ssh-protocol.pac @@ -406,6 +406,7 @@ refine connection SSH_Conn += { version_ = version_server_; } // SSH1 vs SSH2 -> Undefined + // NOLINTBEGIN(bugprone-branch-clone) else if ( version_client_ == SSH1 && version_server_ == SSH2 ) version_ = UNK; // SSH2 vs SSH1 -> Undefined @@ -423,7 +424,7 @@ refine connection SSH_Conn += { // SSH199 vs SSH1 -> 1 else if ( version_client_ == SSH199 && version_server_ == SSH1 ) version_ = version_server_; - + // NOLINTEND(bugprone-branch-clone) return true; %} diff --git a/src/analyzer/protocol/ssl/SSL.cc b/src/analyzer/protocol/ssl/SSL.cc index bad6840c0e..d347465847 100644 --- a/src/analyzer/protocol/ssl/SSL.cc +++ b/src/analyzer/protocol/ssl/SSL.cc @@ -341,7 +341,7 @@ bool SSL_Analyzer::TryDecryptApplicationData(int len, const u_char* data, bool i decrypted.resize(decrypted_len); int res = 0; - if ( ! (res = EVP_DecryptFinal(ctx, NULL, &res)) ) { + if ( res = EVP_DecryptFinal(ctx, NULL, &res); res == 0 ) { DBG_LOG(DBG_ANALYZER, "Decryption failed with return code: %d. Invalid key?\n", res); EVP_CIPHER_CTX_free(ctx); return false; diff --git a/src/analyzer/protocol/tcp/TCP_Reassembler.cc b/src/analyzer/protocol/tcp/TCP_Reassembler.cc index bdfd655359..5af18879bc 100644 --- a/src/analyzer/protocol/tcp/TCP_Reassembler.cc +++ b/src/analyzer/protocol/tcp/TCP_Reassembler.cc @@ -375,7 +375,7 @@ void TCP_Reassembler::BlockInserted(DataBlockMap::const_iterator it) { // Our endpoint's peer doesn't do reassembly and so // (presumably) isn't processing acks. So don't hold // the now-delivered data. - TrimToSeq(last_reassem_seq); + TrimToSeq(last_reassem_seq); // NOLINT(bugprone-branch-clone) else if ( e->NoDataAcked() && zeek::detail::tcp_max_initial_window && e->Size() > static_cast(zeek::detail::tcp_max_initial_window) ) @@ -396,7 +396,7 @@ void TCP_Reassembler::Overlap(const u_char* b1, const u_char* b2, uint64_t n) { if ( DEBUG_tcp_contents ) DEBUG_MSG("%.6f TCP contents overlap: %" PRIu64 " IsOrig()=%d\n", run_state::network_time, n, IsOrig()); - if ( rexmit_inconsistency && memcmp((const void*)b1, (const void*)b2, n) && + if ( rexmit_inconsistency && (memcmp((const void*)b1, (const void*)b2, n) != 0) && // The following weeds out keep-alives for which that's all // we've ever seen for the connection. (n > 1 || endp->peer->HasDoneSomething()) ) { diff --git a/src/broker/Data.cc b/src/broker/Data.cc index 33d67a334c..cadc86fdd2 100644 --- a/src/broker/Data.cc +++ b/src/broker/Data.cc @@ -458,7 +458,7 @@ struct type_checker { result_type operator()(const std::string& a) { switch ( type->Tag() ) { - case TYPE_STRING: return true; + case TYPE_STRING: case TYPE_FILE: return true; default: return false; } diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index 2e36509a2f..dff6055e32 100644 --- a/src/broker/Manager.cc +++ b/src/broker/Manager.cc @@ -1590,7 +1590,7 @@ void Manager::ProcessMessage(std::string_view topic, broker::zeek::Event& ev) { if ( p.size() > topic.size() ) continue; - if ( strncmp(p.data(), topic.data(), p.size()) != 0 ) + if ( strncmp(p.data(), topic.data(), p.size()) != 0 ) // NOLINT(bugprone-suspicious-stringview-data-usage) continue; DBG_LOG(DBG_BROKER, "Skip processing of forwarded event: %s %s", std::string{name}.c_str(), @@ -1937,7 +1937,7 @@ void Manager::ProcessStoreResponse(detail::StoreHandleVal* s, broker::store::res BrokerData tmp{std::move(*response.answer)}; request->second->Result(detail::query_result(std::move(tmp).ToRecordVal())); } - else if ( response.answer.error() == broker::ec::request_timeout ) { + else if ( response.answer.error() == broker::ec::request_timeout ) { // NOLINT(bugprone-branch-clone) // Fine, trigger's timeout takes care of things. } else if ( response.answer.error() == broker::ec::stale_data ) { diff --git a/src/cluster/websocket/auxil/CMakeLists.txt b/src/cluster/websocket/auxil/CMakeLists.txt index 0fc5b821f7..f13a5f8e14 100644 --- a/src/cluster/websocket/auxil/CMakeLists.txt +++ b/src/cluster/websocket/auxil/CMakeLists.txt @@ -9,3 +9,7 @@ add_subdirectory(IXWebSocket) # We are not interested in any warnings from `IXWebSocket`'s compilation # itself, suppress them. target_compile_options(ixwebsocket PRIVATE "-w") + +# Disable clang-tidy and iwyu. +set_target_properties(ixwebsocket PROPERTIES CXX_INCLUDE_WHAT_YOU_USE "") +set_target_properties(ixwebsocket PROPERTIES CXX_CLANG_TIDY "") diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index 2dab26e25a..b12e95c89f 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -110,7 +110,10 @@ RecordValPtr X509::ParseCertificate(X509Val* cert_val, file_analysis::File* f) { auto pX509Cert = make_intrusive(BifType::Record::X509::Certificate); BIO* bio = BIO_new(BIO_s_mem()); + // The cast here is intentional to force it into a specific version of Assign() + // NOLINTNEXTLINE(bugprone-misplaced-widening-cast) pX509Cert->Assign(0, static_cast(X509_get_version(ssl_cert) + 1)); + i2a_ASN1_INTEGER(bio, X509_get_serialNumber(ssl_cert)); int len = BIO_read(bio, buf, sizeof(buf)); pX509Cert->Assign(1, make_intrusive(len, buf)); @@ -374,6 +377,8 @@ void X509::ParseSAN(X509_EXTENSION* ext) { emails->Assign(emails->Size(), std::move(bs)); break; + + default: break; } } diff --git a/src/file_analysis/analyzer/x509/X509Common.cc b/src/file_analysis/analyzer/x509/X509Common.cc index 2e916cff60..9722e393d5 100644 --- a/src/file_analysis/analyzer/x509/X509Common.cc +++ b/src/file_analysis/analyzer/x509/X509Common.cc @@ -128,8 +128,8 @@ double X509Common::GetTimeFromAsn1(const ASN1_TIME* atime, file_analysis::File* return 0; } - lSecondsFromUTC = ((pString[1] - '0') * 10 + (pString[2] - '0')) * 60; - lSecondsFromUTC += (pString[3] - '0') * 10 + (pString[4] - '0'); + lSecondsFromUTC = static_cast((pString[1] - '0') * 10) + static_cast((pString[2] - '0') * 60); + lSecondsFromUTC += static_cast((pString[3] - '0') * 10) + (pString[4] - '0'); if ( *pString == '-' ) lSecondsFromUTC = -lSecondsFromUTC; diff --git a/src/input/readers/raw/Raw.cc b/src/input/readers/raw/Raw.cc index 2660cc8912..6dec5c6bcd 100644 --- a/src/input/readers/raw/Raw.cc +++ b/src/input/readers/raw/Raw.cc @@ -607,7 +607,6 @@ bool Raw::DoUpdate() { } } - std::string line; assert((NumFields() == 1 && ! use_stderr) || (NumFields() == 2 && use_stderr)); for ( ;; ) { if ( stdin_towrite > 0 ) diff --git a/src/iosource/Manager.cc b/src/iosource/Manager.cc index 5b649f667c..12e072fb27 100644 --- a/src/iosource/Manager.cc +++ b/src/iosource/Manager.cc @@ -38,7 +38,7 @@ void Manager::WakeupHandler::Ping(std::string_view where) { // Calling DBG_LOG calls fprintf, which isn't safe to call in a signal // handler. if ( signal_val != 0 ) - DBG_LOG(DBG_MAINLOOP, "Pinging WakeupHandler from %s", where.data()); + DBG_LOG(DBG_MAINLOOP, "Pinging WakeupHandler from %.*s", static_cast(where.size()), where.data()); flare.Fire(true); } diff --git a/src/logging/Manager.cc b/src/logging/Manager.cc index c702750bb6..89dcb1fa13 100644 --- a/src/logging/Manager.cc +++ b/src/logging/Manager.cc @@ -340,7 +340,9 @@ Manager::Filter::~Filter() { for ( int i = 0; i < num_fields; ++i ) delete fields[i]; - free(fields); + // Static cast this to void* to avoid a clang-tidy warning about converting from the + // double-pointer to void* + free(static_cast(fields)); Unref(path_val); Unref(config); @@ -777,19 +779,8 @@ bool Manager::TraverseRecord(Stream* stream, Filter* filter, RecordType* rt, Tab continue; } - else if ( t->Tag() == TYPE_TABLE && t->AsTableType()->IsSet() ) { - // That's ok, we handle it below. - } - - else if ( t->Tag() == TYPE_VECTOR ) { - // That's ok, we handle it below. - } - - else if ( t->Tag() == TYPE_FILE ) { - // That's ok, we handle it below. - } - - else if ( t->Tag() == TYPE_FUNC ) { + else if ( (t->Tag() == TYPE_TABLE && t->AsTableType()->IsSet()) || t->Tag() == TYPE_VECTOR || + t->Tag() == TYPE_FILE || t->Tag() == TYPE_FUNC ) { // That's ok, we handle it below. } @@ -822,7 +813,9 @@ bool Manager::TraverseRecord(Stream* stream, Filter* filter, RecordType* rt, Tab // Alright, we want this field. filter->indices.push_back(new_indices); - void* tmp = realloc(filter->fields, sizeof(threading::Field*) * (filter->num_fields + 1)); + // Static cast this to void* to avoid a clang-tidy warning about converting from the + // double-pointer to void* + void* tmp = realloc(static_cast(filter->fields), sizeof(threading::Field*) * (filter->num_fields + 1)); if ( ! tmp ) { reporter->Error("out of memory in add_filter"); diff --git a/src/logging/writers/sqlite/SQLite.cc b/src/logging/writers/sqlite/SQLite.cc index 67de679e6d..02871c1cfd 100644 --- a/src/logging/writers/sqlite/SQLite.cc +++ b/src/logging/writers/sqlite/SQLite.cc @@ -68,12 +68,12 @@ string SQLite::GetTableType(int arg_type, int arg_subtype) { case TYPE_ENUM: case TYPE_STRING: case TYPE_FILE: - case TYPE_FUNC: type = "text"; break; - + case TYPE_FUNC: case TYPE_TABLE: case TYPE_VECTOR: - type = "text"; // dirty - but sqlite does not directly support arrays. so - we just roll - // it into a ","-separated string. + // For TYPE_TABLE and TYPE_VECTOR, this is sort of dirty, but sqlite does not + // directly support arrays. so we just roll it all into a comma-separated string. + type = "text"; break; default: diff --git a/src/module_util.cc b/src/module_util.cc index 7602f55f75..5b448be9dd 100644 --- a/src/module_util.cc +++ b/src/module_util.cc @@ -66,8 +66,8 @@ TEST_CASE("module_util normalized_module_name") { } string normalized_module_name(const char* module_name) { - int mod_len; - if ( (mod_len = strlen(module_name)) >= 2 && streq(module_name + mod_len - 2, "::") ) + int mod_len = strlen(module_name); + if ( (mod_len >= 2) && streq(module_name + mod_len - 2, "::") != 0 ) mod_len -= 2; return {module_name, static_cast(mod_len)}; diff --git a/src/net_util.cc b/src/net_util.cc index b756d07ed9..34a33bccf7 100644 --- a/src/net_util.cc +++ b/src/net_util.cc @@ -167,6 +167,7 @@ TEST_CASE("fmt_mac") { CHECK(my_fmt_mac("", 0) == ""); CHECK(my_fmt_mac("\x01\x02\x03\x04\x05\x06", 4) == ""); CHECK(my_fmt_mac("\x01\x02\x03\x04\x05\x06", 6) == "01:02:03:04:05:06"); + // NOLINTNEXTLINE(bugprone-string-literal-with-embedded-nul) CHECK(my_fmt_mac("\x01\x02\x03\x04\x05\x06\x00\x00", 8) == "01:02:03:04:05:06"); CHECK(my_fmt_mac("\x01\x02\x03\x04\x05\x06\x07\x08", 8) == "01:02:03:04:05:06:07:08"); CHECK(my_fmt_mac("\x08\x07\x06\x05\x04\x03\x02\x01", 8) == "08:07:06:05:04:03:02:01"); @@ -191,6 +192,7 @@ TEST_CASE("fmt_mac_bytes") { CHECK(len2 == 2 * 6 + 5); CHECK(memcmp(buf2.get(), "01:02:03:04:05:06", len2 + 1) == 0); + // NOLINTNEXTLINE(bugprone-string-literal-with-embedded-nul) auto [buf3, len3] = my_fmt_mac_bytes("\x01\x02\x03\x04\x05\x06\x00\x00", 8); CHECK(len3 == 2 * 6 + 5); CHECK(memcmp(buf3.get(), "01:02:03:04:05:06", len3 + 1) == 0); diff --git a/src/packet_analysis/protocol/arp/ARP.cc b/src/packet_analysis/protocol/arp/ARP.cc index 43fd5d776f..d92d003c37 100644 --- a/src/packet_analysis/protocol/arp/ARP.cc +++ b/src/packet_analysis/protocol/arp/ARP.cc @@ -60,7 +60,7 @@ ARPAnalyzer::ARPAnalyzer() : zeek::packet_analysis::Analyzer("ARP") {} #endif #ifndef ar_tpa -#define ar_tpa(ap) ((caddr_t((ap) + 1)) + 2 * (ap)->ar_hln + (ap)->ar_pln) +#define ar_tpa(ap) ((caddr_t((ap) + 1)) + 2 * static_cast((ap)->ar_hln) + (ap)->ar_pln) #endif #ifndef ARPOP_REVREQUEST diff --git a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc index f1f5cd45f3..714782eb8e 100644 --- a/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc +++ b/src/packet_analysis/protocol/tcp/TCPSessionAdapter.cc @@ -57,7 +57,7 @@ void TCPSessionAdapter::Init() { } void TCPSessionAdapter::Done() { - Analyzer::Done(); + IP::SessionAdapter::Done(); if ( run_state::terminating && connection_pending && is_active && ! BothClosed() ) Event(connection_pending); @@ -299,7 +299,7 @@ static zeek::RecordValPtr build_syn_packet_val(bool is_orig, const zeek::IP_Hdr* // Parse TCP options. u_char* options = (u_char*)tcp + sizeof(struct tcphdr); - u_char* opt_end = (u_char*)tcp + tcp->th_off * 4; + u_char* opt_end = (u_char*)tcp + static_cast(tcp->th_off * 4); while ( options < opt_end ) { unsigned int opt = options[0]; @@ -460,23 +460,27 @@ static int32_t update_last_seq(analyzer::tcp::TCP_Endpoint* endpoint, uint32_t l // ### perhaps trust RST seq #'s if initial and not too // outlandish, but not if they're coming after the other // side has sent a FIN - trust the FIN ack instead - ; + ; // NOLINT(bugprone-branch-clone) + else if ( flags.FIN() && endpoint->LastSeq() == endpoint->StartSeq() + 1 ) // Update last_seq based on the FIN even if delta_last < 0. // This is to accommodate > 2 GB connections for which // we've only seen the SYN and the FIN (hence the check // for last_seq == start_seq + 1). - endpoint->UpdateLastSeq(last_seq); + endpoint->UpdateLastSeq(last_seq); // NOLINT(bugprone-branch-clone) + else if ( endpoint->state == analyzer::tcp::TCP_ENDPOINT_RESET ) // don't trust any subsequent sequence numbers - ; + ; // NOLINT(bugprone-branch-clone) + else if ( delta_last > 0 ) // ### check for large jumps here. // ## endpoint->last_seq = last_seq; - endpoint->UpdateLastSeq(last_seq); + endpoint->UpdateLastSeq(last_seq); // NOLINT(bugprone-branch-clone) + else if ( delta_last <= 0 && len > 0 ) endpoint->DidRxmit(); @@ -1458,7 +1462,7 @@ void TCPSessionAdapter::SynWeirds(analyzer::tcp::TCP_Flags flags, analyzer::tcp: int TCPSessionAdapter::ParseTCPOptions(const struct tcphdr* tcp, bool is_orig) { // Parse TCP options. const u_char* options = (const u_char*)tcp + sizeof(struct tcphdr); - const u_char* opt_end = (const u_char*)tcp + tcp->th_off * 4; + const u_char* opt_end = (const u_char*)tcp + static_cast(tcp->th_off * 4); std::vector opts; while ( options < opt_end ) { diff --git a/src/probabilistic/Hasher.cc b/src/probabilistic/Hasher.cc index 4805cc8f48..632b3543aa 100644 --- a/src/probabilistic/Hasher.cc +++ b/src/probabilistic/Hasher.cc @@ -71,6 +71,8 @@ std::unique_ptr Hasher::Unserialize(BrokerDataView data) { case Default: hasher.reset(new DefaultHasher(k, {h1, h2})); break; case Double: hasher.reset(new DoubleHasher(k, {h1, h2})); break; + + default: break; } // Note that the derived classed don't hold any further state of diff --git a/src/script_opt/CPP/Consts.cc b/src/script_opt/CPP/Consts.cc index d29f1d6b5f..919533edd0 100644 --- a/src/script_opt/CPP/Consts.cc +++ b/src/script_opt/CPP/Consts.cc @@ -63,7 +63,6 @@ shared_ptr CPPCompile::RegisterConstant(const ValPtr& vp, int& con } auto tag = t->Tag(); - auto const_name = const_info[tag]->NextName(); shared_ptr gi; switch ( tag ) { @@ -73,14 +72,11 @@ shared_ptr CPPCompile::RegisterConstant(const ValPtr& vp, int& con case TYPE_COUNT: gi = make_shared(to_string(vp->AsCount()) + "ULL"); break; - case TYPE_DOUBLE: gi = make_shared(to_string(vp->AsDouble())); break; - - case TYPE_TIME: gi = make_shared(to_string(vp->AsDouble())); break; - + case TYPE_DOUBLE: + case TYPE_TIME: case TYPE_INTERVAL: gi = make_shared(to_string(vp->AsDouble())); break; - case TYPE_ADDR: gi = make_shared(this, vp); break; - + case TYPE_ADDR: case TYPE_SUBNET: gi = make_shared(this, vp); break; case TYPE_ENUM: gi = make_shared(this, vp); break; diff --git a/src/script_opt/CPP/Exprs.cc b/src/script_opt/CPP/Exprs.cc index c9ad33122c..6c8fc1579f 100644 --- a/src/script_opt/CPP/Exprs.cc +++ b/src/script_opt/CPP/Exprs.cc @@ -1241,7 +1241,6 @@ string CPPCompile::GenField(const ExprPtr& rec, int field) { auto pt = processed_types.find(rt); ASSERT(pt != processed_types.end()); auto rt_offset = pt->second->Offset(); - string field_name = rt->FieldName(field); field_decls.emplace_back(rt_offset, rt->FieldDecl(field)); if ( rfm != record_field_mappings.end() ) diff --git a/src/script_opt/CPP/InitsInfo.cc b/src/script_opt/CPP/InitsInfo.cc index 2500b42806..d63e1a45de 100644 --- a/src/script_opt/CPP/InitsInfo.cc +++ b/src/script_opt/CPP/InitsInfo.cc @@ -491,7 +491,6 @@ ListTypeInfo::ListTypeInfo(CPPCompile* _c, TypePtr _t) } void ListTypeInfo::AddInitializerVals(std::vector& ivs) const { - string type_list; for ( auto& t : types ) { auto iv = Fmt(c->TypeOffset(t)); ivs.emplace_back(iv); diff --git a/src/script_opt/CPP/RuntimeInitSupport.cc b/src/script_opt/CPP/RuntimeInitSupport.cc index ed361bcd06..bf342ce006 100644 --- a/src/script_opt/CPP/RuntimeInitSupport.cc +++ b/src/script_opt/CPP/RuntimeInitSupport.cc @@ -211,9 +211,11 @@ IDPtr find_global__CPP(const char* g) { RecordTypePtr get_record_type__CPP(const char* record_type_name) { IDPtr existing_type; - if ( record_type_name && (existing_type = global_scope()->Find(record_type_name)) && - existing_type->GetType()->Tag() == TYPE_RECORD ) - return cast_intrusive(existing_type->GetType()); + if ( record_type_name ) { + IDPtr existing_type = global_scope()->Find(record_type_name); + if ( existing_type && existing_type->GetType()->Tag() == TYPE_RECORD ) + return cast_intrusive(existing_type->GetType()); + } return make_intrusive(new type_decl_list()); } diff --git a/src/script_opt/CPP/RuntimeVec.cc b/src/script_opt/CPP/RuntimeVec.cc index 7c6e6ab717..e7d61b223d 100644 --- a/src/script_opt/CPP/RuntimeVec.cc +++ b/src/script_opt/CPP/RuntimeVec.cc @@ -56,6 +56,7 @@ static VectorTypePtr base_vector_type__CPP(const VectorTypePtr& vt, bool is_bool // is an optional kernel to use for vectors whose underlying type // is "double". It needs to be optional because C++ will (rightfully) // complain about applying certain C++ unary operations to doubles. +// NOLINTBEGIN(bugprone-macro-parentheses) #define VEC_OP1(name, op, double_kernel) \ VectorValPtr vec_op_##name##__CPP(const VectorValPtr& v, const TypePtr& t) { \ auto vt = base_vector_type__CPP(cast_intrusive(t)); \ @@ -79,6 +80,7 @@ static VectorTypePtr base_vector_type__CPP(const VectorTypePtr& vt, bool is_bool \ return v_result; \ } +// NOLINTEND(bugprone-macro-parentheses) // Instantiates a double_kernel for a given operation. #define VEC_OP1_WITH_DOUBLE(name, op) \ @@ -96,6 +98,7 @@ VEC_OP1(comp, ~, ) // A kernel for applying a binary operation element-by-element to two // vectors of a given low-level type. +// NOLINTBEGIN(bugprone-macro-parentheses) #define VEC_OP2_KERNEL(accessor, type, op, zero_check) \ for ( unsigned int i = 0; i < v1->Size(); ++i ) { \ auto v1_i = v1->ValAt(i); \ @@ -107,6 +110,7 @@ VEC_OP1(comp, ~, ) v_result->Assign(i, make_intrusive(v1_i->accessor() op v2_i->accessor())); \ } \ } +// NOLINTEND(bugprone-macro-parentheses) // Analogous to VEC_OP1, instantiates a function for a given binary operation, // with customizable kernels for "int" and "double" operations. diff --git a/src/script_opt/CPP/Types.cc b/src/script_opt/CPP/Types.cc index 42f4c7ec82..93093b7aaa 100644 --- a/src/script_opt/CPP/Types.cc +++ b/src/script_opt/CPP/Types.cc @@ -32,11 +32,9 @@ bool CPPCompile::IsNativeType(const TypePtr& t) const { case TYPE_SUBNET: case TYPE_TABLE: case TYPE_TYPE: - case TYPE_VECTOR: return false; - - case TYPE_LIST: - // These occur when initializing tables. - return false; + case TYPE_VECTOR: + // These occur when initializing tables. + case TYPE_LIST: return false; default: reporter->InternalError("bad type in CPPCompile::IsNativeType"); return false; } @@ -114,7 +112,7 @@ const char* CPPCompile::TypeName(const TypePtr& t) { case TYPE_BOOL: return "bool"; case TYPE_COUNT: return "zeek_uint_t"; case TYPE_DOUBLE: return "double"; - case TYPE_ENUM: return "zeek_int_t"; + case TYPE_ENUM: case TYPE_INT: return "zeek_int_t"; case TYPE_INTERVAL: return "double"; case TYPE_PORT: return "zeek_uint_t"; diff --git a/src/script_opt/CSE.cc b/src/script_opt/CSE.cc index de2d542a40..e1bf0518c7 100644 --- a/src/script_opt/CSE.cc +++ b/src/script_opt/CSE.cc @@ -255,7 +255,6 @@ bool CSE_ValidityChecker::CheckTableRef(const TypePtr& t) { return CheckSideEffe bool CSE_ValidityChecker::CheckCall(const CallExpr* c) { auto func = c->Func(); - std::string desc; if ( func->Tag() != EXPR_NAME ) // Can't analyze indirect calls. return Invalid(); diff --git a/src/script_opt/GenIDDefs.cc b/src/script_opt/GenIDDefs.cc index 6a44694c6f..83e4239069 100644 --- a/src/script_opt/GenIDDefs.cc +++ b/src/script_opt/GenIDDefs.cc @@ -265,11 +265,9 @@ TraversalCode GenIDDefs::PostStmt(const Stmt* s) { break; } + // No need to do anything, the work all occurs + // with NoFlowAfter. case STMT_FALLTHROUGH: - // No need to do anything, the work all occurs - // with NoFlowAfter. - break; - default: break; } @@ -379,15 +377,13 @@ bool GenIDDefs::CheckLHS(const ExprPtr& lhs, const ExprPtr& rhs) { return true; } + // If we want to track record field initializations, + // we'd handle that here. case EXPR_FIELD: - // If we want to track record field initializations, - // we'd handle that here. - return false; - case EXPR_INDEX: - // If we wanted to track potential alterations of - // aggregates, we'd do that here. - return false; + // If we wanted to track potential alterations of + // aggregates, we'd do that here. + case EXPR_INDEX: return false; default: reporter->InternalError("bad tag in GenIDDefs::CheckLHS"); } diff --git a/src/script_opt/IDOptInfo.cc b/src/script_opt/IDOptInfo.cc index fbec619bf7..b1fa228e13 100644 --- a/src/script_opt/IDOptInfo.cc +++ b/src/script_opt/IDOptInfo.cc @@ -323,14 +323,12 @@ void IDOptInfo::ConfluenceBlockEndsAfter(const Stmt* s, bool no_orig_flow) { continue; } - else if ( ur.EndsAfter() < cs_stmt_num ) + else if ( ur.EndsAfter() < cs_stmt_num || pc.count(i) == 0 ) // Irrelevant, didn't extend into confluence region. // We test here just to avoid the set lookup in // the next test, which presumably will sometimes // be a tad expensive. - continue; - - else if ( pc.count(i) == 0 ) + // or // This region isn't active, and we're not // tracking it for confluence. continue; @@ -440,13 +438,10 @@ int IDOptInfo::FindRegionBeforeIndex(int stmt_num) { if ( ur.StartsAfter() >= stmt_num ) break; - if ( ur.EndsAfter() == NO_DEF ) - // It's active for everything beyond its start. - region_ind = i; - - else if ( ur.EndsAfter() >= stmt_num - 1 ) - // It's active at the beginning of the statement of - // interest. + // It's active for everything beyond its start. + // or + // It's active at the beginning of the statement of interest. + if ( (ur.EndsAfter() == NO_DEF) || (ur.EndsAfter() >= stmt_num - 1) ) region_ind = i; } diff --git a/src/script_opt/Reduce.cc b/src/script_opt/Reduce.cc index 344f8ec07b..bcb2044e3a 100644 --- a/src/script_opt/Reduce.cc +++ b/src/script_opt/Reduce.cc @@ -481,10 +481,8 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const { auto aggr = e1->GetOp1(); auto aggr_t = aggr->GetType(); - if ( pfs->HasSideEffects(SideEffectsOp::READ, aggr_t) ) - has_side_effects = true; - - else if ( aggr_t->Tag() == TYPE_TABLE && pfs->IsTableWithDefaultAggr(aggr_t.get()) ) + if ( (pfs->HasSideEffects(SideEffectsOp::READ, aggr_t)) || + (aggr_t->Tag() == TYPE_TABLE && pfs->IsTableWithDefaultAggr(aggr_t.get())) ) has_side_effects = true; } diff --git a/src/script_opt/Stmt.cc b/src/script_opt/Stmt.cc index 3e45161d7c..977e998864 100644 --- a/src/script_opt/Stmt.cc +++ b/src/script_opt/Stmt.cc @@ -1281,7 +1281,7 @@ StmtPtr CheckAnyLenStmt::DoReduce(Reducer* c) { } void CheckAnyLenStmt::StmtDescribe(ODesc* d) const { - Stmt::StmtDescribe(d); + Stmt::StmtDescribe(d); // NOLINT(bugprone-parent-virtual-call) e->Describe(d); if ( ! d->IsBinary() ) diff --git a/src/script_opt/ZAM/BuiltInSupport.cc b/src/script_opt/ZAM/BuiltInSupport.cc index 927c07e615..2a66fd274a 100644 --- a/src/script_opt/ZAM/BuiltInSupport.cc +++ b/src/script_opt/ZAM/BuiltInSupport.cc @@ -12,9 +12,6 @@ FixedCatArg::FixedCatArg(TypePtr _t) : t(std::move(_t)) { case TYPE_BOOL: max_size = 1; break; case TYPE_INT: - max_size = 20; // sufficient for 64 bits - break; - case TYPE_COUNT: max_size = 20; // sufficient for 64 bits break; diff --git a/src/script_opt/ZAM/ZBody.cc b/src/script_opt/ZAM/ZBody.cc index 689f269730..4cb96716ce 100644 --- a/src/script_opt/ZAM/ZBody.cc +++ b/src/script_opt/ZAM/ZBody.cc @@ -202,7 +202,7 @@ static void vec_exec(ZOp op, TypePtr t, VectorVal*& v1, const VectorVal* v2, con auto vi = (*v[i]).rhs_accessor; \ if ( ov_check(vi) ) { \ std::string err = "overflow promoting from "; \ - err += ov_err; \ + err += (ov_err); \ err += " arithmetic value"; \ /* The run-time error will throw an exception, so recover intermediary memory. */ \ delete res_zv; \ diff --git a/src/session/Key.cc b/src/session/Key.cc index 669ddb4c61..91eef2bdb3 100644 --- a/src/session/Key.cc +++ b/src/session/Key.cc @@ -65,9 +65,7 @@ bool Key::operator<(const Key& rhs) const { } bool Key::operator==(const Key& rhs) const { - if ( size != rhs.size ) - return false; - else if ( type != rhs.type ) + if ( size != rhs.size || type != rhs.type ) return false; return memcmp(data, rhs.data, size) == 0; diff --git a/src/spicy/runtime-support.cc b/src/spicy/runtime-support.cc index ec9d133a9d..76cf8c1f59 100644 --- a/src/spicy/runtime-support.cc +++ b/src/spicy/runtime-support.cc @@ -236,7 +236,7 @@ zeek::analyzer::ID rt::current_analyzer_id() { if ( auto x = cookie->protocol ) { return x->analyzer->GetID(); } - else if ( auto x = cookie->file ) { + else if ( auto x = cookie->file ) { // NOLINT(bugprone-branch-clone) return 0; } else if ( auto x = cookie->packet ) { diff --git a/src/storage/backend/redis/Redis.cc b/src/storage/backend/redis/Redis.cc index ace0a7645a..67d2f3ca93 100644 --- a/src/storage/backend/redis/Redis.cc +++ b/src/storage/backend/redis/Redis.cc @@ -246,7 +246,7 @@ OperationResult Redis::DoOpen(OpenResultCallback* cb, RecordValPtr options) { StringValPtr host = backend_options->GetField("server_host"); if ( host ) { PortValPtr port = backend_options->GetField("server_port"); - server_addr = util::fmt("%s:%d", host->ToStdStringView().data(), port->Port()); + server_addr = util::fmt("%s:%d", host->ToStdString().c_str(), port->Port()); REDIS_OPTIONS_SET_TCP(&opt, host->ToStdStringView().data(), port->Port()); } else { @@ -695,10 +695,12 @@ OperationResult Redis::ParseReplyError(std::string_view op_str, std::string_view if ( async_ctx->err == REDIS_ERR_TIMEOUT ) return {ReturnCode::TIMEOUT}; else if ( async_ctx->err == REDIS_ERR_IO ) - return {ReturnCode::OPERATION_FAILED, util::fmt("%s operation IO error: %s", op_str.data(), strerror(errno))}; + return {ReturnCode::OPERATION_FAILED, util::fmt("%.*s operation IO error: %s", static_cast(op_str.size()), + op_str.data(), strerror(errno))}; else return {ReturnCode::OPERATION_FAILED, - util::fmt("%s operation failed: %s", op_str.data(), reply_err_str.data())}; + util::fmt("%.*s operation failed: %.*s", static_cast(op_str.size()), op_str.data(), + static_cast(reply_err_str.size()), reply_err_str.data())}; } void Redis::DoPoll() { diff --git a/src/storage/serializer/json/JSON.cc b/src/storage/serializer/json/JSON.cc index ecc4296e37..7140aa603f 100644 --- a/src/storage/serializer/json/JSON.cc +++ b/src/storage/serializer/json/JSON.cc @@ -35,8 +35,9 @@ zeek::expected JSON::Unserialize(byte_buffer_span buf, Type std::string_view version = std::string_view(text).substr(0, semicolon); if ( version != versioned_name ) - return zeek::unexpected( - util::fmt("Version doesn't match: %s vs %s", version.data(), versioned_name.c_str())); + return zeek::unexpected(util::fmt("Version doesn't match: %.*s vs %s", + static_cast(version.size()), version.data(), + versioned_name.c_str())); return zeek::detail::ValFromJSON(text.substr(semicolon + 1), type, Func::nil); } diff --git a/src/telemetry/Manager.cc b/src/telemetry/Manager.cc index 25405c2fb2..0772879581 100644 --- a/src/telemetry/Manager.cc +++ b/src/telemetry/Manager.cc @@ -301,7 +301,8 @@ ValPtr Manager::CollectMetrics(std::string_view prefix_pattern, std::string_view // Due to the name containing the full information about a metric including a potential unit add an // asterisk to the end of the full pattern so matches work correctly. - std::string full_pattern = util::fmt("%s_%s", prefix_pattern.data(), name_pattern.data()); + std::string full_pattern = util::fmt("%.*s_%.*s", static_cast(prefix_pattern.size()), prefix_pattern.data(), + static_cast(name_pattern.size()), name_pattern.data()); if ( full_pattern[full_pattern.size() - 1] != '*' ) full_pattern.append("*"); @@ -380,7 +381,8 @@ ValPtr Manager::CollectHistogramMetrics(std::string_view prefix_pattern, std::st // Due to the name containing the full information about a metric including a potential unit add an // asterisk to the end of the full pattern so matches work correctly. - std::string full_pattern = util::fmt("%s_%s", prefix_pattern.data(), name_pattern.data()); + std::string full_pattern = util::fmt("%.*s_%.*s", static_cast(prefix_pattern.size()), prefix_pattern.data(), + static_cast(name_pattern.size()), name_pattern.data()); if ( full_pattern[full_pattern.size() - 1] != '*' ) full_pattern.append("*"); diff --git a/src/telemetry/Utils.cc b/src/telemetry/Utils.cc index f099252500..c7cca4ec84 100644 --- a/src/telemetry/Utils.cc +++ b/src/telemetry/Utils.cc @@ -16,7 +16,8 @@ std::string BuildFullPrometheusName(std::string_view prefix, std::string_view na if ( prefix.empty() || name.empty() ) reporter->FatalError("Telemetry metric families must have a non-zero-length prefix and name"); - std::string fn = util::fmt("%s_%s", prefix.data(), name.data()); + std::string fn = util::fmt("%.*s_%.*s", static_cast(prefix.size()), prefix.data(), + static_cast(name.size()), name.data()); std::for_each(fn.begin(), fn.end(), [](char& c) { if ( ! std::isalnum(c) ) c = '_'; diff --git a/src/threading/SerialTypes.cc b/src/threading/SerialTypes.cc index 6ae211c3bc..53560ee572 100644 --- a/src/threading/SerialTypes.cc +++ b/src/threading/SerialTypes.cc @@ -245,8 +245,8 @@ bool Value::Read(detail::SerializationFormat* fmt) { switch ( family ) { case 4: val.addr_val.family = IPv4; return fmt->Read(&val.addr_val.in.in4, "addr-in4"); - case 6: val.addr_val.family = IPv6; return fmt->Read(&val.addr_val.in.in6, "addr-in6"); + default: reporter->Warning("Unknown family type %d when reading addr\n", family); break; } // Can't be reached. @@ -270,6 +270,7 @@ bool Value::Read(detail::SerializationFormat* fmt) { val.subnet_val.length = (uint8_t)length; val.subnet_val.prefix.family = IPv6; return fmt->Read(&val.subnet_val.prefix.in.in6, "subnet-in6"); + default: reporter->Warning("Unknown family type %d when reading subnet\n", family); break; } // Can't be reached. diff --git a/src/util.cc b/src/util.cc index 6bc246172d..e8b33bf59c 100644 --- a/src/util.cc +++ b/src/util.cc @@ -93,8 +93,7 @@ std::string extract_ip(const std::string& i) { if ( s.size() > 1 && s.substr(0, 2) == "0x" ) s.erase(0, 2); - size_t pos = 0; - if ( (pos = s.find(']')) != std::string::npos ) + if ( size_t pos = s.find(']'); pos != std::string::npos ) s = s.substr(0, pos); return s; @@ -298,9 +297,9 @@ void hmac_md5(size_t size, const unsigned char* bytes, unsigned char digest[16]) static bool read_random_seeds(const char* read_file, uint32_t* seed, std::array& buf) { - FILE* f = nullptr; + FILE* f = fopen(read_file, "r"); - if ( ! (f = fopen(read_file, "r")) ) { + if ( ! f ) { reporter->Warning("Could not open seed file '%s': %s", read_file, strerror(errno)); return false; } @@ -328,9 +327,9 @@ static bool read_random_seeds(const char* read_file, uint32_t* seed, static bool write_random_seeds(const char* write_file, uint32_t seed, std::array& buf) { - FILE* f = nullptr; + FILE* f = fopen(write_file, "w+"); - if ( ! (f = fopen(write_file, "w+")) ) { + if ( ! f ) { reporter->Warning("Could not create seed file '%s': %s", write_file, strerror(errno)); return false; } @@ -1764,7 +1763,7 @@ vector* tokenize_string(std::string_view input, std::string_view delim, return rval; } -vector tokenize_string(std::string_view input, const char delim) noexcept { +vector tokenize_string(std::string_view input, const char delim) { vector rval; size_t pos = 0; @@ -1876,9 +1875,9 @@ double current_time(bool real) { struct timeval double_to_timeval(double t) { struct timeval tv; - double t1 = floor(t); - tv.tv_sec = int(t1); - tv.tv_usec = int((t - t1) * 1e6 + 0.5); + double t1 = std::floor(t); + tv.tv_sec = static_cast(t1); + tv.tv_usec = std::lround((t - t1) * 1e6); return tv; } @@ -2337,6 +2336,7 @@ TEST_CASE("util json_escape_utf8") { // Valid ASCII and valid ASCII control characters CHECK(json_escape_utf8("a") == "a"); + // NOLINTNEXTLINE(bugprone-string-literal-with-embedded-nul) CHECK(json_escape_utf8("\b\f\n\r\t\x00\x15") == "\b\f\n\r\t\x00\x15"); // Table 3-7 in https://www.unicode.org/versions/Unicode12.0.0/ch03.pdf describes what is @@ -2403,14 +2403,10 @@ static bool check_ok_utf8(const unsigned char* start, const unsigned char* end) if ( result != conversionOK ) return false; - if ( (output[0] <= 0x001F) || (output[0] == 0x007F) || (output[0] >= 0x0080 && output[0] <= 0x009F) ) - // Control characters - return false; - else if ( output[0] >= 0xE000 && output[0] <= 0xF8FF ) - // Private Use Area - return false; - else if ( output[0] >= 0xFFF0 && output[0] <= 0xFFFF ) - // Specials Characters + if ( ((output[0] <= 0x001F) || (output[0] == 0x007F) || + (output[0] >= 0x0080 && output[0] <= 0x009F)) || // Control characters + (output[0] >= 0xE000 && output[0] <= 0xF8FF) || // Private Use Area + (output[0] >= 0xFFF0 && output[0] <= 0xFFFF) ) // Special characters return false; return true; diff --git a/src/util.h b/src/util.h index 3e8d2943af..7beb6a9001 100644 --- a/src/util.h +++ b/src/util.h @@ -338,7 +338,7 @@ inline std::string get_escaped_string(const std::string& str, bool escape_all) { std::vector* tokenize_string(std::string_view input, std::string_view delim, std::vector* rval = nullptr, int limit = 0); -std::vector tokenize_string(std::string_view input, const char delim) noexcept; +std::vector tokenize_string(std::string_view input, const char delim); extern char* copy_string(const char* str, size_t len); extern char* copy_string(const char* s); diff --git a/src/zeek-setup.cc b/src/zeek-setup.cc index 45582170c8..7bf0fee8f5 100644 --- a/src/zeek-setup.cc +++ b/src/zeek-setup.cc @@ -1018,7 +1018,7 @@ SetupResult setup(int argc, char** argv, Options* zopts) { } // Cooperate with nohup(1). - if ( (oldhandler = setsignal(SIGHUP, sig_handler)) != SIG_DFL ) + if ( oldhandler = setsignal(SIGHUP, sig_handler); oldhandler != SIG_DFL ) (void)setsignal(SIGHUP, oldhandler); // If we were priming the DNS cache (i.e. -P was passed as an argument), flush anything diff --git a/src/zeekygen/Target.cc b/src/zeekygen/Target.cc index a7e9b5bad7..b7ba5cc6fd 100644 --- a/src/zeekygen/Target.cc +++ b/src/zeekygen/Target.cc @@ -346,7 +346,7 @@ void PackageTarget::DoFindDependencies(const vector& infos) { continue; for ( size_t j = 0; j < pkg_deps.size(); ++j ) { - if ( strncmp(script->Name().c_str(), pkg_deps[j]->Name().c_str(), pkg_deps[j]->Name().size()) ) + if ( strncmp(script->Name().c_str(), pkg_deps[j]->Name().c_str(), pkg_deps[j]->Name().size()) != 0 ) continue; DBG_LOG(DBG_ZEEKYGEN, "Script %s associated with package %s", script->Name().c_str(),