Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "GetOverlappedResult: More data available" on windows 10 #495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adambor
Copy link

@adambor adambor commented Dec 8, 2024

I've ran into an issue on windows 10, with the ledger-agent crashing with the following error:

  File "f:\trezor-agent\libagent\win_server.py", line 178, in recv
    chunk_size = win32pipe.GetOverlappedResult(self.handle, self.overlapped, False)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pywintypes.error: (234, 'GetOverlappedResult', 'More data is available.')

This PR is a fix for it, essentially just catching ERROR_MORE_DATA and returning the whole read buffer in that case.

@adambor
Copy link
Author

adambor commented Dec 14, 2024

Fixed the pycodestyle issue, it should now pass the CI.

@romanz
Copy link
Owner

romanz commented Dec 15, 2024

Please squash and rebase over latest master (the last CI failure should be fixed by #480).

@romanz
Copy link
Owner

romanz commented Dec 15, 2024

@gtbuchanan @SlugFiller could you please take a look?
I don't use Windows, so I would appreciate your feedback :)

@SlugFiller
Copy link
Contributor

I haven't encountered the error myself, which makes me not completely sure on when ERROR_MORE_DATA is received. I am uncertain if returning the full buffer is correct, here, as it might actually be returning an uninitialized buffer with garbage data.

At the very least, if the buffer has been completely filled, the operation should be over with NO_ERROR. It might actually be more correct to check if the task was cancelled, and if not, to try loop, wait, and attempt to read the result again.

@adambor
Copy link
Author

adambor commented Dec 16, 2024

IMO (there also isn't much clarity on the error from the microsoft side), the ERROR_MORE_DATA means that the read buffer was completely filled with data and there is still more data to be read, this is in line with my observation (the agent doesn't work without this fix on win10 19045.5131).

Moreover there already was an attempt to catch the ERROR_MORE_DATA error on lines 165-169, however it is not returned as an error code there, but instead thrown by the later win32pipe.GetOverlappedResult call.

@adambor adambor force-pushed the ab/fix-win-overlapped-result branch from ce18fb3 to e076b9c Compare December 16, 2024 10:10
@adambor
Copy link
Author

adambor commented Dec 16, 2024

Please squash and rebase over latest master (the last CI failure should be fixed by #480).

Squashed & rebased

@gtbuchanan
Copy link
Contributor

Here is the overlapped IO example from Microsoft. It does loop to call ReadFile and GetOverlappedResult again in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants