{"thread":{"id":"60448","subject":"[PATCH] tests: handle \"funny\" exit code 127 produced by MSVC-compiled exes","startedAt":"2023-10-30T15:45:36Z","lastAt":"2023-11-01T20:18:14Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484117","messageId":"pull.1604.git.1698680732691.gitgitgadget@gmail.com","threadId":"60448","inReplyTo":null,"subject":"[PATCH] tests: handle \"funny\" exit code 127 produced by MSVC-compiled exes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-30T15:45:32Z","receivedAt":"2023-10-30T15:45:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe exit code 127 is well-documented to mean: command not found.\n\nUnfortunately, it is also used as fall-back in Cygwin's\n`pinfo::status_exit()` method (which maps things like Windows'\n`STATUS_ACCESS_VIOLATION` to `128 | SIGSEGV`).\n\nThis is particularly unfortunate because there is no explicit mapping\nfor `STATUS_STACK_OVERFLOW`. Meaning: when MSVC-compiled executables\nproduce a stack overflow the exit code in the Cygwin Bash will be 127.\nConsequently, the same will be true for the MSYS2 Bash that is used by\nGit for Windows.\n\nNow, `jk/tree-name-and-depth-limit` introduces a pair of test cases that\nexpect a command that produces a stack overflow to fail, which it\ntypically does with exit code 139 (which means SIGSEGV).\n\nBut since MSVC-compiled `git.exe` exits with `STATUS_STACK_OVERFLOW`\nwhich the MSYS2 runtime maps to 127, and since 127 is taken to mean\n\"command not found\" by `test_must_fail`, even though everything works as\nplanned the two new test cases fail when run in `win+VS test`.\n\nLet's work around this by:\n\n1) recording which C compiler was used, and\n\n2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a\n   regular failure.\n\nThere is a slight downside of this approach in that a real missing\ncommand could be mistaken for a failure. However, this would be caught\non other platforms, and besides, we use `test_must_fail` only for `git`\nand `scalar` anymore, and we can be pretty certain that both are there.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Fix t6700.[45] in win+VS test\n    \n    These two test cases have been failing for a while in Git for Windows'\n    shears/* branches. Took a good while to figure out, too.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1604\n\n contrib/buildsystems/CMakeLists.txt | 3 ++-\n t/test-lib-functions.sh             | 3 +++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 6b819e2fbdf..e164484be98 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -1057,7 +1057,8 @@ if(NOT PYTHON_TESTS)\n \tset(NO_PYTHON 1)\n endif()\n \n-file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"SHELL_PATH='${SHELL_PATH}'\\n\")\n+file(WRITE ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"CMAKE_C_COMPILER='${CMAKE_C_COMPILER}'\\n\")\n+file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"SHELL_PATH='${SHELL_PATH}'\\n\")\n file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"TEST_SHELL_PATH='${TEST_SHELL_PATH}'\\n\")\n file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"PERL_PATH='${PERL_PATH}'\\n\")\n file(APPEND ${CMAKE_BINARY_DIR}/GIT-BUILD-OPTIONS \"DIFF='${DIFF}'\\n\")\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2f8868caa17..ee19c748973 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1112,6 +1112,9 @@ test_must_fail () {\n \t\treturn 1\n \telif test $exit_code -eq 127\n \tthen\n+\t\t# Work-around for MSVC-compiled executables\n+\t\tcase \"$CMAKE_C_COMPILER\" in *MSVC*) return 0;; esac\n+\n \t\techo >&4 \"test_must_fail: command not found: $*\"\n \t\treturn 1\n \telif test $exit_code -eq 126\n\nbase-commit: 3130c155df9a65ebccf128b4af5a19af49532580\n-- \ngitgitgadget\n"},{"id":"484135","messageId":"20231030175656.GA855403@coredump.intra.peff.net","threadId":"60448","inReplyTo":"pull.1604.git.1698680732691.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: handle \"funny\" exit code 127 produced by MSVC-compiled exes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-30T17:56:56Z","receivedAt":"2023-10-30T17:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 30, 2023 at 03:45:32PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Now, `jk/tree-name-and-depth-limit` introduces a pair of test cases that\n> expect a command that produces a stack overflow to fail, which it\n> typically does with exit code 139 (which means SIGSEGV).\n\nI think you're misinterpreting the purpose of the tests from that\nseries; they're not intended to segfault. Quoting from t6700:\n\n  # We'll test against two depths here: a small one that will let us check the\n  # behavior of the config setting easily, and a large one that should be\n  # forbidden by default. Testing the default depth will let us know whether our\n  # default is enough to prevent segfaults on systems that run the tests.\n\nSo for the \"big tree\" tests in that file, we are looking for a\ncontrolled failure rather than a segfault. And indeed, the end of that\nseries already lowered the default to accommodate the msys windows\nbuild; see the discussion in 4d5693ba05 (lower core.maxTreeDepth default\nto 2048, 2023-08-31).\n\nSo I think the test is working as designed here: it is showing us that\nthe default value is not sufficient to protect MSVC builds from running\nout of stack space. There are a few options there:\n\n  1. We can lower the default everywhere.\n\n  2. We can lower it just for MSVC builds.\n\n  3. We can accept the situation and skip the tests for that build.\n\nThere's a bit more discussion in the commit I referenced above.\n\n> Let's work around this by:\n> \n> 1) recording which C compiler was used, and\n> \n> 2) adding an MSVC-only exception to `test_must_fail` to treat 127 as a\n>    regular failure.\n> \n> There is a slight downside of this approach in that a real missing\n> command could be mistaken for a failure. However, this would be caught\n> on other platforms, and besides, we use `test_must_fail` only for `git`\n> and `scalar` anymore, and we can be pretty certain that both are there.\n\nI think there is another much worse downside to your patch: we will stop\nnoticing when MSVC builds segfault in the tests. The purpose of\ntest_must_fail is to allow controlled and expected failure returns from\nthe command, but still report on unexpected situations (signal death,\ncommand not found, and so on).\n\n-Peff\n"},{"id":"484302","messageId":"pull.1604.v2.git.1698843810814.gitgitgadget@gmail.com","threadId":"60448","inReplyTo":"pull.1604.git.1698680732691.gitgitgadget@gmail.com","subject":"[PATCH v2] max_tree_depth: lower it for MSVC to avoid stack overflows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-01T13:03:30Z","receivedAt":"2023-11-01T13:03:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThere seems to be some internal stack overflow detection in MSVC's\n`malloc()` machinery that seems to be independent of the `stack reserve`\nand `heap reserve` sizes specified in the executable (editable via\n`EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`).\n\nIn the newly test cases added by `jk/tree-name-and-depth-limit`, this\nstack overflow detection is unfortunately triggered before Git can print\nout the error message about too-deep trees and exit gracefully. Instead,\nit exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric\nvalue -1073741571, something the MSYS2 runtime we sadly need to use to\nrun Git's test suite cannot handle and which it internally maps to the\nexit code 127. Git's test suite, in turn, mistakes this to mean that the\ncommand was not found, and fails both test cases.\n\nHere is an example stack trace from an example run:\n\n    [0x0]   ntdll!RtlpAllocateHeap+0x31   0x4212603f50   0x7ff9d6d4cd49\n    [0x1]   ntdll!RtlpAllocateHeapInternal+0x6c9   0x42126041b0   0x7ff9d6e14512\n    [0x2]   ntdll!RtlDebugAllocateHeap+0x102   0x42126042b0   0x7ff9d6dcd8b0\n    [0x3]   ntdll!RtlpAllocateHeap+0x7ec70   0x4212604350   0x7ff9d6d4cd49\n    [0x4]   ntdll!RtlpAllocateHeapInternal+0x6c9   0x42126045b0   0x7ff9596ed480\n    [0x5]   ucrtbased!heap_alloc_dbg_internal+0x210   0x42126046b0   0x7ff9596ed20d\n    [0x6]   ucrtbased!heap_alloc_dbg+0x4d   0x4212604750   0x7ff9596f037f\n    [0x7]   ucrtbased!_malloc_dbg+0x2f   0x42126047a0   0x7ff9596f0dee\n    [0x8]   ucrtbased!malloc+0x1e   0x42126047d0   0x7ff730fcc1ef\n    [0x9]   git!do_xmalloc+0x2f   0x4212604800   0x7ff730fcc2b9\n    [0xa]   git!do_xmallocz+0x59   0x4212604840   0x7ff730fca779\n    [0xb]   git!xmallocz_gently+0x19   0x4212604880   0x7ff7311b0883\n    [0xc]   git!unpack_compressed_entry+0x43   0x42126048b0   0x7ff7311ac9a4\n    [0xd]   git!unpack_entry+0x554   0x42126049a0   0x7ff7311b0628\n    [0xe]   git!cache_or_unpack_entry+0x58   0x4212605250   0x7ff7311ad3a8\n    [0xf]   git!packed_object_info+0x98   0x42126052a0   0x7ff7310a92da\n    [0x10]   git!do_oid_object_info_extended+0x3fa   0x42126053b0   0x7ff7310a44e7\n    [0x11]   git!oid_object_info_extended+0x37   0x4212605460   0x7ff7310a38ba\n    [0x12]   git!repo_read_object_file+0x9a   0x42126054a0   0x7ff7310a6147\n    [0x13]   git!read_object_with_reference+0x97   0x4212605560   0x7ff7310b4656\n    [0x14]   git!fill_tree_descriptor+0x66   0x4212605620   0x7ff7310dc0a5\n    [0x15]   git!traverse_trees_recursive+0x3f5   0x4212605680   0x7ff7310dd831\n    [0x16]   git!unpack_callback+0x441   0x4212605790   0x7ff7310b4c95\n    [0x17]   git!traverse_trees+0x5d5   0x42126058a0   0x7ff7310dc0f2\n    [0x18]   git!traverse_trees_recursive+0x442   0x4212605980   0x7ff7310dd831\n    [0x19]   git!unpack_callback+0x441   0x4212605a90   0x7ff7310b4c95\n    [0x1a]   git!traverse_trees+0x5d5   0x4212605ba0   0x7ff7310dc0f2\n    [0x1b]   git!traverse_trees_recursive+0x442   0x4212605c80   0x7ff7310dd831\n    [0x1c]   git!unpack_callback+0x441   0x4212605d90   0x7ff7310b4c95\n    [0x1d]   git!traverse_trees+0x5d5   0x4212605ea0   0x7ff7310dc0f2\n    [0x1e]   git!traverse_trees_recursive+0x442   0x4212605f80   0x7ff7310dd831\n    [0x1f]   git!unpack_callback+0x441   0x4212606090   0x7ff7310b4c95\n    [0x20]   git!traverse_trees+0x5d5   0x42126061a0   0x7ff7310dc0f2\n    [0x21]   git!traverse_trees_recursive+0x442   0x4212606280   0x7ff7310dd831\n    [...]\n    [0xfad]   git!cmd_main+0x2a2   0x42126ff740   0x7ff730fb6345\n    [0xfae]   git!main+0xe5   0x42126ff7c0   0x7ff730fbff93\n    [0xfaf]   git!wmain+0x2a3   0x42126ff830   0x7ff731318859\n    [0xfb0]   git!invoke_main+0x39   0x42126ff8a0   0x7ff7313186fe\n    [0xfb1]   git!__scrt_common_main_seh+0x12e   0x42126ff8f0   0x7ff7313185be\n    [0xfb2]   git!__scrt_common_main+0xe   0x42126ff960   0x7ff7313188ee\n    [0xfb3]   git!wmainCRTStartup+0xe   0x42126ff990   0x7ff9d5ed257d\n    [0xfb4]   KERNEL32!BaseThreadInitThunk+0x1d   0x42126ff9c0   0x7ff9d6d6aa78\n    [0xfb5]   ntdll!RtlUserThreadStart+0x28   0x42126ff9f0   0x0\n\nI verified manually that `traverse_trees_cur_depth` was 562 when that\nhappened, which is far below the 2048 that were already accepted into\nGit as a hard limit.\n\nDespite many attempts to figure out which of the internals trigger this\n`STATUS_STACK_OVERFLOW` and how to maybe increase certain sizes to avoid\nrunning into this issue and let Git behave the same way as under Linux,\nI failed to find any build-time/runtime knob we could turn to that\neffect.\n\nNote: even switching to using a different allocator (I used mimalloc\nbecause that's what Git for Windows uses for its GCC builds) does not\nhelp, as the zlib code used to unpack compressed pack entries _still_\nuses the regular `malloc()`. And runs into the same issue.\n\nNote also: switching to using a different allocator _also_ for zlib code\nseems _also_ not to help. I tried that, and it still exited with\n`STATUS_STACK_OVERFLOW` that seems to have been triggered by a\n`mi_assert_internal()`, i.e. an internal assertion of mimalloc...\n\nSo the best bet to work around this for now seems to just lower the\nmaximum allowed tree depth _even further_ for MSVC builds.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Fix t6700.[45] in win+VS test\n    \n    These two test cases have been failing for a while in Git for Windows'\n    shears/* branches. Took a good while to figure out, too.\n    \n    Changes since v1:\n    \n     * Rewrite the patch to instead lower the max_allowed_tree_depth\n       threshold even further for MSVC, side-stepping the stack overflow.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1604\n\nRange-diff vs v1:\n\n 1:  0e6e53bd824 < -:  ----------- tests: handle \"funny\" exit code 127 produced by MSVC-compiled exes\n -:  ----------- > 1:  5f738a78eb1 max_tree_depth: lower it for MSVC to avoid stack overflows\n\n\n environment.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/environment.c b/environment.c\nindex bb3c2a96a33..9e37bf58c0c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -81,7 +81,20 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n-int max_allowed_tree_depth = 2048;\n+int max_allowed_tree_depth =\n+#ifdef _MSC_VER\n+\t/*\n+\t * When traversing into too-deep trees, Visual C-compiled Git seems to\n+\t * run into some internal stack overflow detection in the\n+\t * `RtlpAllocateHeap()` function that is called from within\n+\t * `git_inflate_init()`'s call tree. The following value seems to be\n+\t * low enough to avoid that by letting Git exit with an error before\n+\t * the stack overflow can occur.\n+\t */\n+\t512;\n+#else\n+\t2048;\n+#endif\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\n\nbase-commit: 3130c155df9a65ebccf128b4af5a19af49532580\n-- \ngitgitgadget\n"},{"id":"484314","messageId":"20231101201810.GA1419081@coredump.intra.peff.net","threadId":"60448","inReplyTo":"pull.1604.v2.git.1698843810814.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] max_tree_depth: lower it for MSVC to avoid stack overflows","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-01T20:18:10Z","receivedAt":"2023-11-01T20:18:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 01, 2023 at 01:03:30PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> So the best bet to work around this for now seems to just lower the\n> maximum allowed tree depth _even further_ for MSVC builds.\n\nThanks for rewriting this. The resulting patch looks good to me.\n\nJust a few small thoughts:\n\n> There seems to be some internal stack overflow detection in MSVC's\n> `malloc()` machinery that seems to be independent of the `stack reserve`\n> and `heap reserve` sizes specified in the executable (editable via\n> `EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`).\n\nYikes, I'm sure that paragraph sums up a painful debugging journey. :)\n\n> In the newly test cases added by `jk/tree-name-and-depth-limit`, this\n> stack overflow detection is unfortunately triggered before Git can print\n> out the error message about too-deep trees and exit gracefully. Instead,\n> it exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric\n> value -1073741571, something the MSYS2 runtime we sadly need to use to\n> run Git's test suite cannot handle and which it internally maps to the\n> exit code 127. Git's test suite, in turn, mistakes this to mean that the\n> command was not found, and fails both test cases.\n\nI think this detail is OK, but the bit about mistaking 127 is IMHO kind\nof irrelevant to the purpose of the patch. The whole point of those\ntests is that they would trigger in a segfault to alert us that the\ndefault depth limit was too high, and they did. So it was in fact lucky\nthat even though the segfault was munged into 127, our test_must_fail\nstill noticed it.\n\n> Note: even switching to using a different allocator (I used mimalloc\n> because that's what Git for Windows uses for its GCC builds) does not\n> help, as the zlib code used to unpack compressed pack entries _still_\n> uses the regular `malloc()`. And runs into the same issue.\n\nI didn't think zlib ever malloc'd, since we feed it streaming data (and\nit will return and ask us to flush if the output buffer is full). But I\nadmit I haven't dug too far into it, and it sounds like you may have.\n\nWhat I was wondering specifically is whether you're actually hitting the\nraw malloc() (as opposed to xmalloc) calls in diff-delta.c (which would\ndepend on how you've set up the different allocator).\n\nEither way, changing anything there is well outside the scope of your\npatch. I've just always wondered if those raw malloc() calls might cause\nheadaches, and whether this might be a concrete example of such.\n\n-Peff\n"}]}