Copilot commented on code in PR #13269:
URL: https://github.com/apache/trafficserver/pull/13269#discussion_r3430975951
##########
tests/gold_tests/cripts/cripts.test.py:
##########
@@ -91,8 +106,25 @@ def runCertsTest(self):
tr.Processes.Default.Streams.stderr = "gold/certs_cript.gold"
tr.StillRunningAfter = self.server
+ def runQueryCopyTest(self):
+ tr = Test.AddTestRun('Exercise Query copy ctor / copy-assign in
cripts.')
+ tr.MakeCurlCommand(
+ f'-v -H "Host: query.example.com"
"http://127.0.0.1:{self.ts.Variables.port}/query?foo=1&bar=2&baz=3";',
ts=self.ts)
+ tr.Processes.Default.ReturnCode = 0
+ # Live URL is untouched: original query is preserved.
+ tr.Processes.Default.Streams.stderr = Testers.ContainsExpression(
+ r"X-Original-Query: foo=1&bar=2&baz=3", "Original query must
round-trip unchanged")
+ # Mutating the copy (Erase "foo") must not have leaked into the
original.
+ tr.Processes.Default.Streams.stderr += Testers.ContainsExpression(
+ r"X-Copy-Query: bar=2&baz=3", "Erase on the copy must drop foo
without affecting the original")
+ # Copy-assignment path produces a snapshot equal to the original.
+ tr.Processes.Default.Streams.stderr += Testers.ContainsExpression(
+ r"X-Assigned-Query: foo=1&bar=2&baz=3", "Copy-assigned query must
equal the original")
Review Comment:
These assertions hard-code the serialized query parameter order (e.g.,
`bar=2&baz=3`). If the query serialization preserves order today this passes,
but it’s brittle if implementations change (e.g., hashing/map iteration order,
normalization rules). Consider matching in an order-insensitive way (e.g.,
regex allowing either order, or parsing the header value into pairs and
comparing as sets) so the test focuses on the copy/isolation behavior rather
than ordering.
##########
tests/gold_tests/cripts/gold/basic_cript.gold:
##########
@@ -7,6 +7,7 @@
``
< HTTP/1.1 200 OK
< responseHeader: changed
+< Content-Length: 0
Review Comment:
This PR is described as enabling `Url::Query` copy via deep-copying
`_state`, but it also changes baseline gold output for existing tests by adding
`Content-Length: 0` (and similarly in `certs_cript.gold`). If this is an
intentional side-effect of fixing the origin response headers in
`cripts.test.py`, it would help to either (a) scope these header/gold updates
to the new query-copy test only, or (b) call out in the PR description why
existing gold outputs are expected to change.
##########
include/cripts/Urls.hpp:
##########
@@ -485,6 +485,25 @@ class Url
_state->standalone = true;
}
+ // Query owns a std::unique_ptr<State>, which deletes the implicit copy
operations.
+ // NOTE: not a true deep copy — `_owner` is shared, so Flush() on a copy
writes back
+ // to the source URL via TSUrlHttpQuerySet.
+ Query(const self_type &o) : Component(o), _state(o._state ?
std::make_unique<State>(*o._state) : nullptr) {}
+
+ self_type &
+ operator=(const self_type &o)
+ {
+ if (this != &o) {
+ auto new_state = o._state ? std::make_unique<State>(*o._state) :
nullptr;
+ Component::operator=(o);
+ _state = std::move(new_state);
+ }
+ return *this;
+ }
+
+ Query(self_type &&) = default;
+ self_type &operator=(self_type &&) = default;
Review Comment:
Deep-copying `State` is likely to copy `string_view` members (or other
non-owning references) that still point into TS-owned URL/query buffers.
Because `_owner` is shared and the query buffer can be replaced/invalidated by
operations like `Flush()`/`TSUrlHttpQuerySet`, a copied `Query` can end up with
dangling `string_view`s and trigger use-after-free/invalid memory reads. A
safer approach is to make copied instances start with `_state == nullptr`
(forcing a re-parse on first use), or to change `State` to own the strings it
references (or implement a generation/invalidation mechanism tied to `_owner`
mutations). At minimum, the API contract should clearly warn that copies are
not safe to use after the underlying URL/query mutates.
--
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]
