Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tcp_input: remove tcp_callback(ABORT) when accept conn recv TCP_RESET #15380

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

zhhyu7
Copy link
Contributor

@zhhyu7 zhhyu7 commented Dec 30, 2024

Summary

when accept conn receives TCP_RESET, only accept conn itself should handle the event, and there is no need to notify the listening conn. otherwise, the server's poll will return with POLLERR and POLLHUP. this may cause some applications to execute incorrect logic in the future.

Impact

Testing

cortex-m33 board.

when accept conn receives TCP_RESET, only accept conn itself should handle
the event, and there is no need to notify the listening conn. otherwise,
the server's poll will return with POLLERR and POLLHUP. this may cause
some applications to execute incorrect logic in the future.

Signed-off-by: zhanghongyu <[email protected]>
@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Dec 30, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 30, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to improve it:

Missing Information in Summary:

  • What functional part of the code is being changed? Be specific. e.g., "The TCP/IP stack, specifically the accept() function implementation."
  • How does the change exactly work? Explain the code changes you made to prevent the listening socket from being notified. For example, "The change prevents propagation of the TCP_RESET event from the accepted socket to the listening socket by [explain the mechanism, e.g., modifying a flag, changing an if condition, etc.]."

Missing Information in Impact:

Fill out all impact sections. Even if the answer is "NO," explicitly state it. This PR likely has implications for:

  • Is new feature added? Is existing feature changed? This is a bug fix, so "Existing feature changed" should be marked. Briefly describe the original faulty behavior.
  • Impact on user: Potentially YES. Explain that the incorrect POLLERR/POLLHUP behavior is fixed, which might affect applications relying on (or misinterpreting) the previous behavior. Explain how users who were relying on the bug should adapt.
  • Impact on documentation: If the previous behavior was documented (even implicitly), "YES" is needed. Briefly explain what documentation needs updating. If no documentation changes are needed, explicitly state "NO - The change corrects undocumented, erroneous behavior."
  • Impact on compatibility: Potentially YES if applications were relying on the bug. State "Potentially YES. Applications incorrectly relying on the previous POLLERR/POLLHUP behavior on the listening socket after a TCP_RESET on the accepted socket will need to be adjusted." If you're confident there's no compatibility impact, explain why.

Missing Information in Testing:

  • Build Host(s): Be specific. Example: "Linux Ubuntu 20.04, x86_64, GCC 9.4.0"
  • Target(s): Be very specific. Example: "STM32F746NGH6 (Nucleo-F746ZG board), NuttX configuration: nsh"
  • Testing logs before change: Include actual logs demonstrating the problem. Show that the listening socket receives POLLERR/POLLHUP when a client connection resets.
  • Testing logs after change: Include logs demonstrating that the listening socket no longer receives POLLERR/POLLHUP under the same reset conditions. Show that the accepted socket handles the reset gracefully.

Example of an Improved Summary:

The TCP/IP stack, specifically the accept() function implementation, is changed to correct the handling of TCP_RESET events. Previously, when a client connection reset after being accepted, the listening socket was also notified of the event, resulting in POLLERR and POLLHUP being returned by poll(). This change isolates the TCP_RESET event to the accepted socket, preventing this incorrect notification to the listening socket. This fixes unexpected behavior that could lead to incorrect application logic. Fixes [NuttX Issue # if applicable].

By providing this more complete information, your PR will be much easier to review and understand, increasing its chances of being merged quickly.

@xiaoxiang781216 xiaoxiang781216 merged commit f344a42 into apache:master Dec 31, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants