nuttxpr commented on PR #2918: URL: https://github.com/apache/nuttx-apps/pull/2918#issuecomment-2563373235
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary of changes, it lacks crucial details. Here's a breakdown of what's missing: * **Insufficient Summary:** The summary lists *what* was changed but not *why*. It needs to explain the rationale behind replacing `CONFIG_NSH_LINELEN` and moving buffers to the heap. What problem does this solve? What are the benefits? There's no mention of a related NuttX issue. * **Missing Impact Assessment:** Simply stating "N/A" for impact is insufficient. Even seemingly small changes can have unforeseen consequences. The PR needs to explicitly address each impact area (user, build, hardware, documentation, security, compatibility). For example, moving buffers to the heap could impact memory usage. This needs to be acknowledged and discussed. * **Inadequate Testing Information:** "sim/nsh ostest" is not enough. The PR needs to specify the *host* operating system, CPU architecture, and compiler used for testing. It also needs to specify the *target* architecture and board configuration used with the simulator. Most importantly, it's missing the actual *testing logs* before and after the change, which are crucial for demonstrating that the changes work as intended and don't introduce regressions. **Example of how to improve the PR:** **Summary:** This PR addresses potential buffer overflows and improves portability in nshlib and related applications. Applications were depending on the internal `CONFIG_NSH_LINELEN` setting from nshlib, which is not ideal for modularity. This PR replaces the use of `CONFIG_NSH_LINELEN` with the standard `LINE_MAX` define in applications. Additionally, temporary line buffers in `nshlib/nshline` and `nshlib/traceline` are moved from the stack to the heap. This prevents potential stack overflows when processing very long lines and improves the robustness of nshlib. Related NuttX Issue: (If one exists, link it here) **Impact:** * **Impact on user:** YES. Applications using `CONFIG_NSH_LINELEN` will need to be recompiled. This should be a straightforward change, as `LINE_MAX` is a standard define. * **Impact on build:** NO. * **Impact on hardware:** NO. * **Impact on documentation:** YES. Documentation should be updated to reflect the removal of the `CONFIG_NSH_LINELEN` dependency and the change to heap-allocated buffers. (Indicate if documentation changes are included in the PR). * **Impact on security:** YES. This change mitigates potential stack overflow vulnerabilities when processing long lines. * **Impact on compatibility:** Potentially backwards compatible with applications that don't use extremely long lines. However, applications directly relying on `CONFIG_NSH_LINELEN` will need modification. * **Anything else to consider:** The change to heap-allocated buffers might slightly increase heap memory usage. **Testing:** * **Build Host:** Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * **Target:** sim:qemu-x86_64:nsh **Testing logs before change:** ``` (Paste actual logs here showing the problem being addressed) ``` **Testing logs after change:** ``` (Paste actual logs here demonstrating the correct behavior after the change) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
