fs: enhance dup3() mulit-threads saftey

Signed-off-by: ligd <liguiding1@xiaomi.com>

fs/inode/fs_files.c

@@ -209,8 +209,6 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2,
                             int flags)
 {
   FAR struct filelist *list;
-  FAR struct file *filep;
-  FAR struct file  file;
   int count;
   int ret;
 
@@ -224,14 +222,12 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2,
   fd2 = fdcheck_restore(fd2);
 #endif
 
-  list = nxsched_get_files_from_tcb(tcb);
-
-  count = files_countlist(list);
-
   /* Get the file descriptor list.  It should not be NULL in this context. */
 
-  if (fd1 < 0 || fd1 >= count ||
+  list = nxsched_get_files_from_tcb(tcb);
-      fd2 < 0)
+  count = files_countlist(list);
+
+  if (fd1 < 0 || fd1 >= count || fd2 < 0)
     {
       return -EBADF;
     }
@@ -245,28 +241,18 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2,
         }
     }
 
-  filep = files_fget(list, fd2);
-  memcpy(&file, filep, sizeof(struct file));
-  memset(filep, 0,     sizeof(struct file));
-
   /* Perform the dup3 operation */
 
-  ret = file_dup3(files_fget(list, fd1), filep, flags);
+  ret = file_dup3(files_fget(list, fd1), files_fget(list, fd2), flags);
-
+  if (ret < 0)
-#ifdef CONFIG_FDSAN
+    {
-  filep->f_tag_fdsan = file.f_tag_fdsan;
+      return ret;
-#endif
+    }
 
 #ifdef CONFIG_FDCHECK
-  filep->f_tag_fdcheck = file.f_tag_fdcheck;
+  return fdcheck_protect(fd2);
-#endif
-
-  file_close(&file);
-
-#ifdef CONFIG_FDCHECK
-  return ret < 0 ? ret : fdcheck_protect(fd2);
 #else
-  return ret < 0 ? ret : fd2;
+  return fd2;
 #endif
 }
 

fs/vfs/fs_close.c

@@ -28,6 +28,7 @@
 #include <sched.h>
 #include <assert.h>
 #include <errno.h>
+#include <fcntl.h>
 
 #include <nuttx/fs/fs.h>
 
@@ -39,10 +40,11 @@
  ****************************************************************************/
 
 /****************************************************************************
- * Name: file_close
+ * Name: file_close_without_clear
  *
  * Description:
- *   Close a file that was previously opened with file_open().
+ *   Close a file that was previously opened with file_open(), but without
+ *   clear filep.
  *
  * Input Parameters:
  *   filep - A pointer to a user provided memory location containing the
@@ -54,7 +56,7 @@
  *
  ****************************************************************************/
 
-int file_close(FAR struct file *filep)
+int file_close_without_clear(FAR struct file *filep)
 {
   struct inode *inode;
   int ret = OK;
@@ -80,10 +82,45 @@ int file_close(FAR struct file *filep)
       /* And release the inode */
 
       inode_release(inode);
-
+    }
-      /* Reset the user file struct instance so that it cannot be reused. */
+
-
+  return ret;
-      memset(filep, 0, sizeof(*filep));
+}
+
+/****************************************************************************
+ * Name: file_close
+ *
+ * Description:
+ *   Close a file that was previously opened with file_open().
+ *
+ * Input Parameters:
+ *   filep - A pointer to a user provided memory location containing the
+ *           open file data returned by file_open().
+ *
+ * Returned Value:
+ *   Zero (OK) is returned on success; A negated errno value is returned on
+ *   any failure to indicate the nature of the failure.
+ *
+ ****************************************************************************/
+
+int file_close(FAR struct file *filep)
+{
+  int ret;
+
+  ret = file_close_without_clear(filep);
+  if (ret >= 0 && filep->f_inode)
+    {
+      /* Reset the user file struct instance so that it cannot be reused. */
+
+      filep->f_inode = NULL;
+
+#ifdef CONFIG_FDCHECK
+      filep->f_tag_fdcheck = 0;
+#endif
+
+#ifdef CONFIG_FDSAN
+      filep->f_tag_fdsan = 0;
+#endif
     }
 
   return ret;

fs/vfs/fs_dup2.c

@@ -58,7 +58,6 @@
 int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags)
 {
   FAR struct inode *inode;
-  struct file temp;
   int ret;
 
   if (filep1 == NULL || filep1->f_inode == NULL || filep2 == NULL)
@@ -85,23 +84,32 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags)
       return ret;
     }
 
-  /* Then clone the file structure */
+  /* If there is already an inode contained in the new file structure,
+   * close the file and release the inode.
+   * But we need keep the filep2->f_inode, incase of realloced by others.
+   */
 
-  memset(&temp, 0, sizeof(temp));
+  ret = file_close_without_clear(filep2);
+  if (ret < 0)
+    {
+      inode_release(inode);
+      return ret;
+    }
 
   /* The two filep don't share flags (the close-on-exec flag). */
 
   if (flags == O_CLOEXEC)
     {
-      temp.f_oflags = filep1->f_oflags | O_CLOEXEC;
+      filep2->f_oflags = filep1->f_oflags | O_CLOEXEC;
     }
   else
     {
-      temp.f_oflags = filep1->f_oflags & ~O_CLOEXEC;
+      filep2->f_oflags = filep1->f_oflags & ~O_CLOEXEC;
     }
 
-  temp.f_pos    = filep1->f_pos;
+  filep2->f_priv  = NULL;
-  temp.f_inode  = inode;
+  filep2->f_pos   = filep1->f_pos;
+  filep2->f_inode = inode;
 
   /* Call the open method on the file, driver, mountpoint so that it
    * can maintain the correct open counts.
@@ -116,7 +124,7 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags)
 
           if (inode->u.i_mops->dup)
             {
-              ret = inode->u.i_mops->dup(filep1, &temp);
+              ret = inode->u.i_mops->dup(filep1, filep2);
             }
         }
       else
@@ -124,25 +132,25 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags)
         {
           /* (Re-)open the pseudo file or device driver */
 
-          temp.f_priv = filep1->f_priv;
+          filep2->f_priv = filep1->f_priv;
 
           /* Add nonblock flags to avoid happening block when
            * calling open()
            */
 
-          temp.f_oflags |= O_NONBLOCK;
+          filep2->f_oflags |= O_NONBLOCK;
 
           if (inode->u.i_ops->open)
             {
-              ret = inode->u.i_ops->open(&temp);
+              ret = inode->u.i_ops->open(filep2);
             }
 
           if (ret >= 0 && (filep1->f_oflags & O_NONBLOCK) == 0)
             {
-              ret = file_ioctl(&temp, FIONBIO, 0);
+              ret = file_ioctl(filep2, FIONBIO, 0);
               if (ret < 0 && inode->u.i_ops->close)
                 {
-                  ret = inode->u.i_ops->close(&temp);
+                  ret = inode->u.i_ops->close(filep2);
                 }
             }
         }
@@ -156,26 +164,6 @@ int file_dup3(FAR struct file *filep1, FAR struct file *filep2, int flags)
         }
     }
 
-  /* If there is already an inode contained in the new file structure,
-   * close the file and release the inode.
-   */
-
-  ret = file_close(filep2);
-  DEBUGASSERT(ret == 0);
-
-  /* Copy tag */
-
-#ifdef CONFIG_FDSAN
-  temp.f_tag_fdsan = filep1->f_tag_fdsan;
-#endif
-
-#ifdef CONFIG_FDCHECK
-  temp.f_tag_fdcheck = filep1->f_tag_fdcheck;
-#endif
-
-  /* Return the file structure */
-
-  memcpy(filep2, &temp, sizeof(temp));
   return OK;
 }
 

include/nuttx/fs/fs.h

@@ -1132,6 +1132,25 @@ int fs_getfilep(int fd, FAR struct file **filep);
 
 int file_close(FAR struct file *filep);
 
+/****************************************************************************
+ * Name: file_close_without_clear
+ *
+ * Description:
+ *   Close a file that was previously opened with file_open(), but without
+ *   clear filep.
+ *
+ * Input Parameters:
+ *   filep - A pointer to a user provided memory location containing the
+ *           open file data returned by file_open().
+ *
+ * Returned Value:
+ *   Zero (OK) is returned on success; A negated errno value is returned on
+ *   any failure to indicate the nature of the failure.
+ *
+ ****************************************************************************/
+
+int file_close_without_clear(FAR struct file *filep);
+
 /****************************************************************************
  * Name: nx_close_from_tcb
  *