Copilot commented on code in PR #12304:
URL: https://github.com/apache/trafficserver/pull/12304#discussion_r2160417343
##########
plugins/header_rewrite/ruleset.h:
##########
@@ -39,23 +39,44 @@ class RuleSet
{
public:
// Holding the IF and ELSE operators and mods, in two separate linked lists.
- struct OperatorPair {
- OperatorPair() = default;
+ struct OperatorAndMods {
+ OperatorAndMods() = default;
- OperatorPair(const OperatorPair &) = delete;
- OperatorPair &operator=(const OperatorPair &) = delete;
+ OperatorAndMods(const OperatorAndMods &) = delete;
+ OperatorAndMods &operator=(const OperatorAndMods &) = delete;
Operator *oper = nullptr;
OperModifiers oper_mods = OPER_NONE;
};
+ struct CondOpSection {
+ CondOpSection() = default;
+
+ ~CondOpSection()
+ {
+ delete ops.oper;
+ delete next;
Review Comment:
Consider potential risks of recursive deletion in the ~CondOpSection
destructor. If many 'elif' sections are present, this recursive deletion might
lead to a stack overflow in extreme cases; consider documenting the expected
upper bounds or refactoring to an iterative deletion mechanism.
```suggestion
CondOpSection *current = this->next;
while (current) {
CondOpSection *next_section = current->next;
current->next = nullptr; // Avoid dangling pointers
delete current;
current = next_section;
}
```
##########
plugins/header_rewrite/header_rewrite.cc:
##########
@@ -115,28 +115,24 @@ class RulesConfig
bool parse_config(const std::string &fname, TSHttpHookID default_hook, char
*from_url = nullptr, char *to_url = nullptr);
private:
- bool add_rule(RuleSet *rule);
+ void add_rule(std::unique_ptr<RuleSet> rule);
TSCont _cont;
RuleSet *_rules[TS_HTTP_LAST_HOOK + 1];
ResourceIDs _resids[TS_HTTP_LAST_HOOK + 1];
};
-// Helper function to add a rule to the rulesets
-bool
-RulesConfig::add_rule(RuleSet *rule)
+void
+RulesConfig::add_rule(std::unique_ptr<RuleSet> rule)
{
- if (rule && rule->has_operator()) {
- Dbg(dbg_ctl, " Adding rule to hook=%s",
TSHttpHookNameLookup(rule->get_hook()));
- if (nullptr == _rules[rule->get_hook()]) {
- _rules[rule->get_hook()] = rule;
- } else {
- _rules[rule->get_hook()]->append(rule);
- }
- return true;
- }
+ auto raw = rule.release();
- return false;
+ Dbg(pi_dbg_ctl, " Adding rule to hook=%s",
TSHttpHookNameLookup(raw->get_hook()));
+ if (nullptr == _rules[raw->get_hook()]) {
+ _rules[raw->get_hook()] = raw;
+ } else {
+ _rules[raw->get_hook()]->append(raw);
Review Comment:
Transferring ownership from a unique_ptr to a raw pointer via release() can
be error-prone. Consider using smart pointers consistently for the _rules array
to improve memory safety and ease maintenance.
--
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]
