Copilot commented on code in PR #12961:
URL: https://github.com/apache/trafficserver/pull/12961#discussion_r2934491267
##########
src/iocore/net/NetVConnection.cc:
##########
@@ -32,6 +32,7 @@
#include "iocore/net/NetVConnection.h"
#include "iocore/eventsystem/IOBuffer.h"
+#include "records/RecCore.h"
Review Comment:
`#include "records/RecCore.h"` appears unused in this file (no other `Rec*`
symbols referenced). Consider dropping it to avoid unnecessary
dependencies/compile time.
##########
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 condition can be triggered by malformed or intentionally truncated PPv2
headers (e.g., a large `len` field with insufficient bytes), potentially
causing high-volume `error.log` spam. Consider downgrading to `Dbg`/`Warning`
with throttling (or logging at the call site with more context, like the
configured max header size) to avoid operational impact.
##########
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:
+
+ Sets the maximum size of PROXY protocol header to receive.
+ The default size is enough for PROXY protocol version 1. The size needs to
be increased
+ if the version 2 is used with many TLV fields. Although you can set a
number up to 65535,
+ setting a large number can affect performance.
Review Comment:
Docs say you can set this value "up to 65535" for v2 with many TLVs, but
PPv2’s total header size can be 16 + len (up to 65551). If the implementation
keeps a 65535 cap, it would be helpful to clarify in the docs what the limit
applies to (total bytes vs v2 `len`) and note the effective maximum v2 header
size that can be parsed.
##########
src/proxy/ProtocolProbeSessionAccept.cc:
##########
@@ -127,8 +127,10 @@ struct ProtocolProbeTrampoline : public Continuation,
public ProtocolProbeSessio
"ioCompletionEvent: proxy protocol DOES NOT have a configured
allowlist of trusted IPs but proxy protocol is "
"ernabled on this port - processing all connections");
Review Comment:
Typo in the debug message: "ernabled" should be "enabled".
##########
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:
The configured max header size is capped at 65535, but for PROXY protocol v2
the on-wire total header size can be 16 (fixed header) + `len` (0..65535) = up
to 65551 bytes. With the current cap, a maximum-size valid v2 header can never
be fully read/parsed. Consider either (a) defining this config as the maximum
v2 `len` field, or (b) raising the allowed maximum to 65551 (and documenting
that it’s total bytes).
##########
src/iocore/net/NetVConnection.cc:
##########
@@ -51,11 +52,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, which is not valid C++20
and will fail to compile on standard-conforming toolchains. Also,
`max_header_size` can be as large as 65535, which would put a large buffer on
the stack per call. Prefer a heap-backed buffer (e.g.,
`std::vector<char>`/`std::string`) and consider capping/validating the size
used for allocation/read.
--
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]
