diff options
author | Di Chen <dichen@redhat.com> | 2021-10-06 17:04:08 -0400 |
---|---|---|
committer | Frank Ch. Eigler <fche@redhat.com> | 2021-10-06 17:04:22 -0400 |
commit | 260a3105cc0e378882110ba787cd58815183c454 (patch) | |
tree | 56c7433082cff4a167e62e77c0e685440435011f | |
parent | 2e57301be1bbb9c34f8a59122ab500de46eb7acb (diff) | |
download | elfutils-260a3105cc0e378882110ba787cd58815183c454.tar.gz |
PR28242: debuginfod prometheus metric widening
This patch aims to extend http_responses_* metrics with another label
"type" by getting the extra artifact-type content added as a new
key=value tag.
v2, tweaked patch to perform artifact-type sanitization at point of
vulnerability rather than in general metric tabulation logic.
Signed-off-by: Di Chen <dichen@redhat.com>
Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
-rw-r--r-- | debuginfod/ChangeLog | 8 | ||||
-rw-r--r-- | debuginfod/debuginfod.cxx | 79 | ||||
-rw-r--r-- | tests/ChangeLog | 5 | ||||
-rwxr-xr-x | tests/run-debuginfod-000-permission.sh | 12 |
4 files changed, 78 insertions, 26 deletions
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index c2bfce98..de833f7f 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,11 @@ +2021-10-06 Di Chen <dichen@redhat.com> + + PR28242 + * debuginfod.cxx (inc_metrics, add_metrics): Add two-tag variants. + (handler_cb): Call it with artifacttype for http_responses_* metrics. + (handle_buildid): Sanitize artifacttype if necessary. + (dwarf_extract_source_path): Pass sanitizable string param. + 2021-09-17 Noah Sanci <nsanci@redhat.com> * debuginfod-client.c (debuginfod_query_server): curl_multi_perform diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx index 2b9a1c41..d22571ad 100644 --- a/debuginfod/debuginfod.cxx +++ b/debuginfod/debuginfod.cxx @@ -436,7 +436,14 @@ static void inc_metric(const string& metric, static void add_metric(const string& metric, const string& lname, const string& lvalue, double value); -// static void add_metric(const string& metric, double value); +static void inc_metric(const string& metric, + const string& lname, const string& lvalue, + const string& rname, const string& rvalue); +static void add_metric(const string& metric, + const string& lname, const string& lvalue, + const string& rname, const string& rvalue, + double value); + class tmp_inc_metric { // a RAII style wrapper for exception-safe scoped increment & decrement string m, n, v; @@ -1802,7 +1809,7 @@ void debuginfod_pool_end(debuginfod_client* c) static struct MHD_Response* handle_buildid (MHD_Connection* conn, const string& buildid /* unsafe */, - const string& artifacttype /* unsafe */, + string& artifacttype /* unsafe, cleanse on exception/return */, const string& suffix /* unsafe */, int *result_fd) { @@ -1811,7 +1818,10 @@ handle_buildid (MHD_Connection* conn, if (artifacttype == "debuginfo") atype_code = "D"; else if (artifacttype == "executable") atype_code = "E"; else if (artifacttype == "source") atype_code = "S"; - else throw reportable_exception("invalid artifacttype"); + else { + artifacttype = "invalid"; // PR28242 ensure http_resposes metrics don't propagate unclean user data + throw reportable_exception("invalid artifacttype"); + } inc_metric("http_requests_total", "type", artifacttype); @@ -2080,6 +2090,29 @@ add_metric(const string& metric, // and more for higher arity labels if needed +static void +inc_metric(const string& metric, + const string& lname, const string& lvalue, + const string& rname, const string& rvalue) +{ + string key = (metric + "{" + + metric_label(lname, lvalue) + "," + + metric_label(rname, rvalue) + "}"); + unique_lock<mutex> lock(metrics_lock); + metrics[key] ++; +} +static void +add_metric(const string& metric, + const string& lname, const string& lvalue, + const string& rname, const string& rvalue, + double value) +{ + string key = (metric + "{" + + metric_label(lname, lvalue) + "," + + metric_label(rname, rvalue) + "}"); + unique_lock<mutex> lock(metrics_lock); + metrics[key] += value; +} static struct MHD_Response* handle_metrics (off_t* size) @@ -2165,6 +2198,7 @@ handler_cb (void * /*cls*/, struct timespec ts_start, ts_end; clock_gettime (CLOCK_MONOTONIC, &ts_start); double afteryou = 0.0; + string artifacttype, suffix; try { @@ -2203,7 +2237,7 @@ handler_cb (void * /*cls*/, string buildid = url_copy.substr(slash1+1, slash2-slash1-1); size_t slash3 = url_copy.find('/', slash2+1); - string artifacttype, suffix; + if (slash3 == string::npos) { artifacttype = url_copy.substr(slash2+1); @@ -2229,12 +2263,14 @@ handler_cb (void * /*cls*/, else if (url1 == "/metrics") { tmp_inc_metric m ("thread_busy", "role", "http-metrics"); - inc_metric("http_requests_total", "type", "metrics"); + artifacttype = "metrics"; + inc_metric("http_requests_total", "type", artifacttype); r = handle_metrics(& http_size); } else if (url1 == "/") { - inc_metric("http_requests_total", "type", "/"); + artifacttype = "/"; + inc_metric("http_requests_total", "type", artifacttype); r = handle_root(& http_size); } else @@ -2274,19 +2310,21 @@ handler_cb (void * /*cls*/, // related prometheus metrics string http_code_str = to_string(http_code); - if (http_size >= 0) - add_metric("http_responses_transfer_bytes_sum","code",http_code_str, - http_size); - inc_metric("http_responses_transfer_bytes_count","code",http_code_str); - - add_metric("http_responses_duration_milliseconds_sum","code",http_code_str, - deltas*1000); // prometheus prefers _seconds and floating point - inc_metric("http_responses_duration_milliseconds_count","code",http_code_str); - - add_metric("http_responses_after_you_milliseconds_sum","code",http_code_str, - afteryou*1000); - inc_metric("http_responses_after_you_milliseconds_count","code",http_code_str); - + add_metric("http_responses_transfer_bytes_sum", + "code", http_code_str, "type", artifacttype, http_size); + inc_metric("http_responses_transfer_bytes_count", + "code", http_code_str, "type", artifacttype); + + add_metric("http_responses_duration_milliseconds_sum", + "code", http_code_str, "type", artifacttype, deltas*1000); // prometheus prefers _seconds and floating point + inc_metric("http_responses_duration_milliseconds_count", + "code", http_code_str, "type", artifacttype); + + add_metric("http_responses_after_you_milliseconds_sum", + "code", http_code_str, "type", artifacttype, afteryou*1000); + inc_metric("http_responses_after_you_milliseconds_count", + "code", http_code_str, "type", artifacttype); + return rc; } @@ -2334,7 +2372,8 @@ dwarf_extract_source_paths (Elf *elf, set<string>& debug_sourcefiles) struct MHD_Response *r = 0; try { - r = handle_buildid (0, buildid, "debuginfo", "", &alt_fd); + string artifacttype = "debuginfo"; + r = handle_buildid (0, buildid, artifacttype, "", &alt_fd); } catch (const reportable_exception& e) { diff --git a/tests/ChangeLog b/tests/ChangeLog index b62bb350..d289b27c 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,8 @@ +2021-10-06 Di Chen <dichen@redhat.com> + + PR28242 + * run-debuginfod-000-permission.sh: Expect artifacttype metrics. + 2021-09-17 Noah Sanci <nsanci@redhat.com> * run-debuginfod-response-header.sh: removed checking for Connection diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-debuginfod-000-permission.sh index 1e92bdb8..afa7e931 100755 --- a/tests/run-debuginfod-000-permission.sh +++ b/tests/run-debuginfod-000-permission.sh @@ -61,22 +61,22 @@ if [ -r $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then err fi -bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404"}'` +bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'` testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567 || true -bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404"}'` +bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'` if [ "$bytecount_before" != "$bytecount_after" ]; then - echo "http_responses_transfer_bytes_count{code="404"} has changed." + echo "http_responses_transfer_bytes_count{code="404",type="debuginfo"} has changed." err fi # set cache_miss_s to 0 and sleep 1 to make the mtime expire. echo 0 > $DEBUGINFOD_CACHE_PATH/cache_miss_s sleep 1 -bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404"}'` +bytecount_before=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'` testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567 || true -bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404"}'` +bytecount_after=`curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_transfer_bytes_count{code="404",type="debuginfo"}'` if [ "$bytecount_before" == "$bytecount_after" ]; then - echo "http_responses_transfer_bytes_count{code="404"} should be incremented." + echo "http_responses_transfer_bytes_count{code="404",type="debuginfo"} should be incremented." err fi |