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
