clink icon indicating copy to clipboard operation
clink copied to clipboard

Not compatible with Windows Insider Build 20150

Open koppor opened this issue 4 years ago • 35 comments

Version 0.4.9:

grafik

Version 1.0.0a1 does not start at all. cmd.exe hangs during start.

Microsoft Windows 10 Pro Insider Preview 
10.0.20150 Build 20150

koppor avatar Jun 28 '20 12:06 koppor

It seems that you are using it with ConEmu. You need to copy (not rename) clink_x64.dll to clink_dll_x64.dll and clink_x86.dll to clink_dll_x86.dll. This will resolve your problem.

vladimir-poleh avatar Jul 01 '20 23:07 vladimir-poleh

Clink v0.4.9 offers clink_dll_x86.dll and clink_dll_x64.dll. I copied them to the new DLL names, still no success:

grafik

With the most recent release, I had to apply your "patch" resulting in a hang during start:

grafik

koppor avatar Jul 07 '20 05:07 koppor

Tested Clink v0.4.9 with ConEmu v20.06.15 on Microsoft Windows [Version 10.0.19041.329]

For me it works, did you read instal-doc: https://conemu.github.io/en/TabCompletion.html#ConEmu_and_clink

return42 avatar Jul 08 '20 07:07 return42

In "old" Windows versions, it worked fine for me (I'm using clink since more than two years now). Note that I explicitly provided my Windows version. It is higher than the version provided by you.

Build 19041 was released in May 2020, my build 20150 is an insider preview released on June 17th, 2020. I am well aware that using insider previews might break things. I fear that fixing a broken clink is not on Microsoft's priority list. One reason might be that PowerShell offers Emacs keybindings, too.

$ ver

Microsoft Windows [Version 10.0.20150.1000]

@return42 Would you mind to update to the "Beta Channel" and report whether it is broken, for you, too? And if it still works, could you update to the "Dev Channel"?

grafik

grafik

koppor avatar Jul 08 '20 09:07 koppor

Note that I explicitly provided my Windows version.

Now, with renamed title it is more clear that you are not asking about Version 2004 :)

@return42 Would you mind to update to the "Beta Channel" and report whether it is broken, for you, too? And if it still works, could you update to the "Dev Channel"?

No please not .. costs me 1/2 day to get the 2004 update running .. I am glad that after the first start-up difficulties it is now working. I better not change anything, otherwise this MS-win crap will collapse again :-o

Sorry, can't help.

return42 avatar Jul 08 '20 10:07 return42

Note that I explicitly provided my Windows version.

Now, with renamed title it is more clear that you are not asking about Version 2004 :)

@return42 Would you mind to update to the "Beta Channel" and report whether it is broken, for you, too? And if it still works, could you update to the "Dev Channel"?

No please not .. costs me 1/2 day to get the 2004 update running .. I am glad that after the first start-up difficulties it is now working. I better not change anything, otherwise this MS-win crap will collapse again :-o

Sorry, can't help.

Works in beta channel for me, but not in dev.

Lunchb0ne avatar Jul 13 '20 12:07 Lunchb0ne

From what I'm seeing it's a bit more subtle. It seems to me that it does finish loading, but some of the pipes are broken. Specifically it handles key presses, executes the commands, and does pipe the output. It doesn't seem to pipe the prompt to the console, or hook the magic keys (like arrow up etc.) clink_win20161_bug (I can confirm the regression started with build 20150 and is still happening in build 20161)

Maybe @bitcrazed or @craigaloewen can help triage this?

refack avatar Jul 13 '20 19:07 refack

i'm using Windows Terminal and have the same issue

Congee avatar Jul 20 '20 12:07 Congee

Can confirm having the same issue using Windows Terminal or Cmder on Windows build 20170.

talmo avatar Jul 22 '20 16:07 talmo

@mridgers is this project still alive?

Congee avatar Jul 22 '20 16:07 Congee

Found the underlying issue; KernelBase.dll changed in the Insider build. Log on Windows Build 20221 (insider):

21316 do_inject                  185 System: ver=6.2 0.0 arch=9 cpus=16 cpu_type=8664 page_size=4096
21316 do_inject                  190 Version: 0.4.9
21316 do_inject                  191 DLL: C:\tools\Cmder\vendor\clink\clink_dll_x64.dll
21316 do_inject                  193 Parent pid: 7680
21316 check_dll_version           52 DLL version: 00000004 0009b2fe
21316 do_inject_impl             283 Creating remote thread at 00007FF8A33E0390 with parameter 00000196B4FA0000
 7680 set_rl_readline_name        58 Setting rl_readline_name to 'cmd.exe'
 7680 hook_trap_veh              120 VEH hit - caller is 00007FF6276F09F5.
 7680 hook_jmp                   408 Attemping jump hook.
 7680 hook_jmp                   409 Target is kernelbase.dll, ReadConsoleW @ 00007FF8A1C581B0
 7680 hook_jmp_impl              351 Attempting to hook at 00007FF8A1C581B0 with 00007FF8700363A0
 7680 get_instruction_length     316 Matched prolog F1845FE9 (mask = 000000FF)
 7680 write_trampoline_out       202 No nop slide or int3 block detected prior to hook target.
 7680 hook_jmp_impl              387 Failed to write trampoline out.
 7680 hook_jmp                   415 Jump hook failed.
 7680 apply_hook_jmp              73 Unable to hook ReadConsoleW in kernelbase.dll
 7680 hook_trap_veh              125 Hook trap 00007FF8700362D4 failed.

KernelBase.dll from windows build 19041 (i.e. non-insider, working) image

KernelBase.dll from windows build 20221 (i.e. insider, not working) image

It checks for the 'cc' instructions that precede ReadConsoleW so that it knows it's safe to patch into: clink\shared\hook.c image

However, since it appears that those are just garbage instructions that never get reached, we can just comment out the return NULL; and it works fine.

nikitalita avatar Oct 03 '20 03:10 nikitalita

patched version is here: https://github.com/nikitalita/clink/tree/v0.4.9-insider-fix_mridgers

nikitalita avatar Oct 03 '20 03:10 nikitalita

Wow that's great news

Lunchb0ne avatar Oct 03 '20 06:10 Lunchb0ne

Thank you @nikitalita for identifying the issue and a workaround.

The 0x90/0xcc test is intended to ensure the patch doesn't overwrite real code, and it currently is hard coded to use the 5 preceding bytes. For a long term solution I'll make it scan nearby for 5 consecutive bytes that are safe to use, rather than using a hard-coded location.

chrisant996 avatar Oct 03 '20 07:10 chrisant996

excellent! Thanks @chrisant996 !

nikitalita avatar Oct 03 '20 14:10 nikitalita

Apologies, I seem to have included the wrong screenshot for the Windows Insider build 20221, that screenshot was for ReadConsoleInputW, not ReadConsoleW image

nikitalita avatar Oct 03 '20 19:10 nikitalita

Apologies, I seem to have included the wrong screenshot for the Windows Insider build 20221, that screenshot was for ReadConsoleInputW, not ReadConsoleW

Oh, and there's only a 2 byte "safe" region available preceding ReadConsoleW.

Is there a 5+ byte region anywhere within 125 bytes before the beginning of ReadConsoleW? Or with 125 in either direction?

I might need to set up an Insiders Dev channel VM after all.

chrisant996 avatar Oct 03 '20 21:10 chrisant996

Looks like one right here, after the function jumps because of the if/else:

image

nikitalita avatar Oct 03 '20 22:10 nikitalita

decompiled function: image

nikitalita avatar Oct 03 '20 22:10 nikitalita

I'm evaluating using Microsoft Detours for hooking APIs. It handles a lot more edge cases/etc than what's currently in clink (and even supports multiple chipsets, though that's not currently needed by clink). Detours 4.x uses the MIT License.

Detours should resolve this whole class of issues once and for all. That's attractive. 😉

chrisant996 avatar Oct 04 '20 20:10 chrisant996

@nikitalita You can click on the three dots and then choose "Edit" to change the markdown of a comment (and or instance update the screenshot).

Another thing @nikitalita: Could you provide a binary? I did not have success building the thing on GitHub (https://github.com/chrisant996/clink/pull/4#issuecomment-703314239).

koppor avatar Oct 04 '20 20:10 koppor

Merged PR from @nikitalita, and will investigate switching to using Detours moving forward. Merged PR from @koppor to enable builds produced by github.

chrisant996 avatar Oct 04 '20 21:10 chrisant996

@koppor here you go: https://github.com/nikitalita/clink/releases/tag/v1.1.0.72181d

nikitalita avatar Oct 04 '20 21:10 nikitalita

@koppor BTW, as for fixing clink builds on github actions, take a look at what @cmderdev is doing with their appveyor setup: https://github.com/cmderdev/clink/tree/v0.4.9

nikitalita avatar Oct 04 '20 21:10 nikitalita

BTW, here's a build of v0.4.9 with the fix backported: https://github.com/nikitalita/clink/releases/tag/v0.4.9-insiderfix

nikitalita avatar Oct 04 '20 22:10 nikitalita

aw shit, it broke again on 20246. Investigating why.

nikitalita avatar Nov 02 '20 19:11 nikitalita

Looks like in the newest builds it no longer has that int3 block it had previously. @chrisant996 have you implemented detours in your fork yet? image

nikitalita avatar Nov 02 '20 19:11 nikitalita

Not yet, but it sounds like that needs to be a priority.

I was deferring it because it looks like there are some bugs in Detours that people have fixed in forks, and I'll need to collect + review fixes and determine what state of the code base to integrate into Clink.

chrisant996 avatar Nov 02 '20 20:11 chrisant996

Looks like in the newest builds it no longer has that int3 block it had previously.

I think my original change walked past adjacent functions, and I think you restricted it to only search in the immediately adjacent space (which I agree is safer). Am I remembering correctly? Does removing that restriction enable it to work again at least for the moment?

chrisant996 avatar Nov 02 '20 20:11 chrisant996

Looks like in the newest builds it no longer has that int3 block it had previously.

I think my original change walked past adjacent functions, and I think you restricted it to only search in the immediately adjacent space (which I agree is safer). Am I remembering correctly? Does removing that restriction enable it to work again at least for the moment?

I had just restricted the backwards search to stop searching backwards if it hit a RET, I didn't restrict fowards searching. Lifting that wouldn't help here, as the closest int3 block started at exactly 126 bytes forward AND after another function. I just lifted the search size from 125 bytes to 135 bytes and it's working now. I'll submit a patch, but yeah, these kinds of fixes seem increasingly fragile.

nikitalita avatar Nov 03 '20 00:11 nikitalita