apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX by anchao · Pull Request #2918 · apache/nuttx-apps

anchao · 2024-12-27T06:23:07Z

Summary

apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX

Applications should not depend on any properties of nshlib

Signed-off-by: chao an anchao@lixiang.com

nshlib/nshline: move all temp line buffer to heap

Signed-off-by: chao an anchao@lixiang.com

nshlib/traceline: move traceline buffer from stack to heap

Signed-off-by: chao an anchao@lixiang.com

Impact

N/A

Testing

sim/nsh ostest

Signed-off-by: chao an <anchao@lixiang.com>

Applications should not depend on any properties of nshlib Signed-off-by: chao an <anchao@lixiang.com>

nuttxpr · 2024-12-27T06:32:31Z

[Experimental Bot, please feedback here]

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)

xiaoxiang781216 · 2024-12-29T06:13:13Z

+#if (!defined(CONFIG_NSH_DISABLE_MEMDUMP) && defined(NSH_HAVE_WRITEFILE)) || \
+    !defined(CONFIG_NSH_DISABLEBG) || defined(CONFIG_NSH_PIPELINE) || \
+    !defined(CONFIG_NSH_DISABLE_WATCH)
+  char templine[CONFIG_NSH_LINELEN];


but this will increase the heap consumption for the temp use case.

Compared with irregular heap access, shared template and will make it more deterministic

could you split the rename from moving buffer to nsh_vtbl?

the temp buffer is only used in few command, why move it to nsh_vtbl to extend the life to the whole session?

xiaoxiang781216 · 2024-12-31T08:21:42Z

+  /* Trace line buffer */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+  char traceline[CONFIG_NSH_LINELEN];


but traceline is used only when launching program, isn't good to keep the buffer in the whole nsh life cycle

xiaoxiang781216 · 2024-12-31T08:22:41Z

+#if (!defined(CONFIG_NSH_DISABLE_MEMDUMP) && defined(NSH_HAVE_WRITEFILE)) || \
+    !defined(CONFIG_NSH_DISABLEBG) || defined(CONFIG_NSH_PIPELINE) || \
+    !defined(CONFIG_NSH_DISABLE_WATCH)
+  char templine[CONFIG_NSH_LINELEN];


the temp buffer is only used in few command, why move it to nsh_vtbl to extend the life to the whole session?

xiaoxiang781216 · 2024-12-31T08:23:05Z

@@ -380,7 +380,7 @@
 /* Maximum size of one command line (telnet or serial) */

 #ifndef CONFIG_NSH_LINELEN


let's remove CONFIG_NSH_LINELEN directly

cederom

Thank you @anchao :-) My questions:

Where LINE_MAX should be used and where CONFIG_NSH_LINELEN?
Where is LINE_MAX defined?
Wouldn't that create inconsistency?
This seems to require a documentation update? Or at least mention on the change and what/where is preferred in regard to above two questions?

xiaoxiang781216 · 2025-01-03T01:58:32Z

Thank you @anchao :-) My questions:

Where LINE_MAX should be used and where CONFIG_NSH_LINELEN?

Yes, the patch plan to replace all XXX_LINELEN to LINE_MAX.

Where is LINE_MAX defined?

here: apache/nuttx#15344
LINE_MAX is POSIX defined macro.

Wouldn't that create inconsistency?

no, it improves the consistency, since all places will use the standard macro defined by POSIX.

This seems to require a documentation update? Or at least mention on the change and what/where is preferred in regard to above two questions?

patacongo · 2025-01-03T02:10:33Z

3. Where is LINE_MAX defined?

here: apache/nuttx#15344 LINE_MAX is POSIX defined macro.

Specifically, limits.h: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Minimum value is _POSIX2_LINE_MAX

cederom · 2025-01-03T02:13:43Z

Where is LINE_MAX defined?

here: apache/nuttx#15344 LINE_MAX is POSIX defined macro.

Specifically, limits.h: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html

Minimum value is _POSIX2_LINE_MAX

Yup, and the latest one https://pubs.opengroup.org/onlinepubs/9799919799/ :D

Thank you @patacongo and @xiaoxiang781216 :-)

cederom

Thank you @anchao :-)

Please follow all @xiaoxiang781216 remarks, thank you @xiaoxiang781216 :-)
Please update commit message apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX so it includes information that this change follows POSIX Specification https://pubs.opengroup.org/onlinepubs/9799919799/.

xiaoxiang781216 · 2025-01-16T14:50:04Z

@anchao do we still need this patch?

anchao · 2025-01-16T15:08:40Z

@anchao do we still need this patch?

not anymore, let me abandon

anchao added 4 commits December 27, 2024 13:46

nshlib/cn_line: replace CONFIG_NSH_LINELEN to sizeof

73220c3

Signed-off-by: chao an <anchao@lixiang.com>

nshlib/traceline: move traceline buffer from stack to heap

bff158b

Signed-off-by: chao an <anchao@lixiang.com>

nshlib/nshline: move all temp line buffer to heap

18538c4

Signed-off-by: chao an <anchao@lixiang.com>

apps/system: replace CONFIG_NSH_LINELEN to LINE_MAX

ae0325a

Applications should not depend on any properties of nshlib Signed-off-by: chao an <anchao@lixiang.com>

github-actions Bot added Area: Networking Area: System Area: NSH Area: Examples Size: M labels Dec 27, 2024

anchao mentioned this pull request Dec 27, 2024

rename lib_pathbuffer to lib_tempbuffer apache/nuttx#15326

Merged

xiaoxiang781216 reviewed Dec 29, 2024

View reviewed changes

acassis approved these changes Dec 31, 2024

View reviewed changes

xiaoxiang781216 reviewed Dec 31, 2024

View reviewed changes

cederom reviewed Jan 2, 2025

View reviewed changes

cederom requested a review from patacongo January 2, 2025 20:44

cederom approved these changes Jan 3, 2025

View reviewed changes

xiaoxiang781216 mentioned this pull request Jan 12, 2025

goldfish-armeabi-v7a-ap-citest: Update INIT_STACKSIZE to 6144 open-vela/vendor_openvela-archive#7

Merged

This was referenced Jan 14, 2025

apps/nshlib: Using lib_get_tempbuffer() to save stack space #2943

Merged

apps/nshlib: Using lib_get_tempbuffer() to save stack space open-vela/nuttx-apps#65

Merged

anchao closed this Jan 16, 2025

		@@ -380,7 +380,7 @@
		/* Maximum size of one command line (telnet or serial) */

		#ifndef CONFIG_NSH_LINELEN

Conversation

anchao commented Dec 27, 2024

Summary

Impact

Testing

Uh oh!

nuttxpr commented Dec 27, 2024

Uh oh!

xiaoxiang781216 Dec 29, 2024

Choose a reason for hiding this comment

Uh oh!

anchao Dec 29, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

cederom left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 commented Jan 3, 2025

Uh oh!

patacongo commented Jan 3, 2025

Uh oh!

cederom commented Jan 3, 2025

Uh oh!

cederom left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 commented Jan 16, 2025

Uh oh!

anchao commented Jan 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants