From 71f89d6efad5392dbc6a76233d1d19895a799de2 Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sat, 14 Nov 2026 09:05:03 +0800 Subject: [PATCH 1/3] fix(builtin): escape directory and profiler names --- src/brpc/builtin/common.cpp | 23 ++++---- src/brpc/builtin/hotspots_service.cpp | 75 ++++++++++++++++++++++---- test/brpc_builtin_service_unittest.cpp | 58 ++++++++++++++++++++ 3 files changed, 133 insertions(+), 23 deletions(-) diff --git a/src/brpc/builtin/common.cpp b/src/brpc/builtin/common.cpp index bfb8e54628..929307b03d 100644 --- a/src/brpc/builtin/common.cpp +++ b/src/brpc/builtin/common.cpp @@ -25,6 +25,7 @@ #include "butil/file_util.h" // butil::FilePath #include "butil/third_party/murmurhash3/murmurhash3.h" #include "butil/process_util.h" // ReadCommandLine +#include "brpc/controller.h" #include "brpc/server.h" #include "brpc/reloadable_flags.h" #include "brpc/builtin/common.h" @@ -128,21 +129,19 @@ std::ostream& operator<<(std::ostream& os, const PrintedAsDateTime& d) { } std::ostream& operator<<(std::ostream& os, const Path& link) { - if (link.html_addr) { - if (link.html_addr != Path::LOCAL) { - os << ""; - } else { - os << ""; - } + const char* const text = (link.text ? link.text : link.uri); + if (!link.html_addr) { + os << text; + return os; } - if (link.text) { - os << link.text; + + const std::string escaped_uri = WebEscape(link.uri); + if (link.html_addr != Path::LOCAL) { + os << ""; } else { - os << link.uri; - } - if (link.html_addr) { - os << ""; + os << ""; } + os << WebEscape(text) << ""; return os; } diff --git a/src/brpc/builtin/hotspots_service.cpp b/src/brpc/builtin/hotspots_service.cpp index 7ed4295312..5474123e67 100644 --- a/src/brpc/builtin/hotspots_service.cpp +++ b/src/brpc/builtin/hotspots_service.cpp @@ -24,8 +24,10 @@ #include "butil/popen.h" // butil::read_command_output #include "butil/fd_guard.h" // butil::fd_guard #include "butil/iobuf_profiler.h" +#include "butil/strings/string_util.h" #include "brpc/log.h" #include "brpc/controller.h" +#include "brpc/grpc.h" #include "brpc/server.h" #include "brpc/reloadable_flags.h" #include "brpc/builtin/pprof_perl.h" @@ -116,6 +118,38 @@ BRPC_VALIDATE_GFLAG(max_flame_graph_width, PositiveInteger); static const char* const PPROF_FILENAME = "pprof.pl"; static int DEFAULT_PROFILING_SECONDS = 10; static size_t CONCURRENT_PROFILING_LIMIT = 256; +static const char PROFILE_PATH_ENCODING_PREFIX[] = "brpc_pct:"; + +static bool DecodeProfilePathQuery(const std::string* encoded_path, + std::string* decoded_path) { + if (encoded_path == nullptr) { + return true; + } + const size_t prefix_size = sizeof(PROFILE_PATH_ENCODING_PREFIX) - 1; + if (encoded_path->compare(0, prefix_size, PROFILE_PATH_ENCODING_PREFIX) != 0) { + *decoded_path = *encoded_path; + return true; + } + const std::string percent_encoded = encoded_path->substr(prefix_size); + for (size_t i = 0; i < percent_encoded.size(); ++i) { + if (percent_encoded[i] == '%') { + if (i + 2 >= percent_encoded.size() || + !IsHexDigit(percent_encoded[i + 1]) || + !IsHexDigit(percent_encoded[i + 2])) { + return false; + } + i += 2; + } + } + PercentDecode(percent_encoded, decoded_path); + return decoded_path->find('\0') == std::string::npos; +} + +static std::string EncodeProfilePathQuery(const std::string& path) { + std::string percent_encoded; + PercentEncode(path, &percent_encoded); + return std::string(PROFILE_PATH_ENCODING_PREFIX) + percent_encoded; +} struct ProfilingWaiter { Controller* cntl; @@ -627,7 +661,13 @@ static void DoProfiling(ProfilingType type, cntl->server()->PrintTabsBody(os, ProfilingType2String(type)); } - const std::string* view = cntl->http_request().uri().GetQuery("view"); + const std::string* encoded_view = + cntl->http_request().uri().GetQuery("view"); + std::string decoded_view; + if (!DecodeProfilePathQuery(encoded_view, &decoded_view)) { + return cntl->SetFailed(EINVAL, "Invalid query `view'"); + } + const std::string* view = (encoded_view ? &decoded_view : nullptr); if (view) { if (!ValidProfilePath(*view)) { return cntl->SetFailed(EINVAL, "Invalid query `view'"); @@ -895,9 +935,22 @@ static void StartProfiling(ProfilingType type, } const int seconds = ReadSeconds(cntl); - const std::string* view = cntl->http_request().uri().GetQuery("view"); + const std::string* encoded_view = + cntl->http_request().uri().GetQuery("view"); + std::string decoded_view; + if (!DecodeProfilePathQuery(encoded_view, &decoded_view)) { + return cntl->SetFailed(EINVAL, "Invalid query `view'"); + } + const std::string* view = (encoded_view ? &decoded_view : nullptr); const bool show_ccount = cntl->http_request().uri().GetQuery("ccount"); - const std::string* base_name = cntl->http_request().uri().GetQuery("base"); + const std::string* encoded_base_name = + cntl->http_request().uri().GetQuery("base"); + std::string decoded_base_name; + if (!DecodeProfilePathQuery(encoded_base_name, &decoded_base_name)) { + return cntl->SetFailed(EINVAL, "Invalid query `base'"); + } + const std::string* base_name = + (encoded_base_name ? &decoded_base_name : nullptr); const std::string* display_type_query = cntl->http_request().uri().GetQuery("display_type"); DisplayType display_type = DisplayType::kDot; if (display_type_query) { @@ -956,10 +1009,10 @@ static void StartProfiling(ProfilingType type, os << " var targetURL = '/hotspots/" << type_str << "';\n" " targetURL += '?display_type=' + display_type;\n" " if (past_prof != '') {\n" - " targetURL += '&view=' + past_prof;\n" + " targetURL += '&view=brpc_pct:' + encodeURIComponent(past_prof);\n" " }\n" " if (base_prof != '') {\n" - " targetURL += '&base=' + base_prof;\n" + " targetURL += '&base=brpc_pct:' + encodeURIComponent(base_prof);\n" " }\n"; if (type == PROFILING_CONTENTION) { os << @@ -1025,10 +1078,10 @@ static void StartProfiling(ProfilingType type, os << "&ccount"; } if (view) { - os << "&view=" << *view; + os << "&view=" << EncodeProfilePathQuery(*view); } if (base_name) { - os << "&base=" << *base_name; + os << "&base=" << EncodeProfilePathQuery(*base_name); } os << "\",\n" " type: \"GET\",\n" @@ -1095,11 +1148,11 @@ static void StartProfiling(ProfilingType type, ""; os << "
Display:
" @@ -1120,11 +1173,11 @@ static void StartProfiling(ProfilingType type, "
"; } diff --git a/test/brpc_builtin_service_unittest.cpp b/test/brpc_builtin_service_unittest.cpp index 79444d6892..301d3a8aa8 100644 --- a/test/brpc_builtin_service_unittest.cpp +++ b/test/brpc_builtin_service_unittest.cpp @@ -22,16 +22,20 @@ #include #include #include +#include #include #include #include #include "gperftools_helper.h" +#include "butil/file_util.h" +#include "butil/files/scoped_temp_dir.h" #include "butil/time.h" #include "butil/macros.h" #include "brpc/socket.h" #include "brpc/server.h" #include "brpc/channel.h" #include "brpc/controller.h" +#include "brpc/grpc.h" #include "brpc/span.h" #include "brpc/reloadable_flags.h" #include "brpc/builtin/version_service.h" @@ -815,6 +819,60 @@ TEST_F(BuiltinServiceTest, hotspots_query_escaping) { } } +TEST_F(BuiltinServiceTest, hotspots_profile_filename_escaping) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + butil::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + brpc::FLAGS_rpc_profiling_dir = temp_dir.path().value(); + const butil::FilePath profile_dir = temp_dir.path().Append( + brpc::GetProgramChecksum()); + ASSERT_TRUE(butil::CreateDirectory(profile_dir)); + + const std::string filename = "profile'>.cpu"; + const butil::FilePath profile_path = profile_dir.Append(filename); + ASSERT_EQ(0, butil::WriteFile(profile_path, "", 0)); + + ClosureChecker done; + brpc::Controller cntl; + brpc::HotspotsRequest req; + brpc::HotspotsResponse res; + brpc::HotspotsService service; + SetUpController(&cntl, true); + service.cpu(&cntl, &req, &res, &done); + + const std::string& body = cntl.response_attachment().to_string(); + EXPECT_EQ(std::string::npos, body.find(filename)) + << "unescaped profile filename in html: " << body; + EXPECT_NE(std::string::npos, body.find(brpc::WebEscape(filename))); + + const butil::FilePath selected_profile_path = + profile_dir.Append("selected.cpu"); + ASSERT_EQ(0, butil::WriteFile(selected_profile_path, "", 0)); + std::string percent_encoded_path; + brpc::PercentEncode(selected_profile_path.value(), &percent_encoded_path); + ClosureChecker encoded_done; + brpc::Controller encoded_cntl; + brpc::HotspotsRequest encoded_req; + brpc::HotspotsResponse encoded_res; + SetUpController(&encoded_cntl, true); + encoded_cntl.http_request().uri().SetQuery( + "view", "brpc_pct:" + percent_encoded_path); + service.cpu(&encoded_cntl, &encoded_req, &encoded_res, &encoded_done); + EXPECT_FALSE(encoded_cntl.Failed()) << encoded_cntl.ErrorText(); +} + +TEST_F(BuiltinServiceTest, path_html_escaping) { + const std::string payload = "\"'>&"; + std::ostringstream html; + html << brpc::Path(payload.c_str(), brpc::Path::LOCAL, payload.c_str()); + EXPECT_EQ(std::string::npos, html.str().find(payload)); + EXPECT_NE(std::string::npos, html.str().find(brpc::WebEscape(payload))); + + std::ostringstream text; + text << brpc::Path(payload.c_str(), nullptr, payload.c_str()); + EXPECT_EQ(payload, text.str()); +} + TEST_F(BuiltinServiceTest, rpcz_query_escaping) { // Restore -enable_rpcz on any exit of this test. GFLAGS_NAMESPACE::FlagSaver flag_saver; From ccfeb3957e6087e83089d692b2a21bf33a03191a Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sat, 14 Nov 2026 12:56:36 +0800 Subject: [PATCH 2/3] Simplify profiler path handling --- src/brpc/builtin/hotspots_service.cpp | 65 +++----------------------- test/brpc_builtin_service_unittest.cpp | 39 ++++++++++------ 2 files changed, 32 insertions(+), 72 deletions(-) diff --git a/src/brpc/builtin/hotspots_service.cpp b/src/brpc/builtin/hotspots_service.cpp index 5474123e67..1d18f9f14a 100644 --- a/src/brpc/builtin/hotspots_service.cpp +++ b/src/brpc/builtin/hotspots_service.cpp @@ -118,38 +118,6 @@ BRPC_VALIDATE_GFLAG(max_flame_graph_width, PositiveInteger); static const char* const PPROF_FILENAME = "pprof.pl"; static int DEFAULT_PROFILING_SECONDS = 10; static size_t CONCURRENT_PROFILING_LIMIT = 256; -static const char PROFILE_PATH_ENCODING_PREFIX[] = "brpc_pct:"; - -static bool DecodeProfilePathQuery(const std::string* encoded_path, - std::string* decoded_path) { - if (encoded_path == nullptr) { - return true; - } - const size_t prefix_size = sizeof(PROFILE_PATH_ENCODING_PREFIX) - 1; - if (encoded_path->compare(0, prefix_size, PROFILE_PATH_ENCODING_PREFIX) != 0) { - *decoded_path = *encoded_path; - return true; - } - const std::string percent_encoded = encoded_path->substr(prefix_size); - for (size_t i = 0; i < percent_encoded.size(); ++i) { - if (percent_encoded[i] == '%') { - if (i + 2 >= percent_encoded.size() || - !IsHexDigit(percent_encoded[i + 1]) || - !IsHexDigit(percent_encoded[i + 2])) { - return false; - } - i += 2; - } - } - PercentDecode(percent_encoded, decoded_path); - return decoded_path->find('\0') == std::string::npos; -} - -static std::string EncodeProfilePathQuery(const std::string& path) { - std::string percent_encoded; - PercentEncode(path, &percent_encoded); - return std::string(PROFILE_PATH_ENCODING_PREFIX) + percent_encoded; -} struct ProfilingWaiter { Controller* cntl; @@ -661,13 +629,7 @@ static void DoProfiling(ProfilingType type, cntl->server()->PrintTabsBody(os, ProfilingType2String(type)); } - const std::string* encoded_view = - cntl->http_request().uri().GetQuery("view"); - std::string decoded_view; - if (!DecodeProfilePathQuery(encoded_view, &decoded_view)) { - return cntl->SetFailed(EINVAL, "Invalid query `view'"); - } - const std::string* view = (encoded_view ? &decoded_view : nullptr); + const std::string* view = cntl->http_request().uri().GetQuery("view"); if (view) { if (!ValidProfilePath(*view)) { return cntl->SetFailed(EINVAL, "Invalid query `view'"); @@ -935,22 +897,9 @@ static void StartProfiling(ProfilingType type, } const int seconds = ReadSeconds(cntl); - const std::string* encoded_view = - cntl->http_request().uri().GetQuery("view"); - std::string decoded_view; - if (!DecodeProfilePathQuery(encoded_view, &decoded_view)) { - return cntl->SetFailed(EINVAL, "Invalid query `view'"); - } - const std::string* view = (encoded_view ? &decoded_view : nullptr); + const std::string* view = cntl->http_request().uri().GetQuery("view"); const bool show_ccount = cntl->http_request().uri().GetQuery("ccount"); - const std::string* encoded_base_name = - cntl->http_request().uri().GetQuery("base"); - std::string decoded_base_name; - if (!DecodeProfilePathQuery(encoded_base_name, &decoded_base_name)) { - return cntl->SetFailed(EINVAL, "Invalid query `base'"); - } - const std::string* base_name = - (encoded_base_name ? &decoded_base_name : nullptr); + const std::string* base_name = cntl->http_request().uri().GetQuery("base"); const std::string* display_type_query = cntl->http_request().uri().GetQuery("display_type"); DisplayType display_type = DisplayType::kDot; if (display_type_query) { @@ -1009,10 +958,10 @@ static void StartProfiling(ProfilingType type, os << " var targetURL = '/hotspots/" << type_str << "';\n" " targetURL += '?display_type=' + display_type;\n" " if (past_prof != '') {\n" - " targetURL += '&view=brpc_pct:' + encodeURIComponent(past_prof);\n" + " targetURL += '&view=' + past_prof;\n" " }\n" " if (base_prof != '') {\n" - " targetURL += '&base=brpc_pct:' + encodeURIComponent(base_prof);\n" + " targetURL += '&base=' + base_prof;\n" " }\n"; if (type == PROFILING_CONTENTION) { os << @@ -1078,10 +1027,10 @@ static void StartProfiling(ProfilingType type, os << "&ccount"; } if (view) { - os << "&view=" << EncodeProfilePathQuery(*view); + os << "&view=" << *view; } if (base_name) { - os << "&base=" << EncodeProfilePathQuery(*base_name); + os << "&base=" << *base_name; } os << "\",\n" " type: \"GET\",\n" diff --git a/test/brpc_builtin_service_unittest.cpp b/test/brpc_builtin_service_unittest.cpp index 301d3a8aa8..fd7595e373 100644 --- a/test/brpc_builtin_service_unittest.cpp +++ b/test/brpc_builtin_service_unittest.cpp @@ -845,20 +845,31 @@ TEST_F(BuiltinServiceTest, hotspots_profile_filename_escaping) { << "unescaped profile filename in html: " << body; EXPECT_NE(std::string::npos, body.find(brpc::WebEscape(filename))); - const butil::FilePath selected_profile_path = - profile_dir.Append("selected.cpu"); - ASSERT_EQ(0, butil::WriteFile(selected_profile_path, "", 0)); - std::string percent_encoded_path; - brpc::PercentEncode(selected_profile_path.value(), &percent_encoded_path); - ClosureChecker encoded_done; - brpc::Controller encoded_cntl; - brpc::HotspotsRequest encoded_req; - brpc::HotspotsResponse encoded_res; - SetUpController(&encoded_cntl, true); - encoded_cntl.http_request().uri().SetQuery( - "view", "brpc_pct:" + percent_encoded_path); - service.cpu(&encoded_cntl, &encoded_req, &encoded_res, &encoded_done); - EXPECT_FALSE(encoded_cntl.Failed()) << encoded_cntl.ErrorText(); +} + +TEST_F(BuiltinServiceTest, hotspots_non_responsive_raw_profile_paths) { + GFLAGS_NAMESPACE::FlagSaver flag_saver; + butil::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + brpc::FLAGS_rpc_profiling_dir = temp_dir.path().value(); + const butil::FilePath profile_dir = temp_dir.path().Append( + brpc::GetProgramChecksum()); + ASSERT_TRUE(butil::CreateDirectory(profile_dir)); + const butil::FilePath view_path = profile_dir.Append("selected.cpu"); + const butil::FilePath base_path = profile_dir.Append("base.cpu"); + ASSERT_EQ(0, butil::WriteFile(view_path, "", 0)); + ASSERT_EQ(0, butil::WriteFile(base_path, "", 0)); + + ClosureChecker done; + brpc::Controller cntl; + brpc::HotspotsRequest req; + brpc::HotspotsResponse res; + brpc::HotspotsService service; + SetUpController(&cntl, true); + cntl.http_request().uri().SetQuery("view", view_path.value()); + cntl.http_request().uri().SetQuery("base", base_path.value()); + service.cpu_non_responsive(&cntl, &req, &res, &done); + EXPECT_FALSE(cntl.Failed()) << cntl.ErrorText(); } TEST_F(BuiltinServiceTest, path_html_escaping) { From f173800d552de943330a97f88520dc9ab1fc2398 Mon Sep 17 00:00:00 2001 From: songgaoye Date: Sat, 14 Nov 2026 14:35:50 +0800 Subject: [PATCH 3/3] Escape builtin directory and profiler paths --- src/brpc/builtin/common.cpp | 14 +++++++++++++- src/brpc/builtin/hotspots_service.cpp | 3 +++ test/brpc_builtin_service_unittest.cpp | 7 ++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/brpc/builtin/common.cpp b/src/brpc/builtin/common.cpp index 929307b03d..1e1ff05bb6 100644 --- a/src/brpc/builtin/common.cpp +++ b/src/brpc/builtin/common.cpp @@ -26,6 +26,7 @@ #include "butil/third_party/murmurhash3/murmurhash3.h" #include "butil/process_util.h" // ReadCommandLine #include "brpc/controller.h" +#include "brpc/grpc.h" #include "brpc/server.h" #include "brpc/reloadable_flags.h" #include "brpc/builtin/common.h" @@ -123,6 +124,17 @@ void PrintRealDateTime(std::ostream& os, int64_t tm, } } +static std::string EncodeUriPath(const char* path) { + std::string encoded; + PercentEncode(path, &encoded); + size_t pos = 0; + while ((pos = encoded.find("%2f", pos)) != std::string::npos) { + encoded.replace(pos, 3, "/"); + ++pos; + } + return encoded; +} + std::ostream& operator<<(std::ostream& os, const PrintedAsDateTime& d) { PrintRealDateTime(os, d.realtime); return os; @@ -135,7 +147,7 @@ std::ostream& operator<<(std::ostream& os, const Path& link) { return os; } - const std::string escaped_uri = WebEscape(link.uri); + const std::string escaped_uri = WebEscape(EncodeUriPath(link.uri)); if (link.html_addr != Path::LOCAL) { os << ""; } else { diff --git a/src/brpc/builtin/hotspots_service.cpp b/src/brpc/builtin/hotspots_service.cpp index 1d18f9f14a..71c72201d5 100644 --- a/src/brpc/builtin/hotspots_service.cpp +++ b/src/brpc/builtin/hotspots_service.cpp @@ -1065,6 +1065,9 @@ static void StartProfiling(ProfilingType type, for (butil::FilePath name = prof_enum.Next(); !name.empty(); name = prof_enum.Next()) { // NOTE: name already includes dir. + if (!ValidProfilePath(name.value())) { + continue; + } if (past_profs.empty()) { past_profs.reserve(16); } diff --git a/test/brpc_builtin_service_unittest.cpp b/test/brpc_builtin_service_unittest.cpp index fd7595e373..f3f666b313 100644 --- a/test/brpc_builtin_service_unittest.cpp +++ b/test/brpc_builtin_service_unittest.cpp @@ -843,7 +843,7 @@ TEST_F(BuiltinServiceTest, hotspots_profile_filename_escaping) { const std::string& body = cntl.response_attachment().to_string(); EXPECT_EQ(std::string::npos, body.find(filename)) << "unescaped profile filename in html: " << body; - EXPECT_NE(std::string::npos, body.find(brpc::WebEscape(filename))); + EXPECT_EQ(std::string::npos, body.find(brpc::WebEscape(filename))); } @@ -879,6 +879,11 @@ TEST_F(BuiltinServiceTest, path_html_escaping) { EXPECT_EQ(std::string::npos, html.str().find(payload)); EXPECT_NE(std::string::npos, html.str().find(brpc::WebEscape(payload))); + std::ostringstream reserved_html; + reserved_html << brpc::Path("/dir/a?x#fragment", brpc::Path::LOCAL); + EXPECT_NE(std::string::npos, + reserved_html.str().find("href=\"/dir/a%3fx%23fragment\"")); + std::ostringstream text; text << brpc::Path(payload.c_str(), nullptr, payload.c_str()); EXPECT_EQ(payload, text.str());