x64dbg / x64dbgpy

Automating x64dbg using Python, Snapshots:
https://ci.appveyor.com/project/mrexodia/x64dbg-python/build/artifacts
MIT License
1.47k stars 70 forks source link

Hack/workaround to ignore Win10 write exception #32

Closed Andoryuuta closed 5 years ago

Andoryuuta commented 5 years ago

This is a quick hack/workaround for #31 which probably shouldn't be merged (simply ignores the exception). The proper solution would be to update to Win10 >= 1803.

WriteFile was bugged in some windows 10 versions before 1803 (see https://github.com/Microsoft/console/issues/40) and returned a incorrect length value, causing python's write() to fail (see https://bugs.python.org/issue32245).

Dagobert- commented 5 years ago

Hey guys check out https://comptonjapan.com

On Wed, Jan 9, 2019, 17:19 Andrew Gutekanst <notifications@github.com wrote:

This is a quick hack/workaround for #31 https://github.com/x64dbg/x64dbgpy/issues/31 which probably shouldn't be merged (simply ignores the exception). The proper solution would be to update to Win10 >= 1803.

WriteFile was bugged in some windows 10 versions before 1803 (see Microsoft/console#40 https://github.com/Microsoft/console/issues/40) and returned a incorrect length value, causing python's write() to fail (see https://bugs.python.org/issue32245).

You can view, comment on, or merge this pull request online at:

https://github.com/x64dbg/x64dbgpy/pull/32 Commit Summary

  • Add hack to ignore win10 write error

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/x64dbg/x64dbgpy/pull/32, or mute the thread https://github.com/notifications/unsubscribe-auth/AJGpfFgv5vBaHjIll-fvNvT9_8ONTAp8ks5vBpUogaJpZM4Z4m9q .

ngo commented 5 years ago

Does not seem to be specific to win10 < 1803. I am on 1803 and still have this issue.

a1ext commented 5 years ago

Looks ok to me, but I didn't reproduce the issue.

ngo commented 5 years ago

I've just checked and in my case the errno is 9 every time, not 0 like in original report. I will try to debug the issue further.

a1ext commented 5 years ago

ping me on telegram @a14xt

mrexodia commented 5 years ago

Should I merge this or not?

a1ext commented 5 years ago

Should I merge this or not?

Not yet. @Andoryuuta, could you remove errno check in the except block? I saw 0 and 9 errnos, but lets make it safe

ngo commented 5 years ago

@mrexodia I'm now debugging the issue with the help of @a1ext. I will write our conclusion on the proper fix once it is ready. This PR does not address the problem fully, because it only ignores error with errno=0, whereas on my setup I have errno=9.

@a1ext yesterday was also able to reproduce the issue (with errno=0) on his setup.

mrexodia commented 5 years ago

I added you both to the repo, so you should be able to push if the “allow edits by maintainers” was checked. Merge as you see fit :)

ngo commented 5 years ago

Thanks!

ngo commented 5 years ago

@mrexodia so after a bit of debugging we are quite sure that the try: except block that ignores IOErrors (but without the errno check, because both me and @a1ext have seen errno = 0 as well as errno = 9) is the right solution.

We've also found that in case @a1ext 's labeless plugin is also loaded, the error will disappear because that plugin also hooks stdout and stderr.

We will allow @Andoryuuta a couple days to fix the PR himself, because it is the proper way in terms of attribution, and then in case we don't hear from him we'll do the neccessary edits.

a1ext commented 5 years ago

I have just added stdout/stderr proxyfying to the original stream (in Labeless) https://github.com/a1ext/labeless/commit/b9ad867cc9bc2c95618381e4bdd6e608da5598be with wider exceptions suppressing. Now Labeless won't eat output from x64dbgpy.

Andoryuuta commented 5 years ago

@a1ext @ngo Sorry for the delayed response, I haven't looked at my GitHub notifications in a while.

I removed the errno check and updated the comment really quick. If it is not good in some way, feel free to close this PR and do the changes yourself, no need to worry about attribution. :)

mrexodia commented 5 years ago

I fixed AppVeyor to use choco install swig --version 3.0.12 and it builds again. Here is the appveyor.yml if anyone is interested to put it in the repository:

version: 1.0.{build}
image: Visual Studio 2015
build_script:
- cmd: >-
    git submodule update --init --recursive

    choco install -y swig --version 3.0.12

    setenv.bat

    msbuild.exe x64dbgpy.sln /verbosity:minimal /t:Build /p:Configuration=Release;Platform=Win32

    msbuild.exe x64dbgpy.sln /verbosity:minimal /t:Build /p:Configuration=Release;Platform=x64

    call install32.bat

    call install64.bat
artifacts:
- path: release

Also: feel free to merge as you see fit. I'm not actually using this project right now and there are no tests so please test and merge if you find some time @a1ext @ngo.

ngo commented 5 years ago

I've just merged the fix, thanks to all of you for collaboration!