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

net/socket/getsockopt: move the options check to the upper layer #15899

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zhhyu7
Copy link
Contributor

@zhhyu7 zhhyu7 commented Feb 24, 2025

Summary

check parameters at the top level to avoid errors when assigning values in si_getsockopt.

Impact

N/A

Testing

qemu:local

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Feb 24, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 24, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of what the change does, it lacks crucial information:

  • Missing Explanation of Necessity: Why is this change necessary? Is it fixing a bug? Is it improving performance? What was the problem before the change? This is the most important part of the summary.
  • Missing Scope: What functional part of the code is being changed? Be specific (e.g., "socket option handling", "network stack").
  • Missing Technical Detail: How does the change work? What specific code was modified? What are the mechanics of the fix/improvement?
  • Missing Issue References: Are there any related NuttX issues? Even if there aren't, state that explicitly (e.g., "No related issues").
  • Insufficient Impact Assessment: "N/A" is rarely acceptable. Think carefully. Even seemingly small changes can have unexpected impacts. Address all impact categories specifically, even if the answer is "NO". Justify your "NO" answers briefly (e.g., "Impact on build: NO - This change only modifies source code within the existing networking module and does not affect the build process").
  • Missing Testing Detail: "qemu:local" is not enough. What OS and compiler was qemu running on? What NuttX configuration was tested? Be specific about the target architecture and board configuration.
  • Absolutely Missing Test Logs: Providing "before" and "after" logs is essential. Show evidence that the change fixes a problem or implements a new feature. Without logs, there's no way to verify the PR's claims.

Example of an Improved Summary:

"Fixes a potential crash in si_getsockopt when invalid socket options are passed. Previously, parameters were not checked at the top level, leading to possible memory corruption or other undefined behavior when attempting to assign invalid values. This change adds parameter validation at the beginning of the function to catch these errors before any assignments occur."

Example of Improved Testing:

"Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
Target: qemu-system-arm, sim:nsh

Testing logs before change:

nsh> some_command_that_triggers_the_bug
Segmentation fault

Testing logs after change:

nsh> some_command_that_triggers_the_bug
Invalid socket option
```"


By providing this level of detail, reviewers can quickly understand the purpose, scope, and impact of your changes and verify that they work as intended.  This makes the review process much smoother and increases the likelihood of your PR being accepted.

@@ -271,6 +264,13 @@ int psock_getsockopt(FAR struct socket *psock, int level, int option,
{
int ret = -ENOPROTOOPT;

/* Verify that the socket option if valid (but might not be supported ) */

if (!value || !value_len)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!value || !value_len)
if (value == NULL || value_len == 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the title to
net/socket/getsockopt: move the options check to the upper layer

@zhhyu7 zhhyu7 changed the title getsockopt: move the options check to the upper layer net/socket/getsockopt: move the options check to the upper layer Feb 25, 2025
@zhhyu7
Copy link
Contributor Author

zhhyu7 commented Feb 25, 2025

please update the title to net/socket/getsockopt: move the options check to the upper layer

done.

check parameters at the top level to avoid errors when assigning
values in si_getsockopt.

Signed-off-by: zhanghongyu <[email protected]>
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.

6 participants