[PATCH 3/4] commit-graph.c: gracefully handle file descriptor exhaustion
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Apr 23, 2020, 21:41 UTC
- Message-ID
- <2b8ee726690861405f41adede5582b96749e98c5.1587677671.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1587677671.git.me@ttaylorr.com>
When writing a layered commit-graph, the commit-graph machinery uses 'commit_graph_filenames_after' and 'commit_graph_hash_after' to keep track of the layers in the chain that we are in the process of writing.
When the number of commit-graph layers shrinks, we initialize all entries in the aforementioned arrays, because we know the structure of the new commit-graph chain immediately (since there are no new layers, there are no unknown hash values).
But when the number of commit-graph layers grows (i.e., that 'num_commit_graphs_after > num_commit_graphs_before'), then we leave some entries in the filenames and hashes arrays as uninitialized, because we will fill them in later as those values become available.
For instance, we rely on 'write_commit_graph_file's to store the filename and hash of the last layer in the new chain, which is the one that it is responsible for writing. But, it's possible that 'write_commit_graph_file' may fail, e.g., from file descriptor exhaustion. In this case it is possible that 'git_mkstemp_mode' will fail, and that function will return early *before* setting the values for the last commit-graph layer's filename and hash.
This causes a number of upleasant side-effects. For instance, trying to 'free()' each entry in 'ctx->commit_graph_filenames_after' (and similarly for the hashes array) causes us to 'free()' uninitialized memory, since the area is allocated with 'malloc()' and is therefore subject to contain garbage (which is left alone when 'write_commit_graph_file' returns early).
This can manifest in other issues, like a general protection fault, and/or leaving a stray 'commit-graph-chain.lock' around after the process dies. (The reasoning for this is still a mystery to me, since we'd otherwise usually expect the kernel to run tempfile.c's 'atexit()' handlers in the case of a normal death...)
To resolve this, initialize the memory with 'CALLOC_ARRAY' so that uninitialized entries are filled with zeros, and can thus be 'free()'d as a noop instead of causing a fault.
Helped-by: Jeff King <peff@peff.net> Helped-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit-graph.c | 4 ++-- t/t5324-split-commit-graph.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c index afde075962..b2d2fdfe3d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1602,8 +1602,8 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx) free(old_graph_name); } - ALLOC_ARRAY(ctx->commit_graph_filenames_after, ctx->num_commit_graphs_after); - ALLOC_ARRAY(ctx->commit_graph_hash_after, ctx->num_commit_graphs_after); + CALLOC_ARRAY(ctx->commit_graph_filenames_after, ctx->num_commit_graphs_after); + CALLOC_ARRAY(ctx->commit_graph_hash_after, ctx->num_commit_graphs_after); for (i = 0; i < ctx->num_commit_graphs_after && i < ctx->num_commit_graphs_before; i++) diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index e5d8d64170..0d1db31b0a 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -381,4 +381,17 @@ test_expect_success '--split=replace replaces the chain' ' graph_read_expect 2 ' +test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion' ' + git init ulimit && + ( + cd ulimit && + for i in $(test_seq 64) + do + test_commit $i && + test_might_fail run_with_limited_open_files git commit-graph write \ + --split=no-merge --reachable || return 1 + done + ) +' + test_done
-- 2.26.0.113.ge9739cdccc