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

MSYS2 / Git broken in Windows 11 Canary build 27744 #232

Closed
snickler opened this issue Nov 11, 2024 · 9 comments · Fixed by #233
Closed

MSYS2 / Git broken in Windows 11 Canary build 27744 #232

snickler opened this issue Nov 11, 2024 · 9 comments · Fixed by #233

Comments

@snickler
Copy link

Currently experiencing this on the Arm64 version of Windows, but it may also apply to x64 versions of Windows.

Both the standalone MSYS2 and the one bundled with Git for Windows fails to load in Windows 11 Canary build 27744. Posting this issue to bring awareness to anyone how may happen to be on the Windows Insider Canary builds and have noticed anything related to using MSYS2 failing to load.

https://x.com/sinclairinat0r/status/1855733791165169916

@lazka
Copy link
Member

lazka commented Nov 11, 2024

@dscho fyi

edit: seems to be "fixed" though

@dscho
Copy link
Collaborator

dscho commented Nov 11, 2024

edit: seems to be "fixed" though

Yep, I fast-tracked this in git-for-windows/msys2-runtime#76, and did not have time to backport it to MSYS2, too. Will do so right now.

@lazka
Copy link
Member

lazka commented Nov 11, 2024

Ah, no prob, I can do too if needed.

@lazka lazka transferred this issue from msys2/msys2.github.io Nov 11, 2024
@dscho
Copy link
Collaborator

dscho commented Nov 11, 2024

Ah, no prob, I can do too if needed.

Already did it: #233

@pmsjt
Copy link

pmsjt commented Nov 11, 2024

Thanks for addressing this for Arm64. It is really appreciated. Meanwhile we also worked this around in Windows to avoid this problem.

Please note that there are ways this logic can fail even on x86-64 systems, with no emulation or Arm64 code involved.

@dscho
Copy link
Collaborator

dscho commented Nov 11, 2024

Please note that there are ways this logic can fail even on x86-64 systems, with no emulation or Arm64 code involved.

Yes, this is a known problem. But without any alternative to achieve the same effect, Cygwin has no choice but to keep doing it the hacky way they do. Nothing we here in the MSYS2 project can do about it, anyway.

@pmsjt
Copy link

pmsjt commented Nov 11, 2024

Agree. MSYS2 is just working with the cards delt by Cygwin. Cygwin can improve their mechanism tho. It is expensive but possible. Searching for bytes and assuming they are at an instruction boundary is not a robust mechanism. Adding even partial instruction decoding to their logic, for each instruction leading up for what they are looking for would have 2 immediate benefits:

  1. They'd always be at an instruction boundary, which avoids very likely false positives.
  2. It would easily detect when detection goes into the weeds. Today the logic plows through completely invalid code none the wiser.

@dscho
Copy link
Collaborator

dscho commented Nov 11, 2024

Adding even partial instruction decoding to their logic, for each instruction leading up for what they are looking for

Interesting idea! Looking at https://wiki.osdev.org/X86-64_Instruction_Encoding, I would wager a guess that even partial instruction decoding would still be non-trivial to implement. And looking at the source code of find_fast_cwd_pointer(), I suspect that the code of RtlGetCurrentDirectory_U() may have changed often enough (and in non-trivial ways) that even partial instruction decoding would likely not be the ultimate solution: the layer on top that needs to find certain instructions would still have to be adapted as often as the find_fast_cwd_pointer() function has to be adapted in the current incarnation, as it is targeting a seemingly fast-moving target.

@pmsjt
Copy link

pmsjt commented Nov 11, 2024

This is why I acknowledged it would be more expensive. It does help to have a clear view of what the real cost of a solution when such solution is picked ;-).

One additional note is that the risk is there even if the source code doesn't change at all. Other things changing in the same binary cause data and other code to shift around and, suddenly, the embedded offset of some instruction now looks like a call, a jump, or an INT instruction and failures arise even without any changes to the code.

This is what happended actually. In this Canary release no source change was made to this function (or its FFS) but the offset of another instruction now looked like a call instruction to fast_cwd and that's why the crash showed up without any changes.

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 a pull request may close this issue.

4 participants