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

{bp-15287} tcp_recvfrom.c:resolve issue in tcp_recvhandler #15362

Merged
merged 1 commit into from
Dec 28, 2024

Conversation

jerpelea
Copy link
Contributor

Summary

malloc a new iob to handle psock_send_eventhandler when tcp_recvhandler calls tcp_newdata to clear dev->d_iob

Impact

RELEASE

Testing

CI

…n tcp_recvhandler calls tcp_newdata to clear dev->d_iob

Signed-off-by: wangchen <[email protected]>
@jerpelea jerpelea changed the title {bp-15287} tcp_recvfrom.c:malloc a new iob to handle psock_send_eventhandler when tcp_recvhandler calls tcp_newdata to clear dev->d_iob {bp-15287} tcp_recvfrom.c:resolve issue in tcp_recvhandler Dec 27, 2024
@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 27, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 27, 2024

[Experimental Bot, please feedback here]

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

  • Insufficient Summary: "malloc a new iob to handle psock_send_eventhandler when tcp_recvhandler calls tcp_newdata to clear dev->d_iob" is too technical and doesn't explain why this change is necessary. What problem does it solve? What was the previous behavior that was incorrect or inefficient? Explain the bug or the improvement this provides at a higher level.

  • Missing Issue References: Are there any related issues in the NuttX or NuttX Apps repositories? If so, link them. Even if there isn't a directly corresponding issue, mentioning a related one can provide context.

  • Incomplete Impact Assessment: "RELEASE" isn't helpful. Go through each impact item specifically:

    • Is new feature added? Is existing feature changed?: Be explicit.
    • Impact on user: Does this change user-facing behavior? Will applications need modification?
    • Impact on build: Does this require changes to Kconfig options or build scripts?
    • Impact on hardware: Does this only affect specific architectures, boards, or drivers?
    • Impact on documentation: Does the documentation need updates? Has the PR provided those updates?
    • Impact on security: Crucially, does this change have any potential security implications? This must be addressed thoroughly.
    • Impact on compatibility: Does this change break backward compatibility?
    • Anything else to consider: Any performance implications? Memory usage changes?
  • Inadequate Testing Description: "CI" isn't sufficient. List the specific build hosts (OS, CPU, compiler) and target platforms (architecture, board, configuration) where the changes were tested locally. Provide meaningful test logs before and after the change demonstrating the fix or improvement. Don't rely solely on CI.

Example of an Improved Description (fill in the specific details):

Summary

This change fixes a bug [link to NuttX issue # if applicable] where data loss could occur under specific conditions in the TCP/IP stack. Previously, when tcp_recvhandler called tcp_newdata to clear dev->d_iob, the psock_send_eventhandler could lose access to necessary data. This PR resolves this by allocating a new I/O buffer (iob) specifically for psock_send_eventhandler, ensuring data integrity.

Impact

  • Is new feature added? Is existing feature changed?: Bug fix for existing TCP/IP stack functionality.
  • Impact on user: NO. This fix should be transparent to the user.
  • Impact on build: NO.
  • Impact on hardware: NO. This change affects the networking stack and is architecture-independent.
  • Impact on documentation: NO.
  • Impact on security: Potentially YES. The previous bug could have led to denial-of-service scenarios. This fix mitigates that risk. [Elaborate further on any security implications.]
  • Impact on compatibility: NO. This is a bug fix and maintains backward compatibility.
  • Anything else to consider: There might be a slight increase in memory usage due to the allocation of an additional iob. This impact is expected to be negligible.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:nsh, ARM:STM32F4DISCOVERY:nsh

Testing logs before change:

[Provide logs demonstrating the bug, e.g., data loss or incorrect behavior]

Testing logs after change:

[Provide logs demonstrating the corrected behavior, e.g., data is correctly transmitted and received]

@xiaoxiang781216 xiaoxiang781216 merged commit 516b732 into apache:releases/12.8 Dec 28, 2024
25 of 27 checks passed
@jerpelea jerpelea deleted the bp-15287 branch December 28, 2024 08:35
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.

4 participants