| Message ID | pull.1604.v2.git.1698843810814.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b64d78ad02ca14ab4d658a1055f43f8edf5e3990 |
| Headers | show |
| Series | [v2] max_tree_depth: lower it for MSVC to avoid stack overflows | expand |
On Wed, Nov 01, 2023 at 01:03:30PM +0000, Johannes Schindelin via GitGitGadget wrote: > So the best bet to work around this for now seems to just lower the > maximum allowed tree depth _even further_ for MSVC builds. Thanks for rewriting this. The resulting patch looks good to me. Just a few small thoughts: > There seems to be some internal stack overflow detection in MSVC's > `malloc()` machinery that seems to be independent of the `stack reserve` > and `heap reserve` sizes specified in the executable (editable via > `EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`). Yikes, I'm sure that paragraph sums up a painful debugging journey. :) > In the newly test cases added by `jk/tree-name-and-depth-limit`, this > stack overflow detection is unfortunately triggered before Git can print > out the error message about too-deep trees and exit gracefully. Instead, > it exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric > value -1073741571, something the MSYS2 runtime we sadly need to use to > run Git's test suite cannot handle and which it internally maps to the > exit code 127. Git's test suite, in turn, mistakes this to mean that the > command was not found, and fails both test cases. I think this detail is OK, but the bit about mistaking 127 is IMHO kind of irrelevant to the purpose of the patch. The whole point of those tests is that they would trigger in a segfault to alert us that the default depth limit was too high, and they did. So it was in fact lucky that even though the segfault was munged into 127, our test_must_fail still noticed it. > Note: even switching to using a different allocator (I used mimalloc > because that's what Git for Windows uses for its GCC builds) does not > help, as the zlib code used to unpack compressed pack entries _still_ > uses the regular `malloc()`. And runs into the same issue. I didn't think zlib ever malloc'd, since we feed it streaming data (and it will return and ask us to flush if the output buffer is full). But I admit I haven't dug too far into it, and it sounds like you may have. What I was wondering specifically is whether you're actually hitting the raw malloc() (as opposed to xmalloc) calls in diff-delta.c (which would depend on how you've set up the different allocator). Either way, changing anything there is well outside the scope of your patch. I've just always wondered if those raw malloc() calls might cause headaches, and whether this might be a concrete example of such. -Peff
diff --git a/environment.c b/environment.c index bb3c2a96a33..9e37bf58c0c 100644 --- a/environment.c +++ b/environment.c @@ -81,7 +81,20 @@ int merge_log_config = -1; int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */ unsigned long pack_size_limit_cfg; enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET; -int max_allowed_tree_depth = 2048; +int max_allowed_tree_depth = +#ifdef _MSC_VER + /* + * When traversing into too-deep trees, Visual C-compiled Git seems to + * run into some internal stack overflow detection in the + * `RtlpAllocateHeap()` function that is called from within + * `git_inflate_init()`'s call tree. The following value seems to be + * low enough to avoid that by letting Git exit with an error before + * the stack overflow can occur. + */ + 512; +#else + 2048; +#endif #ifndef PROTECT_HFS_DEFAULT #define PROTECT_HFS_DEFAULT 0