Copilot commented on code in PR #13059:
URL: https://github.com/apache/trafficserver/pull/13059#discussion_r3060869191
##########
src/proxy/ProxyTransaction.cc:
##########
@@ -290,3 +293,127 @@ ProxyTransaction::mark_as_tunnel_endpoint()
ink_assert(nvc != nullptr);
nvc->mark_as_tunnel_endpoint();
}
+
+namespace
+{
+/** Build a best-effort request target for access logging. */
+std::string
+synthesize_request_target(std::string_view method, std::string_view scheme,
std::string_view authority, std::string_view path)
+{
+ if (method == static_cast<std::string_view>(HTTP_METHOD_CONNECT)) {
+ if (!authority.empty()) {
+ return std::string(authority);
+ }
+ if (!path.empty()) {
+ return std::string(path);
+ }
+ return {};
+ }
+
+ if (!scheme.empty() && !authority.empty()) {
+ std::string url;
+ url.reserve(scheme.size() + authority.size() + path.size() + 4);
+ url.append(scheme);
+ url.append("://");
+ url.append(authority);
+ if (!path.empty()) {
+ url.append(path);
+ } else {
+ url.push_back('/');
+ }
+ return url;
+ }
+
+ if (!path.empty()) {
+ return std::string(path);
+ }
+
+ return authority.empty() ? std::string{} : std::string(authority);
+}
+
+std::string_view
+get_pseudo_header_value(HTTPHdr const &hdr, std::string_view name)
+{
+ if (auto const *field = hdr.field_find(name); field != nullptr) {
+ return field->value_get();
+ }
+ return {};
+}
+} // end anonymous namespace
+
+void
+ProxyTransaction::log_pre_transaction_access(HTTPHdr const *request, const
char *protocol_str)
+{
+ if (get_sm() != nullptr) {
+ return;
+ }
+
+ if (request == nullptr || !request->valid() || request->type_get() !=
HTTPType::REQUEST) {
+ return;
+ }
+
+ ProxySession *ssn = get_proxy_ssn();
+ if (ssn == nullptr) {
+ return;
+ }
+
+ PreTransactionLogData data;
+
+ data.owned_client_request.create(HTTPType::REQUEST, request->version_get());
+ data.owned_client_request.copy(request);
+ data.client_request = &data.owned_client_request;
+ data.client_connection_is_ssl = ssn->ssl() != nullptr;
+
+ auto const method_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_METHOD);
+ auto const scheme_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_SCHEME);
+ auto const authority_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_AUTHORITY);
+ auto const path_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_PATH);
+
+ if (!method_sv.empty()) {
+ data.owned_method.assign(method_sv.data(), method_sv.size());
+ } else {
+ auto const mget = const_cast<HTTPHdr *>(request)->method_get();
+ data.owned_method.assign(mget.data(), mget.size());
+ }
+
+ data.owned_scheme.assign(scheme_sv.data(), scheme_sv.size());
+ data.owned_authority.assign(authority_sv.data(), authority_sv.size());
+ data.owned_path.assign(path_sv.data(), path_sv.size());
+ data.owned_url = synthesize_request_target(data.owned_method,
data.owned_scheme, data.owned_authority, data.owned_path);
+
+ data.client_req_url_str = data.owned_url.c_str();
+ data.client_req_url_len = data.owned_url.size();
+ data.client_req_url_path_str = data.owned_path.c_str();
+ data.client_req_url_path_len = data.owned_path.size();
Review Comment:
`client_req_url_len` / `client_req_url_path_len` are `int` fields, but
`.size()` returns `size_t`. Assigning without a cast can trigger
narrowing-conversion warnings (and potentially -Werror) and is inconsistent
with the unit test which uses `static_cast<int>(...)`. Please cast explicitly
(and consider clamping if extremely long values are possible).
```suggestion
data.client_req_url_len = static_cast<int>(data.owned_url.size());
data.client_req_url_path_str = data.owned_path.c_str();
data.client_req_url_path_len = static_cast<int>(data.owned_path.size());
```
##########
src/proxy/ProxyTransaction.cc:
##########
@@ -290,3 +293,127 @@ ProxyTransaction::mark_as_tunnel_endpoint()
ink_assert(nvc != nullptr);
nvc->mark_as_tunnel_endpoint();
}
+
+namespace
+{
+/** Build a best-effort request target for access logging. */
+std::string
+synthesize_request_target(std::string_view method, std::string_view scheme,
std::string_view authority, std::string_view path)
+{
+ if (method == static_cast<std::string_view>(HTTP_METHOD_CONNECT)) {
+ if (!authority.empty()) {
+ return std::string(authority);
+ }
+ if (!path.empty()) {
+ return std::string(path);
+ }
+ return {};
+ }
+
+ if (!scheme.empty() && !authority.empty()) {
+ std::string url;
+ url.reserve(scheme.size() + authority.size() + path.size() + 4);
+ url.append(scheme);
+ url.append("://");
+ url.append(authority);
+ if (!path.empty()) {
+ url.append(path);
+ } else {
+ url.push_back('/');
+ }
+ return url;
+ }
+
+ if (!path.empty()) {
+ return std::string(path);
+ }
+
+ return authority.empty() ? std::string{} : std::string(authority);
+}
+
+std::string_view
+get_pseudo_header_value(HTTPHdr const &hdr, std::string_view name)
+{
+ if (auto const *field = hdr.field_find(name); field != nullptr) {
+ return field->value_get();
+ }
+ return {};
+}
+} // end anonymous namespace
+
+void
+ProxyTransaction::log_pre_transaction_access(HTTPHdr const *request, const
char *protocol_str)
+{
+ if (get_sm() != nullptr) {
+ return;
+ }
+
+ if (request == nullptr || !request->valid() || request->type_get() !=
HTTPType::REQUEST) {
+ return;
+ }
+
+ ProxySession *ssn = get_proxy_ssn();
+ if (ssn == nullptr) {
+ return;
+ }
+
+ PreTransactionLogData data;
+
+ data.owned_client_request.create(HTTPType::REQUEST, request->version_get());
+ data.owned_client_request.copy(request);
+ data.client_request = &data.owned_client_request;
+ data.client_connection_is_ssl = ssn->ssl() != nullptr;
+
+ auto const method_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_METHOD);
+ auto const scheme_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_SCHEME);
+ auto const authority_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_AUTHORITY);
+ auto const path_sv = get_pseudo_header_value(*request,
PSEUDO_HEADER_PATH);
+
+ if (!method_sv.empty()) {
+ data.owned_method.assign(method_sv.data(), method_sv.size());
+ } else {
+ auto const mget = const_cast<HTTPHdr *>(request)->method_get();
+ data.owned_method.assign(mget.data(), mget.size());
+ }
+
+ data.owned_scheme.assign(scheme_sv.data(), scheme_sv.size());
+ data.owned_authority.assign(authority_sv.data(), authority_sv.size());
+ data.owned_path.assign(path_sv.data(), path_sv.size());
Review Comment:
`scheme_sv` / `authority_sv` / `path_sv` (and the `method_get()` fallback)
can be default-constructed `std::string_view{}` when the pseudo header is
missing, which means `data()==nullptr`. Calling `std::string::assign(ptr, 0)`
with a null pointer is undefined behavior. Please guard each assign with an
`empty()` check (clear the string when empty) before using `.data()`.
```suggestion
if (!mget.empty()) {
data.owned_method.assign(mget.data(), mget.size());
} else {
data.owned_method.clear();
}
}
if (!scheme_sv.empty()) {
data.owned_scheme.assign(scheme_sv.data(), scheme_sv.size());
} else {
data.owned_scheme.clear();
}
if (!authority_sv.empty()) {
data.owned_authority.assign(authority_sv.data(), authority_sv.size());
} else {
data.owned_authority.clear();
}
if (!path_sv.empty()) {
data.owned_path.assign(path_sv.data(), path_sv.size());
} else {
data.owned_path.clear();
}
```
--
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]
