Lock the scheduler when starting NSH builtin applications to eliminate race conditions

git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@4988 42af7a65-404d-4744-a932-0658087f49c3

ChangeLog.txt

@@ -260,4 +260,6 @@
 	  termios ioctls, then don't bother trying to configure the baud, parity
 	  etc.
 	* apps/nslib:  If waitpid() is supported, then NSH not catches the
-	  return value from spawned applications.
+	  return value from spawned applications (provided by Mike Smith)
+	* apps/nslib:  Lock the schedule while starting built-in applications
+	  in order to eliminate race conditions (also from Mike Smith).

nshlib/nsh_apps.c

@@ -90,7 +90,9 @@
  *    Attempt to execute the application task whose name is 'cmd'
  *
  * Returned Value:
- *   -1 (ERRROR) if the application task corresponding to 'cmd' could not
+ *   <0          If exec_namedapp() fails, then the negated errno value
+ *               is returned.
+ *   -1 (ERROR)  if the application task corresponding to 'cmd' could not
  *               be started (possibly because it doesn not exist).
  *    0 (OK)     if the application task corresponding to 'cmd' was
  *               and successfully started.  If CONFIG_SCHED_WAITPID is
@@ -107,47 +109,78 @@ int nsh_execapp(FAR struct nsh_vtbl_s *vtbl, FAR const char *cmd,
 {
   int ret = OK;
 
-  /* Try to find command within pre-built application list. */
+  /* Lock the scheduler to prevent the application from running until the
+   * waitpid() has been called.
+   */
+
+  sched_lock();
+
+  /* Try to find and execute the command within the list of builtin
+   * applications.
+   */
 
   ret = exec_namedapp(cmd, (FAR const char **)argv);
+  if (ret >= 0)
+    {
+      /* The application was successfully started (but still blocked because the
+       * scheduler is locked).  If the application was not backgrounded, then we
+       * need to wait here for the application to exit.
+       */
+
+#ifdef CONFIG_SCHED_WAITPID
+      if (vtbl->np.np_bg == false)
+        {
+          int rc = 0;
+
+          /* Wait for the application to exit.  Since we have locked the
+           * scheduler above, we know that the application has not yet
+           * started and there is no possibility that it has already exited.
+           * The scheduler will be unlocked while waitpid is waiting and the
+           * application will be able to run.
+           */
+
+          ret = waitpid(ret, &rc, 0);
+          if (ret >= 0)
+            {
+              /* We can't return the exact status (nsh has nowhere to put it)
+               * so just pass back zero/nonzero in a fashion that doesn't look 
+               * like an error.
+               */
+
+              ret = (rc == 0) ? OK : 1;
+
+             /* TODO:  Set the environment variable '?' to a string corresponding
+              * to WEXITSTATUS(rc) so that $? will expand to the exit status of
+              * the most recently executed task.
+              */
+            }
+        }
+      else
+#endif
+        {
+          struct sched_param param;
+          sched_getparam(0, &param);
+          nsh_output(vtbl, "%s [%d:%d]\n", cmd, ret, param.sched_priority);
+
+          /* Backgrounded commands always 'succeed' as long as we can start
+           * them.
+           */
+
+          ret = OK;
+        }
+    }
+
+  sched_unlock();
+
+  /* If exec_namedapp() or waitpid() failed, then return the negated errno
+   * value.
+   */
+
   if (ret < 0)
     {
       return -errno;
     }
 
-#ifdef CONFIG_SCHED_WAITPID
-  if (vtbl->np.np_bg == false)
-    {
-      int rc = 0;
-
-      waitpid(ret, &rc, 0);
-
-      /* We can't return the exact status (nsh has nowhere to put it)
-       * so just pass back zero/nonzero in a fashion that doesn't look 
-       * like an error.
-       */
-
-      ret = (rc == 0) ? OK : 1;
-
-      /* TODO:  Set the environment variable '?' to a string corresponding
-       * to WEXITSTATUS(rc) so that $? will expand to the exit status of
-       * the most recently executed task.
-       */
-    }
-  else
-#endif
-    {
-      struct sched_param param;
-      sched_getparam(0, &param);
-      nsh_output(vtbl, "%s [%d:%d]\n", cmd, ret, param.sched_priority);
-
-      /* Backgrounded commands always 'succeed' as long as we can start
-       * them.
-       */
-
-      ret = OK;
-    }
-
   return ret;
 }
 

nshlib/nsh_parse.c

@@ -492,6 +492,8 @@ static int cmd_exit(FAR struct nsh_vtbl_s *vtbl, int argc, char **argv)
  *   Exectue the command in argv[0]
  *
  * Returned Value:
+ *   <0          If exec_namedapp() fails, then the negated errno value
+ *               is returned.
  *   -1 (ERRROR) if the command was unsuccessful
  *    0 (OK)     if the command was successful
  *    1          if an application task was spawned successfully, but
@@ -521,8 +523,8 @@ static int nsh_execute(FAR struct nsh_vtbl_s *vtbl, int argc, char *argv[])
 #ifdef CONFIG_NSH_BUILTIN_APPS
    ret = nsh_execapp(vtbl, cmd, argv);
 
-   /* The pre-built application was successfully started -- return OK 
+   /* If the built-in application was successfully started, return OK 
-    * or 1 if it returned a non-zero exit status.
+    * or 1 (if the application returned a non-zero exit status).
     */
 
    if (ret >= 0)