[PR] url_sig: modernize memory management [trafficserver]

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt opened a new pull request, #12569:
URL: https://github.com/apache/trafficserver/pull/12569

   This updates url_sig to manage memory via constructors, unique_ptr, std::string, and the like.


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] url_sig: modernize memory management [trafficserver]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #12569:
URL: https://github.com/apache/trafficserver/pull/12569#discussion_r2456093407


##########
plugins/experimental/url_sig/url_sig.cc:
##########
@@ -836,10 +832,10 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri)
 
   switch (cfg->err_status) {
   case TS_HTTP_STATUS_MOVED_TEMPORARILY:
-    Dbg(dbg_ctl, "Redirecting to %s", cfg->err_url);
+    Dbg(dbg_ctl, "Redirecting to %s", cfg->err_url.c_str());
     char *start, *end;
-    start = cfg->err_url;
-    end   = start + strlen(cfg->err_url);
+    start = const_cast<char *>(cfg->err_url.c_str());
+    end   = start + cfg->err_url.size();
     if (TSUrlParse(rri->requestBufp, rri->requestUrl, (const char **)&start, end) != TS_PARSE_DONE) {
       err_log("url", 3, "Error inn TSUrlParse!");

Review Comment:
   Corrected spelling of 'inn' to 'in'.
   ```suggestion
         err_log("url", 3, "Error in TSUrlParse!");
   ```



##########
plugins/experimental/url_sig/url_sig.cc:
##########
@@ -836,10 +832,10 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri)
 
   switch (cfg->err_status) {
   case TS_HTTP_STATUS_MOVED_TEMPORARILY:
-    Dbg(dbg_ctl, "Redirecting to %s", cfg->err_url);
+    Dbg(dbg_ctl, "Redirecting to %s", cfg->err_url.c_str());
     char *start, *end;
-    start = cfg->err_url;
-    end   = start + strlen(cfg->err_url);
+    start = const_cast<char *>(cfg->err_url.c_str());

Review Comment:
   Using const_cast to remove const from std::string::c_str() is unsafe if TSUrlParse modifies the string. This violates std::string's internal buffer guarantees. If TSUrlParse requires a mutable buffer, copy err_url to a mutable buffer first.



##########
plugins/experimental/url_sig/url_sig.cc:
##########
@@ -582,7 +577,8 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo *rri)
 
     // check for path params.
     if (query == nullptr || strstr(query, "E=") == nullptr) {
-      char *const parsed = urlParse(url, cfg->sig_anchor, new_path, 8192, path_params, 8192);
+      char *const parsed = urlParse(url, cfg->sig_anchor.empty() ? nullptr : const_cast<char *>(cfg->sig_anchor.c_str()), new_path,

Review Comment:
   The const_cast here suggests urlParse may modify the sig_anchor string, which is unsafe when passing std::string::c_str(). Consider refactoring urlParse to accept const char* if it doesn't modify the string, or document this requirement clearly.
   ```suggestion
         // Create a mutable copy of sig_anchor if not empty
         std::string sig_anchor_copy;
         char *sig_anchor_ptr = nullptr;
         if (!cfg->sig_anchor.empty()) {
           sig_anchor_copy = cfg->sig_anchor;
           sig_anchor_ptr = &sig_anchor_copy[0];
         }
         char *const parsed = urlParse(url, cfg->sig_anchor.empty() ? nullptr : sig_anchor_ptr, new_path,
   ```



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] url_sig: modernize memory management [trafficserver]

["bneradt (via GitHub)" <gi...@apache.org>]

bneradt merged PR #12569:
URL: https://github.com/apache/trafficserver/pull/12569


-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org