nuttxpr commented on PR #16326:
URL: https://github.com/apache/nuttx/pull/16326#issuecomment-2854833494

   [**\[Experimental Bot, please feedback 
here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   
   
   The PR summary is missing some key information required by the NuttX 
guidelines.  While it explains the *why* (race conditions leading to crashes) 
and the general *how* (error handling and synchronization), it lacks specifics. 
The impact section also lacks detail.  Here's a breakdown of what's missing and 
how to improve it:
   
   **Summary - Missing Information:**
   
   * **What functional part of the code is being changed?**  Be precise.  
Mention the specific files and functions modified (e.g., `fs/vfs/fs_dup.c`, 
`dup2()`, `dup3()`).
   * **How does the change exactly work (what will change and how)?**  The 
description "Waiting for the file I/O to finish before closing" is vague.  What 
synchronization mechanism is used (e.g., mutex, semaphore)? How is the 
"waiting" implemented?  Provide more detail about the `-EBUSY` error handling.  
Where is it returned? What are the code changes that prevent the "disappearing 
file descriptors"?
   
   **Impact - Missing Information:**
   
   * While the impact claims "no impact,"  returning `-EBUSY` *is* a change in 
behavior.  The PR should clearly document this.  Even if unlikely, existing 
applications relying on the (broken) behavior of silently succeeding in race 
conditions might break.  Acknowledge this possibility.  Consider if any error 
handling should be added to user-space applications that use `dup2/3`.
   * **Impact on hardware:** While the testing mentions specific architectures, 
the Impact section should explicitly state which architectures are affected 
(RISC-V).  This helps reviewers quickly understand the scope.
   * **Impact on security:** While the fix likely improves stability, consider 
if it has any security implications.  Could the added synchronization 
mechanisms introduce new vulnerabilities (e.g., deadlocks)?  Even if the answer 
is "NO," explicitly stating it demonstrates that security was considered.
   * The Testing section mentions a "MPFS target." This should also be clearly 
listed in the Impact section under "hardware."
   
   **Testing - Missing Information:**
   
   * **Build Host(s):**  Provide details about the build host environment as 
requested.  This helps reproduce the build and tests.
   * **Testing logs before change / after change:** The PR says "we no longer 
crash," but doesn't provide actual logs. Include snippets of logs demonstrating 
the crash before the fix and the successful operation after the fix.  Even 
short snippets are better than nothing.  This provides concrete evidence of the 
problem and the solution.
   
   **Revised Example (Illustrative – Adapt to your actual changes):**
   
   ## Summary
   
   This PR fixes two race conditions in `dup2()` and `dup3()` (implemented in 
`fs/vfs/fs_dup.c`) that can lead to kernel crashes.  These race conditions 
occur when: (1) a file descriptor is duplicated while another thread is opening 
the same file; and (2) `close()` or `dup2/3()` is called while another thread 
is performing I/O on the same descriptor.
   
   The fix introduces a mutex (`g_dup_lock` in `fs/vfs/fs_dup.c`) to protect 
the critical section where file descriptor operations are performed.  Before 
manipulating a file descriptor, `dup2()` and `dup3()` now acquire this mutex.  
If the target file descriptor is currently involved in an open operation or 
I/O, `-EBUSY` is returned.  After the operations are complete, the mutex is 
released. This prevents concurrent access to the file descriptor table and 
ensures that file structures are not prematurely freed.
   
   Fixes: #[NuttX Issue Number (if applicable)]
   
   ## Impact
   
   * **Is new feature added? Is existing feature changed?**  Existing `dup2()` 
and `dup3()` functionality is fixed.
   * **Impact on user:** YES. Applications relying on the previous (broken) 
behavior where `dup2/3` silently succeeded during race conditions might now 
receive `-EBUSY` errors. They may need to implement error handling for this 
scenario.
   * **Impact on build:** NO.
   * **Impact on hardware:** YES. This fix impacts RISC-V architectures and 
MPFS target.
   * **Impact on documentation:** NO.  (Consider updating documentation if the 
`-EBUSY` error is a significant change.)
   * **Impact on security:** NO. The added mutex is carefully managed to 
prevent deadlocks or other security vulnerabilities.
   * **Impact on compatibility:** YES.  Backward compatibility is maintained, 
but applications relying on the erroneous behavior might experience different 
results.
   * **Anything else to consider:** None.
   
   ## Testing
   
   **Build Host(s):** Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
   
   **Target(s):** RISC-V (rv-virt:ksmp64, rv-virt:knsh64), MPFS configuration.
   
   **Testing logs before change:**
   
   ```
   [ostest] Assertion failed at file: sched/sched_addreadytorun.c line: 482 
task: 1048880
   ```
   
   **Testing logs after change:**
   
   ```
   [ostest] dup2 test passed!
   ```
   
   
   By providing this level of detail, you make it much easier for reviewers to 
understand, verify, and approve your PR. Remember to adapt the example to your 
specific implementation.


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