Skip to content

Reduce nsh stack consumption #2954

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Reduce nsh stack consumption #2954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d351399
nshlib: Remove the unnecessary temp path buffer
xiaoxiang781216 Jan 19, 2025
acd10c6
nshlib: Replace the big temp buffer with lib_get_[path|temp]buffer
xiaoxiang781216 Jan 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions nshlib/nsh_fscmds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2191,7 +2191,6 @@ int cmd_rm(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
bool recursive = false;
bool force = false;
FAR char *fullpath;
char buf[PATH_MAX];
struct stat stat;
int ret = ERROR;
int c;
Expand Down Expand Up @@ -2234,8 +2233,7 @@ int cmd_rm(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
{
if (recursive)
{
strlcpy(buf, fullpath, PATH_MAX);
Copy link
Contributor

@jlaitine jlaitine Nov 24, 2025

Choose a reason for hiding this comment

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

Hi @xiaoxiang781216, I believe this change was wrong;

In the fullpath is commonly allocated with strdup, e.g. in here:

nuttx-apps/nshlib/nsh_envcmds.c

Line 206 in 6028b42

return strdup(relpath);

And in unlink_recursive the file name is added to the fullpath in here:

nuttx-apps/nshlib/nsh_fscmds.c

Line 2170 in 6028b42

snprintf(&path[len], PATH_MAX - len, "/%s", d->d_name);

But, the size of the allocated buffer is actually just "len + 1" on the above line, since the buffer originates from strdup.

So the temporary buffer in here is not useless, it is absolutely needed.

I just found this out when I tried "rm -rf /fs/sdcard/folder", with file in in "folder". This corrupts heap and results in full system crash.

Copy link
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Nov 24, 2025

Choose a reason for hiding this comment

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

yes, you are right, we need call lib_get_pathbuffer.

Copy link
Contributor

@cederom cederom Nov 24, 2025

Choose a reason for hiding this comment

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

Ah I noticed that recently with simpler rm -rf / :-P Should we revert?

Copy link
Contributor

@jlaitine jlaitine Nov 25, 2025

Choose a reason for hiding this comment

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

Perhaps do what @xiaoxiang781216 proposed, instead of allocating the buffer from the stack, we could use lib_get_pathbuffer to keep stack usage smaller. I can make a patch for this if so wanted...

Copy link
Contributor

@jlaitine jlaitine Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I created #3222 for the issue. @xiaoxiang781216 , @cederom please have a look

Copy link
Contributor Author

@xiaoxiang781216 xiaoxiang781216 Nov 25, 2025

Choose a reason for hiding this comment

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

@jlaitine thanks to fix my fault.

Copy link
Contributor

@cederom cederom Nov 25, 2025

Choose a reason for hiding this comment

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

Thank you @jlaitine !! No worries @xiaoxiang781216 !! :-) <3

ret = unlink_recursive(buf, &stat);
ret = unlink_recursive(fullpath, &stat);
}
else
{
Expand Down
14 changes: 11 additions & 3 deletions nshlib/nsh_fsutils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ static int getpid_callback(FAR struct nsh_vtbl_s *vtbl,
FAR struct dirent *entryp, FAR void *pvarg)
{
FAR struct getpid_arg_s *arg = (FAR struct getpid_arg_s *)pvarg;
char buffer[PATH_MAX];
FAR char *buffer;
int fd;
int len;

Expand All @@ -82,20 +82,27 @@ static int getpid_callback(FAR struct nsh_vtbl_s *vtbl,
return -E2BIG;
}

/* Match the name of the process */
buffer = lib_get_pathbuffer();
if (buffer == NULL)
{
return -errno;
}

snprintf(buffer, sizeof(buffer), "%s/%s/cmdline", dirpath, entryp->d_name);
/* Match the name of the process */

snprintf(buffer, PATH_MAX, "%s/%s/cmdline", dirpath, entryp->d_name);
fd = open(buffer, O_RDONLY | O_CLOEXEC);
if (fd < 0)
{
lib_put_pathbuffer(buffer);
return 0;
}

len = read(fd, buffer, sizeof(buffer) - 1);
close(fd);
if (len < 0)
{
lib_put_pathbuffer(buffer);
return -errno;
}

Expand All @@ -107,6 +114,7 @@ static int getpid_callback(FAR struct nsh_vtbl_s *vtbl,
arg->pids[arg->next++] = atoi(entryp->d_name);
}

lib_put_pathbuffer(buffer);
return OK;
}
#endif
Expand Down
9 changes: 8 additions & 1 deletion nshlib/nsh_timcmds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <nuttx/config.h>

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
Expand Down Expand Up @@ -552,7 +553,6 @@ int cmd_timedatectl(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
#ifndef CONFIG_NSH_DISABLE_WATCH
int cmd_watch(FAR struct nsh_vtbl_s *vtbl, int argc, FAR char **argv)
{
char buffer[LINE_MAX];
int interval = 2;
int count = -1;
FAR char *cmd;
Expand Down Expand Up @@ -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

@GUIDINGLI GUIDINGLI Jan 20, 2025

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

@xiaoxiang781216 xiaoxiang781216 Jan 20, 2025

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.

if (buffer == NULL)
{
return ERROR;
}

strlcpy(buffer, cmd, LINE_MAX);
ret = nsh_parse(vtbl, buffer);
lib_put_tempbuffer(buffer);
if (ret < 0)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], cmd, NSH_ERRNO);
Expand Down
Loading