search

mridgers / clink

Bash's powerful command line editing in cmd.exe
mridgers.github.io/clink
GNU General Public License v3.0
3.15k stars 286 forks source link

Not compatible with Windows Insider Build 20150 #543

Open koppor opened 4 years ago

koppor commented 4 years ago

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
vladimir-poleh commented 4 years ago

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.

koppor commented 4 years ago

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

return42 commented 4 years ago

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

koppor commented 4 years ago

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

return42 commented 4 years ago

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.

Lunchb0ne commented 3 years ago

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.

refack commented 3 years ago

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?

Congee commented 3 years ago

i'm using Windows Terminal and have the same issue

talmo commented 3 years ago

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

Congee commented 3 years ago

@mridgers is this project still alive?

nikitalita commented 3 years ago

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 commented 3 years ago

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

Lunchb0ne commented 3 years ago

Wow that's great news

chrisant996 commented 3 years ago

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.

nikitalita commented 3 years ago

excellent! Thanks @chrisant996 !

nikitalita commented 3 years ago

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

chrisant996 commented 3 years ago

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.

nikitalita commented 3 years ago

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

image

nikitalita commented 3 years ago

decompiled function: image

chrisant996 commented 3 years ago

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. 😉

koppor commented 3 years ago

@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).

chrisant996 commented 3 years ago

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

nikitalita commented 3 years ago

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

nikitalita commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

nikitalita commented 3 years ago

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

chrisant996 commented 3 years ago

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 commented 3 years ago

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?

nikitalita commented 3 years ago

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 commented 3 years ago

new v0.4.9 build with this fix: https://github.com/nikitalita/clink/releases/tag/v0.4.9-insiderfixtwo

chrisant996 commented 3 years ago

The reason I restricted it to less than 127 in either direction is it seemed to be producing a 2 byte relative jump that can go up to 127 bytes in either direction.

How is the jump being successful when the distance is greater than 7 bits?

Or did you change the instruction set?

nikitalita commented 3 years ago

The long jump instruction is being written starting at offset byte 126. The two-byte relative jump can just barely make it there. I've adjusted the patch accordingly; It'll only check 127 bytes backwards, and 131 bytes forwards.

chrisant996 commented 3 years ago

Ok great, thank you. I'll start working on Detours sometime soon.

Get Outlook for Androidhttps://aka.ms/ghei36

From: nikitalita notifications@github.com Sent: Tuesday, November 3, 2020 9:32:22 AM To: mridgers/clink clink@noreply.github.com Cc: Chris Antos sparrowhawk996@gmail.com; Mention mention@noreply.github.com Subject: Re: [mridgers/clink] Not compatible with Windows Insider Build 20150 (#543)

The long jump instruction is being written starting at offset byte 126. The two-byte relative jump can just barely make it there. I've adjusted the patch accordingly; It'll only check 127 bytes backwards, and 131 bytes forwards.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mridgers/clink/issues/543#issuecomment-721273392, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEFB4NY5C4YXR5OI7RZQ5BTSOA5CNANCNFSM4OKQHR4Q.

nikitalita commented 3 years ago

BTW, for people still getting bit by this issue, I recommend using chrisant996's version of clink instead of my patch: https://github.com/chrisant996/clink