drivers/net/telnet: Ignore unsupported subnegotiation data#18590
drivers/net/telnet: Ignore unsupported subnegotiation data#18590anchao merged 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a telnet parsing regression where subnegotiation (IAC SB …) bytes could bleed into the first NSH command when NAWS is disabled, by ensuring unsupported subnegotiation payload is consumed until IAC SE.
Changes:
- Adds a new parser state to discard unsupported telnet subnegotiation payload until the IAC SE terminator.
- Refactors the subnegotiation handling so NAWS (when enabled) continues to parse window size while other sub-options are ignored safely.
- Introduces explicit handling of the IAC SE terminator sequence in the state machine.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
linguini1
left a comment
There was a problem hiding this comment.
Please fill out the PR template according to the contributing guide.
|
@ankohuu thank you for this fix! Please include the testing before and after this patch. |
There was a problem hiding this comment.
- Thank you @ankohuu :-)
- Please fill in the PR details using template (see https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md).
- Please provide testing logs before and after the change.
206cdfa to
3e054a4
Compare
Ignore unsupported telnet subnegotiation payload until `IAC SE` so option bytes do not leak into the first NSH command. Keep existing NAWS handling intact and also treat `IAC IAC` inside subnegotiation payload as an escaped `0xFF` data byte rather than a subnegotiation terminator. This makes subnegotiation parsing RFC-compliant for both unsupported options and NAWS payload processing. Signed-off-by: Shunchao Hu <ankohuu@gmail.com>
ef60ec9 to
4094fff
Compare
|
Refactor to address copilot's two comments, already tested locally, will update pr details using template later. Thank you all for your comments and help. |
@ankohuu Got it. request the review from Copilot again. I think we can refer to Copilot's review comments, but we don't necessarily have to follow them strictly. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Just PR update and testing logs and we are ready to go, thanks @ankohuu :-) |
Summary
Fix telnet sub-negotiation parsing so unsupported sub-option payload does not
leak into the first NSH command.
The issue happens when the telnet client sends
IAC SB ... IAC SEsub-negotiation data and the option is not handled by the current build,
for example when
CONFIG_TELNET_SUPPORT_NAWSis disabled orsome new sub-options. In that case, the previous implementation could
return to normal input processing and let sub-negotiation payload
bytes appear as part of the first command read by NSH.
This change keeps consuming unsupported sub-negotiation payload until
IAC SE, and also handles escapedIAC IACcorrectly insidesub-negotiation payload so a literal
0xFFdata byte is not mistaken forthe sub-negotiation terminator.
NAWS handling remains unchanged in functionality, but now follows the
same sub-negotiation framing rules more strictly.
Reference:
Impact
bugfix
Testing
I confirm that changes are verified on local setup and works as intended:
telnetd &from NSH as telnet serverTest python pseudo code
Testing logs before change:
Testing logs after change with and without CONFIG_TELNET_SUPPORT_NAWS config: