pussuw commented on code in PR #16361:
URL: https://github.com/apache/nuttx/pull/16361#discussion_r2086069196
##########
fs/inode/fs_files.c:
##########
@@ -66,61 +84,102 @@
* Name: files_fget_by_index
****************************************************************************/
-static FAR struct file *files_fget_by_index(FAR struct filelist *list,
- int l1, int l2, FAR bool *new)
+static FAR struct file *files_fget_by_index(FAR struct fdlist *list,
+ int l1, int l2,
+ FAR struct fd_flags *fdflags)
{
FAR struct file *filep;
+ FAR struct fd *fd;
irqstate_t flags;
flags = spin_lock_irqsave_notrace(&list->fl_lock);
- filep = &list->fl_files[l1][l2];
- spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
-#ifdef CONFIG_FS_REFCOUNT
- if (filep->f_inode != NULL)
- {
- /* When the reference count is zero but the inode has not yet been
- * released, At this point we should return a null pointer
- */
+ /* The callers have ensured that no out of bounds access can happen here */
- int32_t refs = atomic_read(&filep->f_refs);
- do
+ fd = &list->fl_fds[l1][l2];
+
+ /* The file pointer is removed before the file is freed, so there is no
+ * race condition here as long as we take a reference to the file before
+ * releasing file list lock.
+ */
+
+ filep = fd->f_file;
+ if (filep)
+ {
+ fs_reffilep(filep);
+ if (fdflags)
{
- if (refs == 0)
- {
- filep = NULL;
- break;
- }
+ fdflags->f_cloexec = fd->f_cloexec;
+#ifdef CONFIG_FDSAN
+ fdflags->f_tag_fdsan = fd->f_tag_fdsan;
+#endif
+#ifdef CONFIG_FDCHECK
+ fdflags->f_tag_fdcheck = fd->f_tag_fdcheck;
+#endif
}
- while (!atomic_try_cmpxchg(&filep->f_refs, &refs, refs + 1));
- }
- else if (new == NULL)
- {
- filep = NULL;
- }
- else if (atomic_fetch_add(&filep->f_refs, 1) == 0)
- {
- atomic_fetch_add(&filep->f_refs, 1);
- *new = true;
}
-#else
- if (filep->f_inode == NULL && new == NULL)
+ spin_unlock_irqrestore_notrace(&list->fl_lock, flags);
+
+ return filep;
+}
+
+/****************************************************************************
+ * Name: files_fget_by_fd
+ ****************************************************************************/
+
+static FAR struct file *files_fget_by_fd(FAR struct fdlist *list, int fd,
+ FAR struct fd_flags *flags)
+{
+ return files_fget_by_index(list, fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK,
+ fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK, flags);
+}
+
+/****************************************************************************
+ * Name: files_finstall
+ ****************************************************************************/
+
+static FAR struct file *files_finstall(FAR struct fdlist *list, int fd,
+ FAR struct file *filep,
+ FAR struct fd_flags *fdflags)
+{
+ int i = fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ int j = fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK;
+ FAR struct file *oldfilep;
+ irqstate_t flags;
+
+ /* Atomically replace the file pointer */
+
+ flags = spin_lock_irqsave_notrace(&list->fl_lock);
+
+ oldfilep = list->fl_fds[i][j].f_file;
+ list->fl_fds[i][j].f_file = filep;
+
+ if (flags)
Review Comment:
Oops, nice catch !
--
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
