pussuw commented on code in PR #16326:
URL: https://github.com/apache/nuttx/pull/16326#discussion_r2076153514


##########
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):
   
   Yes I know this very well. open/close/dup is a minefield of race conditions 
and there is no known good way to handle it. My motivation was to prevent the 
kernel from crashing if the user app is poorly made. I did this with the 
minimal effort to the current implementation. This removes a kernel crash when 
using dup & close. I can leave this downstream if the solution is not 
acceptable.
   
   > and shouldn't return the error code(e.g. -EBUSY) in this case.
   
   As per the -EBUSY return value, we could handle EBUSY in libc, so the user 
doesn't see this. This would be a completely POSIX compliant solution.
   
   > and shouldn't return the error code
   
   Btw, a newer POSIX spec allows returning -1 to user, but doesn't mention 
EBUSY. We could return -1 and not touch errno ?
   `If the close operation fails to close fildes2, dup2() shall return -1 
without changing the open file description to which fildes2 refers. [1]`
   
   [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/dup.html



-- 
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

Reply via email to