From a8e137a87975d5c6a5f5ccaeac38bf03d28bc324 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Feb 2020 21:16:57 -0800 Subject: [PATCH 1/6] Format interval values consistently across 32-bit/64-bit platforms --- src/Val.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Val.cc b/src/Val.cc index 595c208f8a..9b95769877 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -13,6 +13,8 @@ #include #include +#include + #include "Attr.h" #include "BroString.h" #include "CompHash.h" @@ -721,7 +723,8 @@ void IntervalVal::ValDescribe(ODesc* d) const if ( ! (v >= unit || v <= -unit) ) continue; - double num = static_cast(static_cast(v / unit)); + double num = v / unit; + num = num < 0 ? std::ceil(num) : std::floor(num); v -= num * unit; to_print = num; } From 8e353aafe5dc510d32d6f24524e56e8dc8632dc3 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Feb 2020 21:18:07 -0800 Subject: [PATCH 2/6] Format tables indexed by patterns consistently across 32-bit/64-bit Uses a full 64 bit integer for length values regardless of actual size_t to get consistent results between either 32-bit and 64-bit platforms. --- src/CompHash.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/CompHash.cc b/src/CompHash.cc index 37ee7e25c9..511d6c620a 100644 --- a/src/CompHash.cc +++ b/src/CompHash.cc @@ -156,10 +156,10 @@ char* CompositeHash::SingleValHash(int type_check, char* kp0, v->AsPattern()->AnywherePatternText() }; - size_t* kp; + uint64_t* kp; for ( int i = 0; i < 2; i++ ) { - kp = AlignAndPadType(kp0+i); + kp = AlignAndPadType(kp0+i); *kp = strlen(texts[i]) + 1; } @@ -507,7 +507,7 @@ int CompositeHash::SingleTypeKeySize(BroType* bt, const Val* v, if ( ! v ) return (optional && ! calc_static_size) ? sz : 0; - sz = SizeAlign(sz, 2 * sizeof(size_t)); + sz = SizeAlign(sz, 2 * sizeof(uint64_t)); sz += strlen(v->AsPattern()->PatternText()) + strlen(v->AsPattern()->AnywherePatternText()) + 2; // 2 for null terminators break; @@ -894,7 +894,7 @@ const char* CompositeHash::RecoverOneVal(const HashKey* k, const char* kp0, } else { - const size_t* const len = AlignType(kp0); + const uint64_t* const len = AlignType(kp0); kp1 = reinterpret_cast(len+2); re = new RE_Matcher(kp1, kp1 + len[0]); From 8fed26824b592b3b55cb34f8bca3e197d2e2e568 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Feb 2020 21:26:55 -0800 Subject: [PATCH 3/6] Replace va_list fmt() overload with vfmt() Using an overload that takes a va_list argument potentially causes accidental misuse on platforms (e.g. 32-bit) where va_list is implemented as a type that may collide with commonly-used argument types. For example: char* c = copy_string("hi"); fmt("%s", (const char*)c); fmt("%s", c); The first fmt() call correctly goes through fmt(const char*, ...) first, but the second mistakenly goes through fmt(const char*, va_list) first because variadic function overloads have lower priority during overload resolution and va_list on a 32-bit system happens to be defined as a pointer type that can match with "char*" but not "const char*". --- NEWS | 6 ++++++ src/broker/Manager.cc | 2 +- src/supervisor/Supervisor.cc | 2 +- src/util.cc | 4 ++-- src/util.h | 2 +- 5 files changed, 11 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index d3777ae5f7..8bdffeda9b 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,12 @@ Changed Functionality Removed Functionality --------------------- +- The fmt() function which takes a va_list argument is replaced, use + the new vfmt() function for equivalent functionality. The former is + deprecated because overloading it with the variadic fmt() function + can cause the unintended overload to be chosen depending on how the + platform implements va_list. + Deprecated Functionality ------------------------ diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index eae0ce723f..bb5d3a6af3 100644 --- a/src/broker/Manager.cc +++ b/src/broker/Manager.cc @@ -625,7 +625,7 @@ void Manager::Error(const char* format, ...) { va_list args; va_start(args, format); - auto msg = fmt(format, args); + auto msg = vfmt(format, args); va_end(args); if ( script_scope ) diff --git a/src/supervisor/Supervisor.cc b/src/supervisor/Supervisor.cc index a1dd2a21d4..685b0707e7 100644 --- a/src/supervisor/Supervisor.cc +++ b/src/supervisor/Supervisor.cc @@ -724,7 +724,7 @@ void Stem::ReportStatus(const Supervisor::Node& node) const void Stem::Log(std::string_view type, const char* format, va_list args) const { - auto raw_msg = fmt(format, args); + auto raw_msg = vfmt(format, args); if ( getenv("ZEEK_DEBUG_STEM_STDERR") ) { diff --git a/src/util.cc b/src/util.cc index 6429e1481d..e75e79882d 100644 --- a/src/util.cc +++ b/src/util.cc @@ -817,7 +817,7 @@ const char* fmt_bytes(const char* data, int len) return buf; } -const char* fmt(const char* format, va_list al) +const char* vfmt(const char* format, va_list al) { static char* buf = 0; static unsigned int buf_len = 1024; @@ -848,7 +848,7 @@ const char* fmt(const char* format, ...) { va_list al; va_start(al, format); - auto rval = fmt(format, al); + auto rval = vfmt(format, al); va_end(al); return rval; } diff --git a/src/util.h b/src/util.h index cb5e1fc2eb..ae2f0fc2d4 100644 --- a/src/util.h +++ b/src/util.h @@ -188,7 +188,7 @@ extern std::string strtolower(const std::string& s); extern const char* fmt_bytes(const char* data, int len); // Note: returns a pointer into a shared buffer. -extern const char* fmt(const char* format, va_list args); +extern const char* vfmt(const char* format, va_list args); // Note: returns a pointer into a shared buffer. extern const char* fmt(const char* format, ...) __attribute__((format (printf, 1, 2))); From aa4185cfff1b16fc88efd185c998876c19ef432b Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Feb 2020 21:39:24 -0800 Subject: [PATCH 4/6] Add a 32-bit task to Cirrus CI config --- .cirrus.yml | 7 +++++++ ci/debian-9-32bit/Dockerfile | 40 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 ci/debian-9-32bit/Dockerfile diff --git a/.cirrus.yml b/.cirrus.yml index 9a1e214cb5..f501f0bc4d 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -77,6 +77,13 @@ debian9_task: << : *RESOURCES_TEMPLATE << : *CI_TEMPLATE +debian9_32bit_task: + container: + # Debian 9 EOL: June 2022 + dockerfile: ci/debian-9-32bit/Dockerfile + << : *RESOURCES_TEMPLATE + << : *CI_TEMPLATE + ubuntu18_task: container: # Ubuntu 18.04 EOL: April 2023 diff --git a/ci/debian-9-32bit/Dockerfile b/ci/debian-9-32bit/Dockerfile new file mode 100644 index 0000000000..053c3ea760 --- /dev/null +++ b/ci/debian-9-32bit/Dockerfile @@ -0,0 +1,40 @@ +FROM i386/debian:9 + +RUN apt-get update && apt-get -y install \ + git \ + cmake \ + make \ + gcc \ + g++ \ + flex \ + bison \ + libpcap-dev \ + libssl-dev \ + python3 \ + python3-dev \ + python3-pip\ + swig \ + zlib1g-dev \ + libkrb5-dev \ + bsdmainutils \ + sqlite3 \ + curl \ + wget \ + xz-utils \ + clang-7 \ + libc++-7-dev \ + libc++abi-7-dev \ + && rm -rf /var/lib/apt/lists/* + +RUN update-alternatives --install /usr/bin/cc cc /usr/bin/clang-7 100 +RUN update-alternatives --install /usr/bin/c++ c++ /usr/bin/clang++-7 100 + +# Many distros adhere to PEP 394's recommendation for `python` = `python2` so +# this is a simple workaround until we drop Python 2 support and explicitly +# use `python3` for all invocations (e.g. in shebangs). +RUN ln -sf /usr/bin/python3 /usr/local/bin/python +RUN ln -sf /usr/bin/pip3 /usr/local/bin/pip + +RUN pip install junit2html + +ENV CXXFLAGS=-stdlib=libc++ From 4375aa150fd7b36abaa2687dddc7459fd6e90c54 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Fri, 14 Feb 2020 23:36:59 -0800 Subject: [PATCH 5/6] Improve HTTP version number comparisons Previous use of floating point comparisons was not always stable. --- src/analyzer/protocol/http/HTTP.cc | 43 ++++++++++++++++-------------- src/analyzer/protocol/http/HTTP.h | 26 ++++++++++++++---- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/analyzer/protocol/http/HTTP.cc b/src/analyzer/protocol/http/HTTP.cc index 56359a177e..3766cc6ddc 100644 --- a/src/analyzer/protocol/http/HTTP.cc +++ b/src/analyzer/protocol/http/HTTP.cc @@ -472,14 +472,16 @@ void HTTP_Entity::SubmitHeader(mime::MIME_Header* h) else if ( mime::istrequal(h->get_name(), "transfer-encoding") ) { - double http_version = 0; + HTTP_Analyzer::HTTP_VersionNumber http_version; + if (http_message->analyzer->GetRequestOngoing()) - http_version = http_message->analyzer->GetRequestVersion(); + http_version = http_message->analyzer->GetRequestVersionNumber(); else // reply_ongoing - http_version = http_message->analyzer->GetReplyVersion(); + http_version = http_message->analyzer->GetReplyVersionNumber(); data_chunk_t vt = h->get_value_token(); - if ( mime::istrequal(vt, "chunked") && http_version == 1.1 ) + if ( mime::istrequal(vt, "chunked") && + http_version == HTTP_Analyzer::HTTP_VersionNumber{1, 1} ) chunked_transfer_state = BEFORE_CHUNK; } @@ -842,7 +844,6 @@ HTTP_Analyzer::HTTP_Analyzer(Connection* conn) { num_requests = num_replies = 0; num_request_lines = num_reply_lines = 0; - request_version = reply_version = 0.0; // unknown version keep_alive = 0; connection_close = 0; @@ -1184,8 +1185,8 @@ void HTTP_Analyzer::GenStats() RecordVal* r = new RecordVal(http_stats_rec); r->Assign(0, val_mgr->GetCount(num_requests)); r->Assign(1, val_mgr->GetCount(num_replies)); - r->Assign(2, new Val(request_version, TYPE_DOUBLE)); - r->Assign(3, new Val(reply_version, TYPE_DOUBLE)); + r->Assign(2, new Val(request_version.ToDouble(), TYPE_DOUBLE)); + r->Assign(3, new Val(reply_version.ToDouble(), TYPE_DOUBLE)); // DEBUG_MSG("%.6f http_stats\n", network_time); ConnectionEventFast(http_stats, {BuildConnVal(), r}); @@ -1318,14 +1319,14 @@ int HTTP_Analyzer::ParseRequest(const char* line, const char* end_of_line) if ( version_start >= end_of_line ) { // If no version is found - SetVersion(request_version, 0.9); + SetVersion(&request_version, {0, 9}); } else { if ( version_start + 8 <= end_of_line ) { version_start += 5; // "HTTP/" - SetVersion(request_version, + SetVersion(&request_version, HTTP_Version(end_of_line - version_start, version_start)); @@ -1348,31 +1349,33 @@ int HTTP_Analyzer::ParseRequest(const char* line, const char* end_of_line) } // Only recognize [0-9][.][0-9]. -double HTTP_Analyzer::HTTP_Version(int len, const char* data) +HTTP_Analyzer::HTTP_VersionNumber HTTP_Analyzer::HTTP_Version(int len, const char* data) { if ( len >= 3 && data[0] >= '0' && data[0] <= '9' && data[1] == '.' && data[2] >= '0' && data[2] <= '9' ) { - return double(data[0] - '0') + 0.1 * double(data[2] - '0'); + uint8_t major = data[0] - '0'; + uint8_t minor = data[2] - '0'; + return {major, minor}; } else { HTTP_Event("bad_HTTP_version", mime::new_string_val(len, data)); - return 0; + return {}; } } -void HTTP_Analyzer::SetVersion(double& version, double new_version) +void HTTP_Analyzer::SetVersion(HTTP_VersionNumber* version, HTTP_VersionNumber new_version) { - if ( version == 0.0 ) - version = new_version; + if ( *version == HTTP_VersionNumber{} ) + *version = new_version; - else if ( version != new_version ) + else if ( *version != new_version ) Weird("HTTP_version_mismatch"); - if ( version > 1.05 ) + if ( version->major > 1 || (version->major == 1 && version->minor > 0) ) keep_alive = 1; } @@ -1434,7 +1437,7 @@ void HTTP_Analyzer::HTTP_Request() request_method, TruncateURI(request_URI->AsStringVal()), TruncateURI(unescaped_URI->AsStringVal()), - new StringVal(fmt("%.1f", request_version)), + new StringVal(fmt("%.1f", request_version.ToDouble())), }); } } @@ -1445,7 +1448,7 @@ void HTTP_Analyzer::HTTP_Reply() { ConnectionEventFast(http_reply, { BuildConnVal(), - new StringVal(fmt("%.1f", reply_version)), + new StringVal(fmt("%.1f", reply_version.ToDouble())), val_mgr->GetCount(reply_code), reply_reason_phrase ? reply_reason_phrase->Ref() : @@ -1565,7 +1568,7 @@ int HTTP_Analyzer::HTTP_ReplyLine(const char* line, const char* end_of_line) return 0; } - SetVersion(reply_version, HTTP_Version(end_of_line - rest, rest)); + SetVersion(&reply_version, HTTP_Version(end_of_line - rest, rest)); for ( ; rest < end_of_line; ++rest ) if ( mime::is_lws(*rest) ) diff --git a/src/analyzer/protocol/http/HTTP.h b/src/analyzer/protocol/http/HTTP.h index aa069a4156..09a266fba5 100644 --- a/src/analyzer/protocol/http/HTTP.h +++ b/src/analyzer/protocol/http/HTTP.h @@ -175,8 +175,24 @@ public: void ConnectionReset() override; void PacketWithRST() override; - double GetRequestVersion() { return request_version; }; - double GetReplyVersion() { return reply_version; }; + struct HTTP_VersionNumber { + uint8_t major = 0; + uint8_t minor = 0; + + bool operator==(const HTTP_VersionNumber& other) const + { return minor == other.minor && major == other.major; } + + bool operator!=(const HTTP_VersionNumber& other) const + { return ! operator==(other); } + + double ToDouble() const + { return major + minor * 0.1; } + }; + + double GetRequestVersion() { return request_version.ToDouble(); }; + double GetReplyVersion() { return reply_version.ToDouble(); }; + HTTP_VersionNumber GetRequestVersionNumber() { return request_version; }; + HTTP_VersionNumber GetReplyVersionNumber() { return reply_version; }; int GetRequestOngoing() { return request_ongoing; }; int GetReplyOngoing() { return reply_ongoing; }; @@ -204,9 +220,9 @@ protected: const char* prefix); int ParseRequest(const char* line, const char* end_of_line); - double HTTP_Version(int len, const char* data); + HTTP_VersionNumber HTTP_Version(int len, const char* data); - void SetVersion(double& version, double new_version); + void SetVersion(HTTP_VersionNumber* version, HTTP_VersionNumber new_version); int RequestExpected() const { return num_requests == 0 || keep_alive; } @@ -227,7 +243,7 @@ protected: int request_state, reply_state; int num_requests, num_replies; int num_request_lines, num_reply_lines; - double request_version, reply_version; + HTTP_VersionNumber request_version, reply_version; int keep_alive; int connection_close; int request_ongoing, reply_ongoing; From a785212e8071733fd04465dc532de73b72d07e45 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Sat, 15 Feb 2020 00:54:44 -0800 Subject: [PATCH 6/6] Improve formatting of doubles that are close to integers Now checks for approximate floating point equality so that more doubles get properly disambiguated from integers --- src/Desc.cc | 9 ++++++++- testing/btest/Baseline/core.print-interval/out | 4 ++-- .../test.log | 4 ++-- testing/external/commit-hash.zeek-testing | 2 +- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/Desc.cc b/src/Desc.cc index 4511f59614..40ef4c6677 100644 --- a/src/Desc.cc +++ b/src/Desc.cc @@ -164,7 +164,14 @@ void ODesc::Add(double d, bool no_exp) Add(tmp); - if ( nearbyint(d) == d && isfinite(d) && ! strchr(tmp, 'e') ) + auto approx_equal = [](double a, double b, double tolerance = 1e-6) -> bool + { + auto v = a - b; + return v < 0 ? -v < tolerance : v < tolerance; + }; + + if ( approx_equal(d, nearbyint(d), 1e-9) && + isfinite(d) && ! strchr(tmp, 'e') ) // disambiguate from integer Add(".0"); } diff --git a/testing/btest/Baseline/core.print-interval/out b/testing/btest/Baseline/core.print-interval/out index 701e5f9cdb..683dd80adc 100644 --- a/testing/btest/Baseline/core.print-interval/out +++ b/testing/btest/Baseline/core.print-interval/out @@ -8,12 +8,12 @@ 1.0 day 10.0 hrs 17.0 mins 36.0 secs 789.0 msecs 123.449984 usecs -1.0 day -10.0 hrs -17.0 mins -36.0 secs -789.0 msecs -123.449984 usecs 1.001 usecs -1.0 msec 1 usec +1.0 msec 1.0 usec 11.0 msecs 8.0 days 12.0 hrs 7.0 hrs 30.0 mins 6.0 mins 30.0 secs 5.0 secs 500.0 msecs -4.0 msecs 500 usecs +4.0 msecs 500.0 usecs 3.5 usecs 2.0 days 2.0 secs diff --git a/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-double/test.log b/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-double/test.log index 21457f916c..743312d7f2 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-double/test.log +++ b/testing/btest/Baseline/scripts.base.frameworks.logging.ascii-double/test.log @@ -22,8 +22,8 @@ 178999999999999996376899522972626047077637637819240219954027593177370961667659291027329061638406108931437333529420935752785895444161234074984843178962619172326295244262722141766382622299223626438470088150218987997954747866198184686628013966119769261150988554952970462018533787926725176560021258785656871583744.0 -178999999999999996376899522972626047077637637819240219954027593177370961667659291027329061638406108931437333529420935752785895444161234074984843178962619172326295244262722141766382622299223626438470088150218987997954747866198184686628013966119769261150988554952970462018533787926725176560021258785656871583744.0 0.000012 -0 --0 +0.0 +-0.0 inf -inf 0.0 diff --git a/testing/external/commit-hash.zeek-testing b/testing/external/commit-hash.zeek-testing index 5c53cb6426..36c7613b60 100644 --- a/testing/external/commit-hash.zeek-testing +++ b/testing/external/commit-hash.zeek-testing @@ -1 +1 @@ -da47ae786562da18910d994b7868530929db6271 +96e78328a1a9e6c07e725b282c293728f70737f0