nuttxpr commented on PR #16288: URL: https://github.com/apache/nuttx/pull/16288#issuecomment-2838085748
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details in both sections and completely omits the "Impact" section's required details. Here's a breakdown of what's missing: **Summary:** * **Missing Context:** The summary mentions "fixes for TLS support," but doesn't explain *why* these fixes are necessary for stack canaries. It needs to explain the connection more clearly. What was broken before? How do these changes fix it? * **Insufficient Detail:** Saying "FSBASE register must be unique per thread" is too technical without further explanation. Why is that register important for stack canaries? How was it handled before, and what's different now? **Impact:** The "Impact" section is severely lacking. Simply stating "stack canaries works on x86" is not enough. Each of the following sub-points needs to be addressed with either "NO" or "YES" followed by a description if "YES": * **Is new feature added? Is existing feature changed?** (Clearly a new feature/change) * **Impact on user:** Will users need to do anything differently to enable or use this feature? Will any existing applications be affected? * **Impact on build:** Will any build flags need to be set? Will build times be affected? * **Impact on hardware:** The PR mentions x86, but are *all* x86 architectures/boards supported? Are there any specific hardware requirements? * **Impact on documentation:** Does the documentation need updating to explain how to enable and use stack canaries? Has documentation been provided in the PR? * **Impact on security:** This is a security feature, so the impact should explain the benefits and limitations of the added protection. * **Impact on compatibility:** Will this change break any existing code? Are there any compatibility concerns with older versions of NuttX? * **Anything else to consider?** Are there any known limitations, performance implications, or other potential issues? **Testing:** * **Insufficient Host Detail:** Saying "tested with `qemu-intel64/jumbo`" is not enough. Specify the host OS, compiler version, and any other relevant details about the build host environment. * **Missing "Before" Logs:** The PR says "Without these changes the system crashes," but doesn't include the actual crash logs *before* the change. These logs are crucial for demonstrating the problem being solved. * **Insufficient "After" Logs:** While "after" logs are present, they should demonstrate the successful operation of stack canaries. Simply saying the system doesn't crash isn't sufficient proof that the feature works correctly. Ideally, the logs should show a deliberate stack overflow attempt being caught by the canary mechanism. In short, the PR needs significantly more detail and a proper breakdown of the impact to meet the NuttX requirements. -- 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
