Prevent additional attempts at closing HID braille connections#16712
Conversation
…an corrupt the heap.
WalkthroughThe update in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Hid
Client->>+Hid: new Hid()
Hid-->>Client: Initialize (self._isClosed = False)
Client->>+Hid: close()
alt If self._isClosed is False
Hid-->>+Client: Close the device (set self._isClosed = True)
else If self._isClosed is True
Hid-->>Client: Device already closed
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| if self._isClosed: | ||
| pass | ||
| super(Hid, self).close() | ||
| winKernel.closeHandle(self._file) | ||
| self._file = None | ||
| hidDll.HidD_FreePreparsedData(self._pd) | ||
| self._isClosed = True |
There was a problem hiding this comment.
The conditional check in the close method effectively prevents multiple closures of the device. However, the use of pass in the conditional block is a bit misleading as it suggests that no action is taken when the device is already closed. It would be clearer to include a log message or a comment explaining that no further action is needed.
if self._isClosed:
- pass
+ log.debug("Attempted to close an already closed device.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._isClosed: | |
| pass | |
| super(Hid, self).close() | |
| winKernel.closeHandle(self._file) | |
| self._file = None | |
| hidDll.HidD_FreePreparsedData(self._pd) | |
| self._isClosed = True | |
| if self._isClosed: | |
| log.debug("Attempted to close an already closed device.") | |
| return | |
| super(Hid, self).close() | |
| winKernel.closeHandle(self._file) | |
| self._file = None | |
| hidDll.HidD_FreePreparsedData(self._pd) | |
| self._isClosed = True |
The sequence of operations in the close method is logically sound, ensuring resource cleanup and state update. However, consider checking and logging an error if HidD_FreePreparsedData fails, as this could lead to resource leaks.
if not hidDll.HidD_FreePreparsedData(self._pd):
+ log.error("Failed to free preparsed data.")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._isClosed: | |
| pass | |
| super(Hid, self).close() | |
| winKernel.closeHandle(self._file) | |
| self._file = None | |
| hidDll.HidD_FreePreparsedData(self._pd) | |
| self._isClosed = True | |
| if self._isClosed: | |
| pass | |
| super(Hid, self).close() | |
| winKernel.closeHandle(self._file) | |
| self._file = None | |
| if not hidDll.HidD_FreePreparsedData(self._pd): | |
| log.error("Failed to free preparsed data.") | |
| self._isClosed = True |
See test results for failed build of commit 06715c35d6 |
|
@coderabbitai please update the title |
|
Tip For best results, initiate chat on the files or code changes. @seanbudd, I noticed you requested an update to the PR title. Could you please specify the new title you'd like to use? Once you provide that, I can assist further in ensuring it reflects accurately in your documentation or discussions, but please note that I'm unable to directly change the PR title myself. You would need to update it in the GitHub interface. |
Link to issue number:
Fixes #16218
Summary of the issue:
NVDA could silently crash when a Braille HID device was disconnected/closed.
Description of user facing changes
No more crashes under the above conditions.
Description of development approach
Introduced a flag that tracks if the close() method has already been called, and don't perform the operation again.
Testing strategy:
Tried to reproduce the crash.
Known issues with pull request:
None.
Code Review Checklist:
Summary by CodeRabbit