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

nshlib/nsh_parse: Fix variable arguments concat error of nsh_execute() #2850

Conversation

JianyuWang0623
Copy link
Contributor

Summary

Fix variable arguments concat error of nsh_execute()

Without this patch

nsh> set time 5
nsh> echo $time
5
nsh> sleep $time &
sh [5:100]
nsh> nsh: sleep: missing required argument(s)

With this patch

  nsh> set time 5
  nsh> echo $time
  5
  nsh> sleep $time &
  sh [4:100]
  nsh> ps
    PID GROUP PRI POLICY   TYPE    NPX STATE    EVENT     SIGMASK            STACK COMMAND
      0     0   0 FIFO     Kthread   - Ready              0000000000000000 0069616 Idle_Task
      1     0 224 FIFO     Kthread   - Waiting  Signal    0000000000000000 0067536 loop_task
      2     0 224 FIFO     Kthread   - Waiting  Semaphore 0000000000000000 0067504 hpwork 0x501760e0 0x50176128
      3     3 100 FIFO     Task      - Running            0000000000000000 0067536 nsh_main
      4     4 100 FIFO     Task      - Waiting  Signal    0000000000000000 0067520 sh -c sleep

Reported by yangao1@xiaomi.com, owned by @jasonbu, updated by @JianyuWang0623

Impact

nshlib/nsh_parse:nsh_execute()

Testing

  1. Selftest as above/commit message
  2. NuttX CI

@nuttxpr
Copy link

nuttxpr commented Nov 13, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be more explicit.

Here's a breakdown of why and some suggestions for improvement:

Strengths:

  • Clear Summary: The summary clearly explains the problem, the fix, and provides helpful before/after examples demonstrating the change's impact. Including the reporter and owner is also good practice.
  • Testing Provided: The PR includes testing logs (albeit simplified) and mentions NuttX CI, demonstrating effort to validate the changes.
  • Impact Partially Addressed: The impact on nshlib/nsh_parse:nsh_execute() is mentioned, indicating the specific code area affected.

Areas for Improvement:

  • Expand Impact Section: While the affected code is mentioned, the PR should explicitly answer the impact questions (user, build, hardware, documentation, security, compatibility) with "YES" or "NO" and provide explanations where necessary. Even if the answer is "NO", stating it explicitly is important for clarity. For example:
    • Impact on user: YES (Users will now be able to use variable substitution correctly within backgrounded commands in NSH.)
    • Impact on build: NO
    • Impact on hardware: NO
    • Impact on documentation: Potentially YES - if this bug wasn't previously documented, consider adding a note about the corrected behavior.
    • Impact on security: NO (Unless the bug could have been exploited, which seems unlikely in this case.)
    • Impact on compatibility: Potentially YES - If the prior behavior was relied upon by some scripts, this fix could break them. Consider this carefully.
  • More Detailed Testing Information: While the examples are good, providing more context about the testing environment would be beneficial. Follow the template's prompts:
    • Build Host(s): Specify the OS, CPU architecture, and compiler used.
    • Target(s): Specify the architecture (e.g., sim, RISC-V, ARM) and the board:config tested.
  • Complete Testing Logs: While short examples are okay for a quick overview, including more complete logs (or a link to them) is recommended. This helps reviewers understand the testing process and reproduce the issue if needed.

By addressing these points, the PR will be even stronger and easier for reviewers to assess.

Without this patch

  nsh> set time 5
  nsh> echo $time
  5
  nsh> sleep $time &
  sh [5:100]
  nsh> nsh: sleep: missing required argument(s)

With this patch

  nsh> set time 5
  nsh> echo $time
  5
  nsh> sleep $time &
  sh [4:100]
  nsh> ps
    PID GROUP PRI POLICY   TYPE    NPX STATE    EVENT     SIGMASK            STACK COMMAND
      0     0   0 FIFO     Kthread   - Ready              0000000000000000 0069616 Idle_Task
      1     0 224 FIFO     Kthread   - Waiting  Signal    0000000000000000 0067536 loop_task
      2     0 224 FIFO     Kthread   - Waiting  Semaphore 0000000000000000 0067504 hpwork 0x501760e0 0x50176128
      3     3 100 FIFO     Task      - Running            0000000000000000 0067536 nsh_main
      4     4 100 FIFO     Task      - Waiting  Signal    0000000000000000 0067520 sh -c sleep

Signed-off-by: buxiasen <buxiasen@xiaomi.com>
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_jasonbu_nsh_execute_arg_cat_241113 branch from 9b09e15 to 0d27789 Compare November 14, 2024 04:21
@xiaoxiang781216 xiaoxiang781216 merged commit 3a6ecb8 into apache:master Nov 14, 2024
25 checks passed
@JianyuWang0623
Copy link
Contributor Author

JianyuWang0623 commented Nov 19, 2024

As @jasonbu suggested, the defconfig who`s nsh_linelen is large (e.g. greater than 256 bytes) should be careful with stacksize
CC: @lipengfei28

  • Command
find boards/ -name defconfig | while read i; do line="$(grep CONFIG_NSH_LINELEN $i)"; if [[ "$line" != "" ]]; then line=$(echo $line | cut -d'=' -f2-); if [[ $line -gt 256 ]]; then echo $i: CONFIG_NSH_LINELEN:$line; fi; fi; done
  • Search result
boards/xtensa/esp32/esp32-devkitc/configs/wifi_smp/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/wifinsh/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/efuse/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/mcuboot_update_agent/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/blewifi/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/nxlooper/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/audio/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/wifi/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-devkitc/configs/wifishare/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-lyrat/configs/rtptools/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-lyrat/configs/nxrecorder/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-lyrat/configs/audio/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-lyrat/configs/wifi/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-audio-kit/configs/audio/defconfig: CONFIG_NSH_LINELEN:300
boards/xtensa/esp32/esp32-audio-kit/configs/wifi/defconfig: CONFIG_NSH_LINELEN:300
boards/arm/tiva/lm3s6965-ek/configs/qemu-flat/defconfig: CONFIG_NSH_LINELEN:1000
boards/arm/tiva/lm3s6965-ek/configs/qemu-protected/defconfig: CONFIG_NSH_LINELEN:1000

Mark apache/nuttx#2845.

JianyuWang0623 added a commit to JianyuWang0623/nuttx that referenced this pull request Nov 20, 2024
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
acassis pushed a commit to apache/nuttx that referenced this pull request Nov 21, 2024
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
JaeheeKwon pushed a commit to JaeheeKwon/nuttx that referenced this pull request Nov 28, 2024
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
JianyuWang0623 added a commit to JianyuWang0623/openvela-nuttx that referenced this pull request Jan 16, 2025
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
xiaoxiang781216 pushed a commit to open-vela/nuttx that referenced this pull request Jan 17, 2025
The large max command line length may cause stack overflow.

Test
  ./tools/configure.sh lm3s6965-ek:qemu-flat
  make -j16
  qemu-system-arm -semihosting \
  		-M lm3s6965evb \
  		-device loader,file=nuttx.bin,addr=0x00000000 \
  		-netdev user,id=user0 \
  		-serial mon:stdio -nographic

Link: apache/nuttx-apps#2850
Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
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.

4 participants