xiaoxiang781216 commented on code in PR #16326: URL: https://github.com/apache/nuttx/pull/16326#discussion_r2075896334
########## include/nuttx/fs/fs.h: ########## @@ -462,6 +462,7 @@ struct file int f_oflags; /* Open mode flags */ #ifdef CONFIG_FS_REFCOUNT atomic_t f_refs; /* Reference count */ + sem_t f_closem; /* Free: file is fully closed */ Review Comment: from the spec(https://man7.org/linux/man-pages/man2/dup.2.html): ``` After a successful return, the old and new file descriptors may be used interchangeably. Since the two file descriptors refer to the same open file description, they share file offset and file status flags; for example, if the file offset is modified by using [lseek(2)](https://man7.org/linux/man-pages/man2/lseek.2.html) on one of the file descriptors, the offset is also changed for the other file descriptor. The two file descriptors do not share file descriptor flags (the close-on-exec flag). The close-on-exec flag (FD_CLOEXEC; see [fcntl(2)](https://man7.org/linux/man-pages/man2/fcntl.2.html)) for the duplicate descriptor is off. ``` we should split file into two struct: ``` struct file { #ifdef CONFIG_FS_REFCOUNT atomic_t f_refs; /* Reference count */ #endif off_t f_pos; /* File position */ FAR struct inode *f_inode; /* Driver or file system interface */ FAR void *f_priv; /* Per file driver private data */ #if CONFIG_FS_LOCK_BUCKET_SIZE > 0 bool locked; /* Filelock state: false - unlocked, true - locked */ #endif }; struct fd { struct file *file; int f_oflags; /* Open mode flags */ #ifdef CONFIG_FDSAN uint64_t f_tag_fdsan; /* File owner fdsan tag, init to 0 */ #endif #ifdef CONFIG_FDCHECK uint8_t f_tag_fdcheck; /* File owner fdcheck tag, init to 0 */ #endif #if CONFIG_FS_BACKTRACE > 0 FAR void *f_backtrace[CONFIG_FS_BACKTRACE]; /* Backtrace to while file opens */ #endif }; ``` with this separation, we can avoid the crash natively. ########## fs/inode/fs_files.c: ########## @@ -317,11 +331,36 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, return -EBADF; } + /* dup3() and dup2() dictate that fd2 must be closed prior to reuse */ + + filep = files_fget(list, fd2); + if (filep) + { + /* The file exists and is open, close it here */ + + fs_putfilep(filep); + file_close_wait(filep); + } + + /* This should not fail now */ + filep = files_fget_by_index(list, fd2 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK, fd2 % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK, &new); + /* If return value is NULL, it means the file is partially open. This means + * the userspace is racing against itself. To prevent the kernel from + * crashing due to access to invalid file pointer, just make the user try Review Comment: but the spec require that open/close is atomic(https://man7.org/linux/man-pages/man2/dup.2.html): ``` If the file descriptor newfd was previously open, it is closed before being reused; the close is performed silently (i.e., any errors during the close are not reported by dup2()). The steps of closing and reusing the file descriptor newfd are performed atomically. This is important, because trying to implement equivalent functionality using [close(2)](https://man7.org/linux/man-pages/man2/close.2.html) and dup() would be subject to race conditions, whereby newfd might be reused between the two steps. Such reuse could happen because the main program is interrupted by a signal handler that allocates a file descriptor, or because a parallel thread allocates a file descriptor. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
