Copilot commented on code in PR #13837:
URL: https://github.com/apache/trafficserver/pull/13837#discussion_r4273180693
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string
&fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in
'" + fileName + "'");
+ }
+ std::string f = include;
Review Comment:
Parsing with `strtok_r(const_cast<char*>(line.c_str()), ...)` mutates the
buffer returned by `std::string::c_str()`, which is undefined behavior in C++.
Since this code is being touched, consider switching to a non-mutating parse
approach (e.g., trim + prefix check + substring, `std::string_view`
tokenization, or `std::istringstream`) to avoid UB and make the include parsing
more robust.
##########
src/proxy/http/remap/unit-tests/include-missing-strategy.yaml:
##########
@@ -0,0 +1,37 @@
+# @file
+#
+# Unit test data strategy.yaml file for testing the NextHopStrategyFactory
+#
+# @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.
+#
+# @section details Details
+#
+#
+# unit test strategy.yaml with a '#include' directive missing its file name
+#
+#include
+#
+strategies:
+ - strategy: 'strategy-1'
+ policy: consistent_hash
+ hash_key: cache_key
+ go_direct: false
+ groups:
+ - *g1
+ scheme: http
Review Comment:
This fixture includes an undefined YAML alias (`*g1`). It currently won’t
matter because the loader throws on the bare `#include` before YAML parse, but
it makes the test data fragile (future refactors to include handling could
cause the test to fail for the wrong reason). To keep the fixture focused,
either remove the `strategies` block entirely (since the test only checks that
nothing loads) or define a local anchor for `g1` so the YAML is internally
valid.
##########
src/proxy/http/remap/unit-tests/test_NextHopStrategyFactory.cc:
##########
@@ -40,6 +40,17 @@
SCENARIO("factory tests loading yaml configs", "[loadConfig]")
{
+ GIVEN("Loading a strategy.yaml with a bare '#include' directive.")
+ {
+ NextHopStrategyFactory nhf(TS_SRC_DIR "/include-missing-strategy.yaml");
+
+ THEN("no strategies are loaded")
+ {
+ REQUIRE(nhf.strategies_loaded == false);
+ REQUIRE(nhf.strategyInstance("strategy-1") == nullptr);
+ }
+ }
Review Comment:
This test validates the failure outcome, but not the new behavior promised
by the PR description (a clear error for bare `#include`). Consider asserting
that the emitted error/exception text contains something like `'#include'
without a file name` (e.g., by capturing logs or by exposing/returning the load
error) so regressions don’t silently pass with a different/less helpful failure
mode.
##########
src/proxy/http/remap/NextHopStrategyFactory.cc:
##########
@@ -254,14 +254,18 @@ NextHopStrategyFactory::loadConfigFile(const std::string
&fileName, std::strings
if (line[0] == '#') {
tok = strtok_r(const_cast<char *>(line.c_str()), sep, &last);
if (tok != nullptr && strcmp(tok, "#include") == 0) {
- std::string f = strtok_r(nullptr, sep, &last);
+ const char *include = strtok_r(nullptr, sep, &last);
+ if (include == nullptr) {
+ throw std::invalid_argument("'#include' without a file name in
'" + fileName + "'");
+ }
+ std::string f = include;
if (include_once.find(f) == include_once.end()) {
include_once.insert(f);
// try to load included file.
try {
loadConfigFile(f, doc, include_once);
} catch (std::exception &ex) {
- throw std::invalid_argument("Unable to open included file '" +
f + "' from '" + fileName + "'");
+ throw std::invalid_argument("Unable to load included file '" +
f + "' from '" + fileName + "': " + ex.what());
}
Review Comment:
The catch should take `const std::exception&` (exceptions are typically
caught by const reference). Also, if retaining more diagnostic context is
important, consider preserving the original exception via
`std::throw_with_nested` (instead of only flattening into a new message), while
still keeping the friendly top-level 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]
