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

Add tls cleanup to protect tcp #14866

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

Meissi-jian
Copy link
Contributor

@Meissi-jian Meissi-jian commented Nov 20, 2024

Summary

This PR addresses an issue where abnormal application exits were causing crash in the TCP/IP stack due to tcp conn will be used after free.

Impact

tcp_send / tcp_recv / tcp_connect
Impact on user: YES (Applications using TCP will now have improved stability on abnormal exit).
Impact on build: NO
Impact on hardware: NO
Impact on documentation: YES (Documentation should be updated to describe the new TLS cleanup mechanism and how it prevents issues on application exit)
Impact on security: Potentially YES (If the abnormal exit was exploitable, this fix might improve security. Explain clearly).
Impact on compatibility: NO (If this is true, state it explicitly)

Testing

Target: sim and others product by monkey
Log: see " pthread is being canceled, need to cleanup cb",and device didn't crash

… after free

Problem Description:
Problem occurrence when Thread1 creat connect/recv socket and Thread2 cancel Thread1.
1. Thread2 cancel when Thread1 connect event, will cause DEBUGASSERT in devif_callback_free.Because cb in g_cbfreelist.
2. Thread2 cancel when Thread1 recvfrom sem-wait, when the FIN packet input and will trigger tcp_recvhandler and will crash.Becuase some thread stack data was freed.

Signed-off-by: meijian <[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 Nov 20, 2024
assert: tcp conn and didn't recv ack, tcp_free will assert in L909
and no need to tcp free because tcp close will free by lpwork.

Signed-off-by: meijian <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Nov 20, 2024

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. It's far too brief and lacks critical information. Here's why and how it can be improved:

Summary:

  • Insufficient Detail: "Due to abnormal application exit, so add tls cleanup to protect" is vague. What abnormal application exit? What kind of issues did it cause? Which TLS cleanup is being added? How does this cleanup prevent the issue? Be specific.
  • Missing Context: Which functional part of the code is affected (networking, memory management, etc.)?

Impact:

  • Too Brief: Just saying "tcp" isn't enough. Explain how TCP is impacted. Does this change affect TCP performance? Does it fix a bug related to TCP connections closing? Answer all the impact questions explicitly (YES/NO and explanation). For example:
    • Impact on user: YES (Applications using TCP will now have improved stability on abnormal exit and potentially prevent resource leaks).
    • Impact on build: NO
    • Impact on hardware: NO
    • Impact on documentation: YES (Documentation should be updated to describe the new TLS cleanup mechanism and how it prevents issues on application exit)
    • Impact on security: Potentially YES (If the abnormal exit was exploitable, this fix might improve security. Explain clearly).
    • Impact on compatibility: NO (If this is true, state it explicitly)

Testing:

  • Insufficient Information: "tested passed on product by monkey-test" is not adequate. Provide specifics:
    • Build Host: e.g., Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
    • Target: e.g., sim:qemu-arm
  • Missing Logs: The template requests "Testing logs before change" and "Testing logs after change." These are essential for demonstrating that the change addresses the intended issue and doesn't introduce regressions. Include relevant log snippets (even if short) that show the problem before and the improvement after the change. Even a simple demonstration of a crash before and successful operation after would be much better than nothing.

Example of an Improved Summary:

"This PR addresses an issue where abnormal application exits were causing resource leaks and potential instability in the TCP/IP stack due to incomplete TLS cleanup. The change adds a new cleanup function in the net/tcp directory that is called during application exit to ensure all TLS resources are properly released. This prevents memory leaks and potential socket issues in subsequent TCP connections. Fixes #[NuttX Issue Number]."

By providing more specific information and following the template closely, the PR review process will be much more efficient. Reviewers will have the context they need to understand the change and its implications.

@xiaoxiang781216 xiaoxiang781216 merged commit 6944a16 into apache:master Nov 20, 2024
27 checks passed
@Meissi-jian Meissi-jian deleted the tcp_tls branch November 21, 2024 01:45
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.

5 participants