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

Avoid sharing cygheaps across Cygwin versions #48

Conversation

dscho
Copy link
Member

@dscho dscho commented Jan 30, 2023

It frequently leads to problems when trying, say, to call from Git for Windows' Bash into Cygwin's or MSYS2's, merely because sharing that data is pretty finicky.

For example, using the Git for Windows that is current at time of writing, trying to call MSYS2's Bash from Git for Windows' Bash fails somewhere in _cmalloc(), without any error message, and with the rather misleading exit code 127 (a code which is reserved to indicate that a command was not found).

Let's just treat these as completely incompatible with one another, by virtue of using a different CHILD_INFO_MAGIC constant.

One consequence is that spawned MSYS processes using a different MSYS2 runtime will not be visible as such to the parent process, i.e. they cannot share any resources such as pseudo terminals. But that's okay, they are simply treated as if they were regular Win32 programs.

This should also help the scenario where interactions between two different versions of Git for Windows lead to those infamous cygheap base mismatch detected problems mentioned e.g. in git-for-windows/git#4255

This fixes git-for-windows/git#4255

It frequently leads to problems when trying, say, to call from Git for
Windows' Bash into Cygwin's or MSYS2's, merely because sharing that data
is pretty finicky.

For example, using the Git for Windows that is current at time of
writing, trying to call MSYS2's Bash from Git for Windows' Bash fails
somewhere in `_cmalloc()`, without any error message, and with the
rather misleading exit code 127 (a code which is reserved to indicate
that a command was not found).

Let's just treat these as completely incompatible with one another, by
virtue of using a different `CHILD_INFO_MAGIC` constant.

One consequence is that spawned MSYS processes using a different MSYS2
runtime will not be visible as such to the parent process, i.e. they
cannot share any resources such as pseudo terminals. But that's okay,
they are simply treated as if they were regular Win32 programs.

This should also help the scenario where interactions between two
different versions of Git for Windows lead to those infamous `cygheap
base mismatch detected` problems mentioned e.g. in
git-for-windows/git#4255

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho self-assigned this Jan 30, 2023
A recent binutils version introduced `libsframe` and made it a
dependency of `libbfd`. Which means that we have to link that library
into `dumper.exe`, too, if it exists.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho force-pushed the prevent-different-msys2-runtime-versions-from-sharing-cygheaps branch from a7f7189 to 4704d29 Compare January 31, 2023 09:05
@dscho dscho marked this pull request as ready for review January 31, 2023 13:32
@dscho
Copy link
Member Author

dscho commented Jan 31, 2023

I tested this a bit and am fairly confident that it 1) solves the problem and 2) does not introduce other problems.

@@ -545,7 +545,7 @@ get_cygwin_startup_info ()
child_info *res = (child_info *) si.lpReserved2;

if (si.cbReserved2 < EXEC_MAGIC_SIZE || !res
|| res->intro != PROC_MAGIC_GENERIC || res->magic != CHILD_INFO_MAGIC)
|| res->intro != PROC_MAGIC_GENERIC || res->magic != (CHILD_INFO_MAGIC ^ CYGWIN_VERSION_DLL_COMBINED))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised at first at the use of bitwise XOR here, as I would expect this to be a bitwise OR if these were flags. However, CYGWIN_VERSION_DLL_COMBINED is an integer with multiple bits (currently set to 3003006 to represent 3.3.6).

So this "magic" is very magical, it seems (and you do a similar update in sigproc.cc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the commit message motivates this well, saying that we should use a new magic value whenever Git for Windows' version of MSYS2 is updated. (It's not accurate that only a new Git version would change this magic, but if two Git for Windows releases have the identical MSYS2 version, then they should be compatible still.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. The idea here is to have some kind of unique identifier that won't ever be shared between different MSYS2 runtime versions. Hence the XOR (because the OR would decrease the entropy).

Copy link

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak for all of the magic in this PR, but it seems like a reasonable approach. It definitely seems like the compatibility issue will be avoided by further partitioning shared memory, not combining any previously-separated partitions.

@@ -545,7 +545,7 @@ get_cygwin_startup_info ()
child_info *res = (child_info *) si.lpReserved2;

if (si.cbReserved2 < EXEC_MAGIC_SIZE || !res
|| res->intro != PROC_MAGIC_GENERIC || res->magic != CHILD_INFO_MAGIC)
|| res->intro != PROC_MAGIC_GENERIC || res->magic != (CHILD_INFO_MAGIC ^ CYGWIN_VERSION_DLL_COMBINED))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the commit message motivates this well, saying that we should use a new magic value whenever Git for Windows' version of MSYS2 is updated. (It's not accurate that only a new Git version would change this magic, but if two Git for Windows releases have the identical MSYS2 version, then they should be compatible still.)

@dscho dscho merged commit ebcc274 into git-for-windows:main Jan 31, 2023
@dscho dscho deleted the prevent-different-msys2-runtime-versions-from-sharing-cygheaps branch January 31, 2023 14:09
dscho added a commit to dscho/MSYS2-packages that referenced this pull request Jan 31, 2023
…e versions

When calling, say, a Cygwin Bash (or even a regular MSYS2 Bash, or even
a MinGit Bash of a different Git for Windows version), it is possible
that the MSYS2 runtime of the called Bash is incompatible with the MSYS2
runtime of the caller. When that happens, all kinds of chaos can ensue.

One possible symptom is that the MSYS2 runtime aborts with the error
message "cygheap base mismatch detected".

Another possible symptom is that the MSYS2 runtime (or the Cygwin
runtime) exits silently, with exit code 127 (which is misleading as that
exit code is reserved to imply that an executable could not be found).

With this patch, we try harder to prevent such a thing from happening.

This is a companion to git-for-windows/msys2-runtime#48

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to dscho/MSYS2-packages that referenced this pull request Jan 31, 2023
…e versions

When calling, say, a Cygwin Bash (or even a regular MSYS2 Bash, or even
a MinGit Bash of a different Git for Windows version), it is possible
that the MSYS2 runtime of the called Bash is incompatible with the MSYS2
runtime of the caller. When that happens, all kinds of chaos can ensue.

One possible symptom is that the MSYS2 runtime aborts with the error
message "cygheap base mismatch detected".

Another possible symptom is that the MSYS2 runtime (or the Cygwin
runtime) exits silently, with exit code 127 (which is misleading as that
exit code is reserved to imply that an executable could not be found).

With this patch, we try harder to prevent such a thing from happening.

This is a companion to git-for-windows/msys2-runtime#48

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added a commit to git-for-windows/git-for-windows-automation that referenced this pull request Feb 21, 2023
When the MSYS2-runtime version changed recently, the `msys-2.0.dll`
contents of the minimal SDK diverged from the contents of the same file
in the Git for Windows installation in `C:\Program Files`.

Since we adjust the `PATH` before caching, that matters because the
`gzip` executable that is found in the `PATH` by `tar -z` is the version
from the minimal SDK, which therefore insists on using the MSYS2 runtime
from the minimal SDK, while the `tar` executable used by @actions/cache
is the `C:\Program Files\Git\usr\bin\tar.exe` one and hence uses a
potentially-incompatible MSYS2 runtime version of the Git for Windows
installation.

In git-for-windows/msys2-runtime#48, we tried to
address this by teaching the MSYS2 runtime to avoid talking to a
different MSYS2 runtime version. But the indicator used in that Pull
Request is the version of the _Cygwin_ runtime on which it is based.
Which means that Git for Windows-only changes in the MSYS2 runtime are
not reflected in that indicator, and the MSYS2 runtime _still_ tries to
talk to the other version, and the only symptom the user gets is a terse
`Child returned status 127`.

Let's work around this here (taking the pressure off fixing the MSYS2
runtime logic) simply by adjusting the `PATH` only _after_ letting
@actions/cache do its thing.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
github-actions bot pushed a commit to git-for-windows/build-extra that referenced this pull request Feb 22, 2023
When trying to call Cygwin (or for that matter, MSYS2) programs from Git

about a "cygheap"](git-for-windows/git#4255)

[now](git-for-windows/msys2-runtime#48)

allowing basic interactions. While it is still not possible for, say,

_is_ now possible for Cygwin's `zstd.exe` in conjuction with Git for

Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
dscho added a commit to dscho/msys2-runtime that referenced this pull request May 12, 2023
…ys2-runtime-versions-from-sharing-cygheaps

Avoid sharing cygheaps across Cygwin versions
dscho added a commit to dscho/msys2-runtime that referenced this pull request Jun 16, 2023
…ys2-runtime-versions-from-sharing-cygheaps

Avoid sharing cygheaps across Cygwin versions
dscho added a commit to dscho/msys2-runtime that referenced this pull request Jun 19, 2023
…ys2-runtime-versions-from-sharing-cygheaps

Avoid sharing cygheaps across Cygwin versions
dscho added a commit to dscho/msys2-runtime that referenced this pull request Sep 6, 2023
…ys2-runtime-versions-from-sharing-cygheaps

Avoid sharing cygheaps across Cygwin versions
dscho added a commit to dscho/msys2-runtime that referenced this pull request Nov 29, 2023
…ys2-runtime-versions-from-sharing-cygheaps

Avoid sharing cygheaps across Cygwin versions
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.

MinGit can be incompatible with Git for Windows installation of the same bitness
2 participants