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

Reduce nsh stack consumption #2954

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Reduce nsh stack consumption #2954

merged 2 commits into from
Jan 20, 2025

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Jan 19, 2025

Summary

  • nshlib: Remove the unnecessary temp path buffer
  • nshlib: Replace the big temp buffer with lib_get_[path|temp]buffer

Impact

  • stack consumption decrease.
  • nshlib: nsh_fscmds, nsh_fsutils, nsh_timcmds.

Testing

sim:nsh

@nuttxpr
Copy link

nuttxpr commented Jan 19, 2025

[Experimental Bot, please feedback here]

No, this PR summary does not fully meet the NuttX requirements. While it addresses what is changed, it lacks crucial details about why and how. Here's a breakdown:

Missing from Summary:

  • Why: The summary states what was removed/replaced (temp path buffer, big temp buffer), but not why this was necessary. Was it causing a bug? Improving performance? Reducing memory usage? This is critical for reviewers to understand the motivation behind the change.
  • How: While it mentions the replacement functions (lib_get_[path|temp]buffer), it doesn't explain how these functions address the issue. Do they allocate memory differently? Are they more efficient? What's the difference between the old and new approach?

Missing from Impact:

  • Specificity: "minor" is not sufficient. While the impact might be minor, you still need to explicitly answer each question with YES/NO and provide descriptions where necessary. For example:
    • Impact on user: NO
    • Impact on build: NO
    • Impact on hardware: NO
    • Impact on documentation: Possibly YES if the API changes require documentation updates.
    • Impact on security: Possibly YES if the previous buffer handling had security implications (e.g., buffer overflows). Explain why or why not.
    • Impact on compatibility: NO (presumably, but state this explicitly)

Missing from Testing:

  • Insufficient Detail: sim:nsh is not enough information. Specify:
    • Build Host: e.g., Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
    • Target: e.g., sim:nsh is okay, but more context might be helpful. Which simulator? QEMU?
  • Missing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual logs demonstrating the issue before the change and the improved behavior after the change. Even if the change is small, show something. For example, commands you ran in NSH and their output.

Example of an improved summary:

## Summary

This PR addresses potential buffer overflow issues and improves memory management in nshlib by removing a large, statically allocated temporary path buffer. This buffer was previously used for various path manipulations and presented a risk of overflow if unexpectedly long paths were encountered. The change replaces the static buffer with calls to `lib_get_pathbuffer()` and `lib_get_tempbuffer()`. These functions dynamically allocate buffer space as needed, eliminating the fixed size limitation and the associated overflow risk.  This also improves memory efficiency by only allocating the required space.

Relates to [NuttX Issue #XXX](If applicable)

## Impact

* Is new feature added? NO
* Is existing feature changed? YES (Improved memory management in nshlib path handling)
* Impact on user: NO
* Impact on build: NO
* Impact on hardware: NO
* Impact on documentation: NO (If no API changes) / YES (If API changes, explain what needs updating)
* Impact on security: YES (The previous static buffer posed a potential buffer overflow risk. This change eliminates that risk).
* Impact on compatibility: NO
* Anything else to consider?  This change improves the robustness and security of path handling within nshlib.


## Testing

Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
Target: QEMU sim:nsh

Testing logs before change:

nsh> pwd // Demonstrating some path-related command and its output before the change
/
nsh> mkdir -p /very/long/path/that/might/cause/issues
... (Output showing the error or unexpected behavior, if any)


Testing logs after change:

nsh> pwd // The same command after the change
/
nsh> mkdir -p /very/long/path/that/might/cause/issues
... (Output showing the correct behavior after the change)

to save the stack consumption

Signed-off-by: Xiang Xiao <[email protected]>
Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @xiaoxiang781216 :-)

@@ -595,8 +595,15 @@ int cmd_watch(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)

for (i = 0; i < count; i++)
{
FAR char *buffer = lib_get_tempbuffer(LINE_MAX);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not get at line 595 & put at line 615 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because line 613 call sleep, which may sleep for a long time.

Copy link
Contributor

@GUIDINGLI GUIDINGLI left a comment

Choose a reason for hiding this comment

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

LGTM

@GUIDINGLI GUIDINGLI merged commit 5330966 into apache:master Jan 20, 2025
37 checks passed
@xiaoxiang781216 xiaoxiang781216 deleted the fs_ branch January 20, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants