Copilot commented on code in PR #12961:
URL: https://github.com/apache/trafficserver/pull/12961#discussion_r2934506020
##########
src/iocore/net/NetVConnection.cc:
##########
@@ -51,11 +51,19 @@ DbgCtl dbg_ctl_ssl{"ssl"};
If the buffer has PROXY Protocol, it will be consumed by this function.
*/
bool
-NetVConnection::has_proxy_protocol(IOBufferReader *reader)
+NetVConnection::has_proxy_protocol(IOBufferReader *reader, int max_header_size)
{
- char buf[PPv1_CONNECTION_HEADER_LEN_MAX + 1];
swoc::TextView tv;
- tv.assign(buf, reader->memcpy(buf, sizeof(buf), 0));
+
+ char preface[PPv2_CONNECTION_HEADER_LEN];
+ tv.assign(preface, reader->memcpy(preface, sizeof(preface), 0));
+ if (!proxy_protocol_detect(tv)) {
+ return false;
+ }
+
+ int bufsize = max_header_size;
+ char buf[bufsize];
+ tv.assign(buf, reader->memcpy(buf, bufsize, 0));
Review Comment:
`char buf[bufsize];` is a variable-length array (VLA), which is not valid
C++20 and will fail to compile on standards-conforming compilers. Also,
allocating up to ~64KB (configurable) on the stack per call is risky. Use a
heap-backed buffer (e.g., `std::vector<char>`/`std::string`) and keep `bufsize`
as `size_t` (optionally clamp to a sane maximum).
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -5234,6 +5234,19 @@ UDP Configuration
Enables (``1``) or disables (``0``) UDP GRO. When enabled, |TS| will try to
use it
when reading the UDP socket.
+
+PROXY protocol Configuration
+=============================
+
+.. ts:cv:: CONFIG proxy.config.proxy_protocol.max_header_size INT 109
+ :reloadable:
Review Comment:
Docs mark `proxy.config.proxy_protocol.max_header_size` as `:reloadable:`,
but `RecordsConfig.cc` currently declares it as `RECU_RESTART_TS`
(restart-required). Either update the record to be dynamic, or adjust the
documentation so the reload semantics are accurate.
##########
src/iocore/net/ProxyProtocol.cc:
##########
@@ -237,7 +238,7 @@ proxy_protocol_v2_parse(ProxyProtocol *pp_info, const
swoc::TextView &msg)
uint16_t tlv_len = 0;
if (msg.size() < total_len) {
- Dbg(dbg_ctl_proxyprotocol_v2, "The amount of available data is smaller
than the expected size");
+ Error("The size of PP header received (%zu) is smaller than the expected
size (%zu)", msg.size(), total_len);
Review Comment:
This now logs at `Error` level when the received buffer is shorter than the
v2 header’s declared length. That condition can occur with partial reads
(normal) or when `max_header_size` is configured too small, and will spam
`error.log` under load. Consider reverting to `Dbg` (or at most a rate-limited
`Warning`) and/or treating this as “need more data” rather than an error.
##########
src/records/RecordsConfig.cc:
##########
@@ -1550,7 +1550,14 @@ static constexpr RecordElement RecordsConfig[] =
//# Thread watchdog
//#
//###########
- {RECT_CONFIG, "proxy.config.exec_thread.watchdog.timeout_ms", RECD_INT, "0",
RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-10000]", RECA_NULL}
+ {RECT_CONFIG, "proxy.config.exec_thread.watchdog.timeout_ms", RECD_INT, "0",
RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-10000]", RECA_NULL},
+
+ //###########
+ //#
+ //# PROXY protocol
+ //#
+ //###########
+ {RECT_CONFIG, "proxy.config.proxy_protocol.max_header_size", RECD_INT,
"109", RECU_RESTART_TS, RR_NULL, RECC_INT, "[109-65535]", RECA_NULL},
Review Comment:
This record is marked `RECU_RESTART_TS`, but the value is read into
`HttpConfig` and copied into per-thread `HttpConfigParams` during
`HttpConfig::reconfigure()`, and the docs mark it as `:reloadable:`. To avoid
confusing/incorrect behavior, this should likely be `RECU_DYNAMIC` (consistent
with other size limits like `proxy.config.http.request_line_max_size`).
##########
src/iocore/net/ProxyProtocol.cc:
##########
@@ -453,6 +454,18 @@ proxy_protocol_v2_build(uint8_t *buf, size_t max_buf_len,
const ProxyProtocol &p
} // namespace
+bool
+proxy_protocol_detect(swoc::TextView tv)
+{
+ if (tv.size() >= PPv1_CONNECTION_HEADER_LEN_MIN &&
tv.starts_with(PPv1_CONNECTION_PREFACE)) {
+ return true;
+ } else if (tv.size() >= PPv2_CONNECTION_HEADER_LEN &&
tv.starts_with(PPv2_CONNECTION_PREFACE)) {
+ return true;
+ } else {
+ return false;
+ }
Review Comment:
New helper `proxy_protocol_detect()` isn’t covered by the existing PROXY
protocol unit tests (there are already extensive tests for
`proxy_protocol_parse`). Please add a few focused cases (v1/v2 positive
detection, and negative/partial buffers) to prevent regressions in the
probe/reader 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
