Copilot commented on code in PR #13887:
URL: https://github.com/apache/trafficserver/pull/13887#discussion_r4345569201
##########
src/cripts/Plugins.cc:
##########
@@ -39,16 +39,13 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
const cripts::string &to_url, const Plugin::Options
&options)
{
Plugin::Remap inst;
- int argc = options.size() + 1;
- const char **argv = new const char *[argc];
const swoc::file::path path(plugin);
// Remap plugins expect the first two arguments to be the from and to URLs.
- argv[0] = from_url.c_str();
- argv[1] = to_url.c_str();
+ std::vector<const char *> argv{from_url.c_str(), to_url.c_str()};
- for (unsigned i = 0; i < options.size(); ++i) {
- argv[i + 2] = options[i].c_str();
+ for (const auto &option : options) {
+ argv.push_back(option.c_str());
}
Review Comment:
Consider adding a null sentinel `argv[argc] == nullptr` for maximum
compatibility with C-style argv consumers (some code iterates until nullptr
even when argc is provided). If you add it, ensure you don’t include the
sentinel in `argc` (e.g., append `nullptr` but pass `argv.size() - 1` as
`argc`).
##########
src/cripts/Plugins.cc:
##########
@@ -39,16 +39,13 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
const cripts::string &to_url, const Plugin::Options
&options)
{
Plugin::Remap inst;
- int argc = options.size() + 1;
- const char **argv = new const char *[argc];
const swoc::file::path path(plugin);
// Remap plugins expect the first two arguments to be the from and to URLs.
- argv[0] = from_url.c_str();
- argv[1] = to_url.c_str();
+ std::vector<const char *> argv{from_url.c_str(), to_url.c_str()};
- for (unsigned i = 0; i < options.size(); ++i) {
- argv[i + 2] = options[i].c_str();
+ for (const auto &option : options) {
+ argv.push_back(option.c_str());
}
Review Comment:
The code passes `char **` to the plugin factory via `const_cast` even though
the backing storage is `const char*` obtained from `c_str()`. If a remap plugin
(or the factory) mutates any argument string, that’s undefined behavior. Prefer
building a mutable argv buffer: keep a `std::vector<std::string>` (owning
copies) and then a parallel `std::vector<char*>` pointing at mutable
`.data()`/`&str[0]` buffers, and pass that to `getRemapPlugin()`.
##########
src/cripts/Plugins.cc:
##########
@@ -60,11 +57,10 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
elevate_access =
RecGetRecordInt("proxy.config.plugin.load_elevated").value_or(0);
ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0);
- inst._plugin = gPluginFactory.getRemapPlugin(path, argc, const_cast<char
**>(argv), error, isPluginDynamicReloadEnabled());
+ inst._plugin = gPluginFactory.getRemapPlugin(path,
static_cast<int>(argv.size()), const_cast<char **>(argv.data()), error,
+
isPluginDynamicReloadEnabled());
Review Comment:
Consider adding a null sentinel `argv[argc] == nullptr` for maximum
compatibility with C-style argv consumers (some code iterates until nullptr
even when argc is provided). If you add it, ensure you don’t include the
sentinel in `argc` (e.g., append `nullptr` but pass `argv.size() - 1` as
`argc`).
##########
src/cripts/Plugins.cc:
##########
@@ -60,11 +57,10 @@ Plugin::Remap::Create(const std::string &tag, const
std::string &plugin, const c
elevate_access =
RecGetRecordInt("proxy.config.plugin.load_elevated").value_or(0);
ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0);
- inst._plugin = gPluginFactory.getRemapPlugin(path, argc, const_cast<char
**>(argv), error, isPluginDynamicReloadEnabled());
+ inst._plugin = gPluginFactory.getRemapPlugin(path,
static_cast<int>(argv.size()), const_cast<char **>(argv.data()), error,
+
isPluginDynamicReloadEnabled());
Review Comment:
The code passes `char **` to the plugin factory via `const_cast` even though
the backing storage is `const char*` obtained from `c_str()`. If a remap plugin
(or the factory) mutates any argument string, that’s undefined behavior. Prefer
building a mutable argv buffer: keep a `std::vector<std::string>` (owning
copies) and then a parallel `std::vector<char*>` pointing at mutable
`.data()`/`&str[0]` buffers, and pass that to `getRemapPlugin()`.
--
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]
