patch, 4 partscommit-graph: handle file descriptor exhaustion

Hi,

This is another series to handle an issue we started seeing at GitHub on repositories that have too many commit-graph layers.

This issue evaded me for several days, but the gist is that having too many descriptors open caused us to fail 'git_mkstemp_mode', which in turn left some values in 'ctx->commit_graph_filenames_after' uninitialized. This in turn caused a general protection fault when trying to free these uninitialized entries. Frustratingly, it causes stray 'commit-graph-chain.lock' files to stick around even after the process has gone away, causing all subsequent commit-graph write operations to fail.

The first two patches are semi-related cleanups and other preparation. The third patch fixes the main issue described here. Peff contributed the fourth patch which teaches the commit-graph machinery to be more willing to close file descriptors.

These patches are very much owed to the help that Stolee and Peff provided while writing them and for their willingness to let me bounce some ideas off of them.

Thanks in advance for your review.

Jeff King (1):
  commit-graph: close descriptors after mmap

Taylor Blau (3):
  commit-graph.c: don't use discarded graph_name in error
  t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to tests
  commit-graph.c: gracefully handle file descriptor exhaustion

 commit-graph.c                | 21 ++++++++-------------
 commit-graph.h                |  3 +--
 fuzz-commit-graph.c           |  5 ++---
 t/t1400-update-ref.sh         |  9 ---------
 t/t5324-split-commit-graph.sh | 13 +++++++++++++
 t/test-lib.sh                 |  9 +++++++++
 6 files changed, 33 insertions(+), 27 deletions(-)

-- 2.26.0.113.ge9739cdccc

When writing a commit-graph layer, we do so in a temporary file which is renamed into place. If we fail to create a temporary file, for e.g., because we have too many open files, then 'git_mkstemp_mode' sets the pattern to the empty string, in which case we get an error something along the lines of:

  error: unable to create ''

It's not useful to show the pattern here at all, since we (1) know the pattern is well-formed, and (2) would have already shown the dirname when trying to create the leading directories. So, replace this error with something friendlier.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In t1400 the prerequisite 'ULIMIT_FILE_DESCRIPTORS' is defined and used to effectively guard the helper function 'run_with_limited_open_files' from being used on systems that do not satisfy this prerequisite.

In the subsequent patch, we will introduce another test outside of t1400 that would benefit from using this prerequisite. So, move it to 'test-lib.sh' instead so that it can be used by multiple tests.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t1400-update-ref.sh | 9 ---------
 t/test-lib.sh         | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

From: Jeff King <peff@peff.net>

We don't ever refer to the descriptor after mmap-ing it. And keeping it open means we can run out of descriptors in degenerate cases (e.g., thousands of split chain files). Let's close it as soon as possible.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c      | 15 +++++----------
 commit-graph.h      |  3 +--
 fuzz-commit-graph.c |  5 ++---
 3 files changed, 8 insertions(+), 15 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Yikes.  

Sorry, I should have looked at the use of mmap in this topioc more carefully when we queued the series. It is an easy mistake to make by anybody new to the API to leave it open while the region is in use.

With this fix, with or without the other topics still in flight, I do not think no code touches graph_fd. Should we remove the graph_fd field from the structure as well?

 commit-graph.h | 2 --
 1 file changed, 2 deletions(-)

On Thu, Apr 23, 2020 at 03:04:19PM -0700, Junio C Hamano wrote:

Oops, I thought I did.

Yes, this should definitely be squashed in.

-Peff

On 4/23/2020 6:04 PM, Junio C Hamano wrote:

You are right. I was new when first contributing the commit-graph. It was also easier to miss because we only had one commit-graph open at the time. Adding in the incremental file format led to multiple file descriptors being open.

However, this idea of closing a descriptor after an mmap is new to me. So I thought about other situations where I made the same mistake. Please see the patch below.

I agree that this should be done.

Thanks, -Stolee

-->8--

From: Derrick Stolee <dstolee@microsoft.com>
Date: Fri, 24 Apr 2020 13:11:13 +0000
Subject: [PATCH] multi-pack-index: close file descriptor after mmap

We recently discovered that the commit-graph was not closing its file descriptor after memory-mapping the file contents. After this mmap() succeeds, there is no need to keep the file descriptor open. In fact, there is signficant reason to close it so we do not run out of descriptors.

This is entirely my fault for not knowing that we can have an open mmap while also closing the descriptor. Some could blame this on being a new contributor when the series was first submitted, but even years later this is still new information to me.

That made me realize that I used the same pattern when opening a multi-pack-index. Since this file is not (yet) incremental, there will be at most one descriptor open for this reason. It is still worth fixing, especially if we extend the format to be incremental like the commit-graph.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 midx.c | 4 +---
 midx.h | 2 --
 2 files changed, 1 insertion(+), 5 deletions(-)

On Fri, Apr 24, 2020 at 09:17:16AM -0400, Derrick Stolee wrote:

It's new to me, too :). If I had known it beforehand, then I would have written the fourth patch here myself. But, I didn't, so I am grateful to Peff for teaching me something new here.

For what it's worth, this didn't apply quite right with 'git am -3 -c', since it didn't seem to recognize that this was your scissors line. If I edit your mail myself by replacing this line with '-- >8 --', then 'git am' applies it just fine.

Right, we want to close as soon as we have mmaped the file.

...and not down here. Thanks.

:). Even better!

This looks great to me, and thanks for being proactive about the fix.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Derrick Stolee <stolee@gmail.com> writes:

The above is sufficient a justification. Let's leave the remaining two paragraphs under three-dashes.

On 4/24/2020 4:02 PM, Junio C Hamano wrote:

Works for me! I also thought there were too many first-person pronouns, but erred on the side of reporting "how did we figure out this was an issue?" in the message.

-Stolee

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(-)

On Thu, Apr 23 2020, Taylor Blau wrote:

This test started failing for me, and now I can't reproduce it anymore, but I reproduced enough of it to think it was odd that it hadn't failed before.

I.e. here we do an "ulimit -n 32" and then run a command, which makes a lot of assumptions about how git is compiled, starts up etc, a lot of which are outside of our control and up to the OS. It's not 32 files we open, but 32 everything. When I could reproduce this it was failing on opening some libpcre file or other, so maybe I linked to one too many libraries.

The one other test that uses this pattern seems like it could be similarly affected, but I haven't had that fail: d415ad022d8 (update-ref: test handling large transactions properly, 2015-04-14)

Since I can't reproduce this anymore maybe I'm reporting a nothingburger. I just wonder if this can really work reliably in the general case, and whether a reliably version of this pattern doesn't need to be one/some of:

 1. Some GIT_TEST_* mode that sets the (unportable) ulimit itself in the
    code, after we reach some point. This is how e.g. web-based REPLs
    often work, load all your known libraries, forbid any file openings
    (or just N number) after that.

 2. Ditto, but have the GIT_TEST_* print to stderr if we reach some
    "checkpoint", have the test only run under limit N if we can reach
    that point (i.e. binary search or brute force to find the exact N
    limit).

 3. Maybe we can assume this would work reliably in cases of a really
    high limit of N, i.e. the d415ad022d8 test doesn't do this, but my
    understanding of it is that we're trying to guard against having all
    loose refs opened at once. So if we create e.g. 2k refs and operate
    on them we can set the limit to "1999".

    That's still assuming the same things about ulimit that make/made
    this test flaky, but we can probably safely assume that just getting
    to "git <something>" being invoked won't open >1k files, but maybe
    not >32.

On Thu, Jun 24, 2021 at 11:51:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

Yes, we could probably just set the "32" a bit higher. Something like "128" may be more conservative. I'd be hesitant to go as high as something like 1999; system defaults are often much lower than that anyway and may get rejected. We may have to adjust the tests to match the idea of what's "a lot of descriptors". (Likewise, the "ulimit -s" stuff has to make the same guess).

The prereq also tries running with the lower ulimit, but it only runs "true". Perhaps running something like "git version" would give us a more realistic idea of the baseline for running a git command (in which case the test would be auto-skipped if somehow 32 descriptors isn't enough on some system). That's not foolproof, though (it might take 31 to run "git version", but 33 to run "update-ref" or something).

I'm willing to let it lie unless you have a current problem. This is all known to be guesswork/heuristics, and the hope was that it simply wouldn't come up. If it's causing even occasional pain we might need to deal with it. But if it's an ephemeral thing that maybe went away, I'm content to stick my fingers back in my ears. :)

-Peff

...I should mention that this is based on my series in [1], since it's much easier to express the new test in t5324 with '--split=no-merge'.

Thanks, Taylor

[1]: https://lore.kernel.org/git/cover.1586836700.git.me@ttaylorr.com/