Repository navigation
Conversation
Change telnet's antiquated corner places where it still used the connection socket directly. Use the transfer send call instead which goes the proper way thought the connection filters.
There was a problem hiding this comment.
🟡 Changes recommended
The new sends ignore partial and zero-byte writes, which can truncate Telnet negotiation messages.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Routes Telnet control messages through connection filters to support proxy tunnels.
Changes:
- Replaces direct socket writes with
Curl_xfer_send(). - Reports filter-aware send errors.
File summaries
| File | Description |
|---|---|
lib/telnet.c |
Routes Telnet negotiation and suboption writes through transfer filters. |
Review details
Suppressed comments (4)
lib/telnet.c:706
- This send may consume fewer than 3 bytes, including zero bytes when the filter is blocked, while returning
CURLE_OK. The code then sends the window-size body, producing a malformed subnegotiation stream. Retry the unsent header before proceeding to the body.
result = Curl_xfer_send(data, tn->subbuffer, 3, FALSE, &nwritten);
lib/telnet.c:713
- The footer is discarded if
Curl_xfer_send()reports a successful short or zero-byte write. This leaves the NAWS subnegotiation unterminated; retry until both footer bytes have been consumed and return any send failure to the transfer.
result = Curl_xfer_send(data, tn->subbuffer + 7, 2, FALSE, &nwritten);
lib/telnet.c:1014
- This XDISPLOC response is silently truncated whenever the connection filter consumes fewer than
lenbytes, becausenwrittenis never checked. Loop or buffer the unconsumed suffix before treating the suboption as sent.
result = Curl_xfer_send(data, temp, len, FALSE, &nwritten);
lib/telnet.c:1046
- This environment response can be much larger than one filter write, but the code ignores
nwrittenand returns success after a partial or zero-byte write. Preserve and retry the remainder so the Telnet subnegotiation is not truncated.
result = Curl_xfer_send(data, temp, len, FALSE, &nwritten);
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bytes_written = swrite(conn->sock[FIRSTSOCKET], buf, 3); | ||
| if(bytes_written < 0) | ||
| failf(data, "Sending data failed (%d)", SOCKERRNO); | ||
| result = Curl_xfer_send(data, buf, 3, FALSE, &nwritten); |
| if(bytes_written < 0) { | ||
| failf(data, "Sending data failed (%d)", SOCKERRNO); | ||
| return CURLE_SEND_ERROR; | ||
| result = Curl_xfer_send(data, temp, len, FALSE, &nwritten); |
There was a problem hiding this comment.
What am I doing here?
| bytes_written = swrite(conn->sock[FIRSTSOCKET], buf, 3); | ||
| if(bytes_written < 0) | ||
| failf(data, "Sending data failed (%d)", SOCKERRNO); | ||
| result = Curl_xfer_send(data, buf, 3, FALSE, &nwritten); |
There was a problem hiding this comment.
If this were a human, violence would creep into my mind.
|
In the mean time, I made this: #22943 |
|
This protocol handler, like all the pingpoing ones, is from another age and time. When Elves still wandered the earth and unicorns were roaming the forests... |
|
the good old days! 🦄 |
Change telnet's antiquated corner places where it still used the connection socket directly. Use the transfer send call instead which goes the proper way thought the connection filters.