Skip to content

Commit

Permalink
nshlib/nsh_parse: use sh -c replace pthread detach when nsh background.
Browse files Browse the repository at this point in the history 
pthread & detach will still quit when parent task exit,
cause nsh_parse clone args leak. nsh should use task instead of thread

this case can reproduce the memory leak.
int main(int argc, FAR char *argv[])
{
  printf("Hello, World!!\n");
  system("sleep 1 &");
  return 0;
}

Signed-off-by: buxiasen <buxiasen@xiaomi.com>
  • Loading branch information
@jasonbu @xiaoxiang781216
jasonbu authored and xiaoxiang781216 committed Oct 21, 2024
1 parent ec458f0 commit 790be6d
Showing 1 changed file with 52 additions and 257 deletions.
309 changes: 52 additions & 257 deletions nshlib/nsh_parse.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include <string.h>
#include <errno.h>
#include <debug.h>
#include <pthread.h>
#include <sched.h>
#include <unistd.h>

Expand Down Expand Up @@ -126,19 +125,6 @@
* Private Types
****************************************************************************/

/* These structure describes the parsed command line */

#ifndef CONFIG_NSH_DISABLEBG
struct cmdarg_s
{
FAR struct nsh_vtbl_s *vtbl; /* For front-end interaction */
int fd_in; /* FD for output redirection */
int fd_out; /* FD for output redirection */
int argc; /* Number of arguments in argv */
FAR char *argv[MAX_ARGV_ENTRIES]; /* Argument list */
};
#endif

/* This structure describes the allocation list */

#ifdef HAVE_MEMLIST
Expand Down Expand Up @@ -174,13 +160,6 @@ static void nsh_alist_free(FAR struct nsh_vtbl_s *vtbl,
FAR struct nsh_alist_s *alist);
#endif

#ifndef CONFIG_NSH_DISABLEBG
static void nsh_releaseargs(struct cmdarg_s *arg);
static pthread_addr_t nsh_child(pthread_addr_t arg);
static struct cmdarg_s *nsh_cloneargs(FAR struct nsh_vtbl_s *vtbl,
int fd_in, int fd_out, int argc, FAR char *argv[]);
#endif

static int nsh_saveresult(FAR struct nsh_vtbl_s *vtbl, bool result);
static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,
int argc, FAR char *argv[], FAR const char *redirfile_in,
Expand Down Expand Up @@ -443,107 +422,6 @@ static void nsh_alist_free(FAR struct nsh_vtbl_s *vtbl,
}
#endif

/****************************************************************************
* Name: nsh_releaseargs
****************************************************************************/

#ifndef CONFIG_NSH_DISABLEBG
static void nsh_releaseargs(struct cmdarg_s *arg)
{
FAR struct nsh_vtbl_s *vtbl = arg->vtbl;
int i;

/* If the output was redirected, then file descriptor should
* be closed. The created task has its one, independent copy of
* the file descriptor
*/

if (vtbl->np.np_redir_out)
{
close(arg->fd_out);
}

/* Same for the input */

if (vtbl->np.np_redir_in)
{
close(arg->fd_in);
}

/* Released the cloned vtbl instance */

nsh_release(vtbl);

/* Release the cloned args */

for (i = 0; i < arg->argc; i++)
{
free(arg->argv[i]);
}

free(arg);
}
#endif

/****************************************************************************
* Name: nsh_child
****************************************************************************/

#ifndef CONFIG_NSH_DISABLEBG
static pthread_addr_t nsh_child(pthread_addr_t arg)
{
struct cmdarg_s *carg = (struct cmdarg_s *)arg;
int ret;

_info("BG %s\n", carg->argv[0]);

/* Execute the specified command on the child thread */

ret = nsh_command(carg->vtbl, carg->argc, carg->argv);

/* Released the cloned arguments */

_info("BG %s complete\n", carg->argv[0]);
nsh_releaseargs(carg);

/* Detach from the pthread since we are not going to join with it.
* Otherwise, we would have a memory leak.
*/

pthread_detach(pthread_self());
return (pthread_addr_t)((uintptr_t)ret);
}
#endif

/****************************************************************************
* Name: nsh_cloneargs
****************************************************************************/

#ifndef CONFIG_NSH_DISABLEBG
static struct cmdarg_s *nsh_cloneargs(FAR struct nsh_vtbl_s *vtbl,
int fd_in, int fd_out, int argc,
FAR char *argv[])
{
struct cmdarg_s *ret = (struct cmdarg_s *)zalloc(sizeof(struct cmdarg_s));
int i;

if (ret)
{
ret->vtbl = vtbl;
ret->fd_in = fd_in;
ret->fd_out = fd_out;
ret->argc = argc;

for (i = 0; i < argc; i++)
{
ret->argv[i] = strdup(argv[i]);
}
}

return ret;
}
#endif

/****************************************************************************
* Name: nsh_saveresult
****************************************************************************/
Expand Down Expand Up @@ -616,8 +494,6 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,
FAR const char *redirfile_in,
FAR const char *redirfile_out, int oflags)
{
int fd_in = STDIN_FILENO;
int fd_out = STDOUT_FILENO;
int ret;

/* DO NOT CHANGE THE ORDERING OF THE FOLLOWING STEPS
Expand Down Expand Up @@ -713,42 +589,6 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,

#endif

/* Redirected output? */

if (vtbl->np.np_redir_out)
{
/* Open the redirection file. This file will eventually
* be closed by a call to either nsh_release (if the command
* is executed in the background) or by nsh_undirect if the
* command is executed in the foreground.
*/

fd_out = open(redirfile_out, oflags, 0666);
if (fd_out < 0)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "open", NSH_ERRNO);
goto errout;
}
}

/* Redirected input? */

if (vtbl->np.np_redir_in)
{
/* Open the redirection file. This file will eventually
* be closed by a call to either nsh_release (if the command
* is executed in the background) or by nsh_undirect if the
* command is executed in the foreground.
*/

fd_in = open(redirfile_in, O_RDONLY, 0);
if (fd_in < 0)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "open", NSH_ERRNO);
goto errout;
}
}

/* Handle the case where the command is executed in background.
* However is app is to be started as built-in new process will
* be created anyway, so skip this step.
Expand All @@ -757,108 +597,79 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,
#ifndef CONFIG_NSH_DISABLEBG
if (vtbl->np.np_bg)
{
struct sched_param param;
struct nsh_vtbl_s *bkgvtbl;
struct cmdarg_s *args;
pthread_attr_t attr;
pthread_t thread;

/* Get a cloned copy of the vtbl with reference count=1.
* after the command has been processed, the nsh_release() call
* at the end of nsh_child() will destroy the clone.
*/
FAR char *sh_argv[4];
FAR char *sh_cmd = "sh";
int i;

bkgvtbl = nsh_clone(vtbl);
if (!bkgvtbl)
{
goto errout_with_redirect;
}
DEBUGASSERT(strncmp(argv[0], sh_cmd, 3) != 0);

/* Create a container for the command arguments */

args = nsh_cloneargs(bkgvtbl, fd_in, fd_out, argc, argv);
if (!args)
sh_argv[0] = sh_cmd;
sh_argv[1] = "-c";
for (i = 0; i < argc - 1; i++)
{
nsh_release(bkgvtbl);
goto errout_with_redirect;
}
FAR char *p_arg = argv[i];
size_t len = strlen(p_arg);

/* Handle redirection of output via a file descriptor */
/* Restore from split args to concat args. */

if (vtbl->np.np_redir_out || vtbl->np.np_redir_in)
{
nsh_redirect(bkgvtbl, fd_in, fd_out, NULL);
DEBUGASSERT(&p_arg[len + 1] == argv[i + 1]);
p_arg[len] = ' ';
}

/* Get the execution priority of this task */
sh_argv[2] = argv[0];
sh_argv[3] = NULL;

ret = sched_getparam(0, &param);
if (ret != 0)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "sched_getparm",
NSH_ERRNO);
/* np.np_bg still there, try use nsh_builtin or nsh_fileapp to
* dispatch the backgroud by sh -c ""
*/

/* NOTE: bkgvtbl is released in nsh_relaseargs() */
return nsh_execute(vtbl, 4, sh_argv,
redirfile_in, redirfile_out, oflags);
}
else
#endif
{
uint8_t save[SAVE_SIZE];

nsh_releaseargs(args);
goto errout;
}
int fd_in = STDIN_FILENO;
int fd_out = STDOUT_FILENO;

/* Determine the priority to execute the command */
/* Redirected output? */

if (vtbl->np.np_nice != 0)
if (vtbl->np.np_redir_out)
{
int priority = param.sched_priority - vtbl->np.np_nice;
if (vtbl->np.np_nice < 0)
{
int max_priority = sched_get_priority_max(SCHED_NSH);
if (priority > max_priority)
{
priority = max_priority;
}
}
else
/* Open the redirection file. This file will eventually
* be closed by a call to either nsh_release (if the command
* is executed in the background) or by nsh_undirect if the
* command is executed in the foreground.
*/

fd_out = open(redirfile_out, oflags, 0666);
if (fd_out < 0)
{
int min_priority = sched_get_priority_min(SCHED_NSH);
if (priority < min_priority)
{
priority = min_priority;
}
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "open", NSH_ERRNO);
return nsh_saveresult(vtbl, true);
}

param.sched_priority = priority;
}

/* Set up the thread attributes */
/* Redirected input? */

pthread_attr_init(&attr);
pthread_attr_setschedpolicy(&attr, SCHED_NSH);
pthread_attr_setschedparam(&attr, &param);

/* Execute the command as a separate thread at the appropriate
* priority.
*/

ret = pthread_create(&thread, &attr, nsh_child, (pthread_addr_t)args);
if (ret != 0)
if (vtbl->np.np_redir_in)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "pthread_create",
NSH_ERRNO_OF(ret));

/* NOTE: bkgvtbl is released in nsh_relaseargs() */
/* Open the redirection file. This file will eventually
* be closed by a call to either nsh_release (if the command
* is executed in the background) or by nsh_undirect if the
* command is executed in the foreground.
*/

nsh_releaseargs(args);
goto errout;
fd_in = open(redirfile_in, O_RDONLY, 0);
if (fd_in < 0)
{
nsh_error(vtbl, g_fmtcmdfailed, argv[0], "open", NSH_ERRNO);
return nsh_saveresult(vtbl, true);
}
}

nsh_output(vtbl, "%s [%d:%d]\n", argv[0], thread,
param.sched_priority);
}
else
#endif
{
uint8_t save[SAVE_SIZE];

/* Handle redirection of stdin/stdout file descriptor */

if (vtbl->np.np_redir_out || vtbl->np.np_redir_in)
Expand Down Expand Up @@ -890,7 +701,7 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,

if (ret < 0)
{
goto errout;
return nsh_saveresult(vtbl, true);
}
}

Expand All @@ -899,22 +710,6 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl,
*/

return nsh_saveresult(vtbl, false);

#ifndef CONFIG_NSH_DISABLEBG
errout_with_redirect:
if (vtbl->np.np_redir_out)
{
close(fd_out);
}

if (vtbl->np.np_redir_in)
{
close(fd_in);
}
#endif

errout:
return nsh_saveresult(vtbl, true);
}

/****************************************************************************
Expand Down

0 comments on commit 790be6d

Please sign in to comment.