Copilot commented on code in PR #13733:
URL: https://github.com/apache/trafficserver/pull/13733#discussion_r4350834792
##########
tests/uranium_tests/pluginTest/crash_test/crash_test.cc:
##########
@@ -24,19 +24,30 @@
* This plugin is for TESTING ONLY - do not use in production!
*
* When a request contains the header "X-Crash-Test: now", this plugin
- * will dereference a null pointer, causing a SIGSEGV.
+ * will dereference a null pointer, causing a SIGSEGV. When the header value
+ * is "throw", it throws a std::runtime_error out of the hook instead, which
+ * nothing catches.
*/
#include <ts/ts.h>
#include <cstring>
#include <cstdlib>
+#include <stdexcept>
#define PLUGIN_NAME "crash_test"
namespace
{
DbgCtl dbg_ctl{PLUGIN_NAME};
+// Kept out of line so this frame, and its name, shows up in the crash log
+// backtrace of the thread that threw.
+[[gnu::noinline]] void
+crash_test_throw()
+{
+ throw std::runtime_error("crash_test throw");
+}
Review Comment:
`[[gnu::noinline]]` is a compiler-specific attribute. If this test plugin is
expected to compile across the project’s supported toolchains, it would be more
robust to use an existing portability macro (if the codebase has one for
“noinline”) or conditionally apply the attribute per compiler. This keeps the
intent (ensuring the frame appears in crash backtraces) without relying on a
GNU-only spelling.
##########
tests/uranium_tests/pluginTest/crash_test/test_crash_test_throw.py:
##########
@@ -0,0 +1,151 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from pathlib import Path
+import re
+import time
+
+from tools.uranium.services import ATS, ATSFactory, Curl, OriginServer,
ServiceFactory
+
+
+def configure_origin(services: ServiceFactory) -> OriginServer:
+ """Create the origin used to establish a healthy baseline request.
+
+ :param services: Factory owning support services and their cleanup.
+ """
+
+ origin = services.origin("origin")
+ origin.add_response(
+ {
+ "headers": "GET / HTTP/1.1\r\nHost: example.com\r\n\r\n",
+ "body": ""
+ },
+ {
+ "headers": "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection:
close\r\n\r\n",
+ "body": "Hello",
+ },
+ )
+ return origin
+
+
+def configure_ats(ats_factory: ATSFactory, *, _origin: OriginServer) -> ATS:
+ """Load the plugin that throws out of a hook and enable the crash-log
helper.
+
+ The terminate handler aborts, so ATS is expected to exit on SIGABRT.
+
+ :param _origin: Test-local origin configured by the test.
+ :param ats_factory: Factory for isolated Traffic Server instances.
+ """
+
+ ats = ats_factory.create("ts", return_code=-6, enable_cache=False)
+ ats.records.update(
+ {
+ "proxy.config.proxy_name": "test_proxy",
+ "proxy.config.url_remap.remap_required": 0,
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "crash_test",
+ "proxy.config.crash_log_helper": "traffic_crashlog",
+ })
+
ats.copy_custom_plugin("{AtsBuildUraniumTestsDir}/pluginTest/crash_test/.libs/crash_test.so")
+ ats.plugin_config.add_line("crash_test.so")
+ ats.remap_config.add_line(f"map / http://127.0.0.1:{_origin.port}/";)
+ return ats
+
+
+def verify_healthy_request(*, _ats: ATS, _curl: Curl) -> None:
+ """Prove ATS is serving traffic before triggering the fault.
+
+ :param _ats: Test-local ats configured by the test.
+ :param _curl: Test-local curl configured by the test.
+ """
+
+ response = _curl.get(
+ _ats,
+ headers={"Host": "example.com"},
+ options=f"--silent --output /dev/null --write-out '%{{http_code}}'",
+ )
+ assert response.returncode == 0, response.output
+ assert response.stdout == "200"
+
+
+def trigger_throw(*, _ats: ATS, _curl: Curl) -> None:
+ """Send the header that makes crash_test throw a std::runtime_error out of
its hook.
+
+ :param _ats: Test-local ats configured by the test.
+ :param _curl: Test-local curl configured by the test.
+ """
+
+ response = _curl.get(
+ _ats,
+ headers={
+ "Host": "example.com",
+ "X-Crash-Test": "throw"
+ },
+ options=f"--silent --output /dev/null",
+ )
+ assert response.returncode in (52, 56), response.output
+ _ats.wait()
+
+
+def wait_for_crash_log(*, _ats: ATS) -> Path:
+ """Wait until traffic_crashlog has finished writing its report.
+
+ :param _ats: Test-local ats configured by the test.
+ """
+
+ deadline = time.monotonic() + 60
+ while time.monotonic() < deadline:
+ matches = list(_ats.log_directory.glob("crash-*.log"))
+ if matches and "Other Non-Crashing Threads:" in
matches[0].read_text(errors="replace"):
+ return matches[0]
Review Comment:
`glob("crash-*.log")` returns an arbitrary order, but the code always
inspects `matches[0]`. If multiple crash logs exist (e.g., retries, multiple
crashes, or leftover artifacts in a reused directory), this can pick the wrong
file and cause flaky results. Prefer selecting deterministically (e.g., sort by
filename or mtime and inspect the newest, or iterate all matches until one
contains the completion marker).
##########
src/tscore/Demangle.cc:
##########
@@ -0,0 +1,76 @@
+/** @file
+
+ Readable C++ type names.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "tscore/Demangle.h"
+
+#include <cstdlib>
+#include <cxxabi.h>
+#include <exception>
+
+namespace ts
+{
+std::string
+demangle(std::type_info const &type) noexcept
+{
+ try {
+ int status = 0;
+ char *name = abi::__cxa_demangle(type.name(), nullptr, nullptr,
&status);
+ std::string result;
+
+ try {
+ result = status == 0 && name != nullptr ? name : type.name();
+ } catch (...) {
+ std::free(name);
+ throw;
+ }
+ std::free(name);
+ return result;
+ } catch (...) {
+ return "<unknown>";
+ }
+}
+
+std::string
+describe_current_exception() noexcept
+{
+ std::exception_ptr const ep = std::current_exception();
+
+ if (!ep) {
+ return "no active exception";
+ }
+
+ try {
+ try {
+ std::rethrow_exception(ep);
+ } catch (std::exception const &ex) {
+ return demangle(typeid(ex)) + ": " + ex.what();
+ } catch (...) {
+ if (std::type_info const *type = abi::__cxa_current_exception_type();
type != nullptr) {
+ return "exception of type " + demangle(*type);
+ }
+ }
+ } catch (...) {
+ }
+ return "exception";
Review Comment:
`demangle()` / `describe_current_exception()` are marked `noexcept` and
documented as “never throws”, but several `return` statements construct
`std::string` directly from literals or via concatenation outside of any “safe”
allocation strategy. In low-memory situations this can still throw (and,
because of `noexcept`, immediately call `std::terminate`), which is
particularly problematic since these helpers are used from terminate/exception
handlers. Consider restructuring so *all* string construction happens inside
guarded `try` blocks into a local `std::string result` (falling back to an
empty string or other non-allocating representation on failure), and only
`return result;` after the guarded section. Alternatively, drop `noexcept` here
and make all call sites wrap the helper in their own `try/catch` with a fixed
fallback message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
