This is an automated email from the ASF dual-hosted git repository. xiaoxiang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/nuttx.git
commit e1cd082c291a1c456731b693c8128ebfa4237953 Author: ligd <[email protected]> AuthorDate: Wed Dec 13 23:09:22 2023 +0800 fs: enhance dup3() mulit-threads saftey Signed-off-by: ligd <[email protected]> --- fs/inode/fs_files.c | 34 ++++++++++---------------------- fs/vfs/fs_close.c | 45 ++++++++++++++++++++++++++++++++++++++---- fs/vfs/fs_dup2.c | 54 ++++++++++++++++++++------------------------------- include/nuttx/fs/fs.h | 19 ++++++++++++++++++ 4 files changed, 91 insertions(+), 61 deletions(-) diff --git a/fs/inode/fs_files.c b/fs/inode/fs_files.c index bcf301d397..f1ebf7c885 100644 --- a/fs/inode/fs_files.c +++ b/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); + /* Get the file descriptor list. It should not be NULL in this context. */ + 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 || - fd2 < 0) + 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); - -#ifdef CONFIG_FDSAN - filep->f_tag_fdsan = file.f_tag_fdsan; -#endif - -#ifdef CONFIG_FDCHECK - filep->f_tag_fdcheck = file.f_tag_fdcheck; -#endif - - file_close(&file); + ret = file_dup3(files_fget(list, fd1), files_fget(list, fd2), flags); + if (ret < 0) + { + return ret; + } #ifdef CONFIG_FDCHECK - return ret < 0 ? ret : fdcheck_protect(fd2); + return fdcheck_protect(fd2); #else - return ret < 0 ? ret : fd2; + return fd2; #endif } diff --git a/fs/vfs/fs_close.c b/fs/vfs/fs_close.c index 3015223853..6058a41c04 100644 --- a/fs/vfs/fs_close.c +++ b/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); + } + return ret; +} + +/**************************************************************************** + * 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. */ - memset(filep, 0, sizeof(*filep)); + filep->f_inode = NULL; + +#ifdef CONFIG_FDCHECK + filep->f_tag_fdcheck = 0; +#endif + +#ifdef CONFIG_FDSAN + filep->f_tag_fdsan = 0; +#endif } return ret; diff --git a/fs/vfs/fs_dup2.c b/fs/vfs/fs_dup2.c index 7bf6102851..c7baf3a28f 100644 --- a/fs/vfs/fs_dup2.c +++ b/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; - temp.f_inode = inode; + filep2->f_priv = NULL; + 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; } diff --git a/include/nuttx/fs/fs.h b/include/nuttx/fs/fs.h index b41272f8b1..d4f598cdab 100644 --- a/include/nuttx/fs/fs.h +++ b/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 *
