Re: [PATCH] tmp-objdir: do not opendir() when handling a signal

On Mon, Sep 26, 2022 at 11:53:03PM +0000, John Cai via GitGitGadget wrote:

Yuck. Your analysis looks right, and certainly opendir() can't really work without allocating memory for the pointer-to-DIR.

It's not really "may not" here, is it? It will never get cleaned up on a signal now. I don't think remove_dir_recursively() will try to rmdir() in this case. But even if it did, we'll always have a "pack" subdirectory (minus the small race before we've created it).

That's unfortunate, but I don't think we really have a portable alternative. We can't keep an exact list of files to be deleted, because some of them will be created by sub-processes. We could perhaps exec a helper that does the deletion, but that seems like a race and portability nightmare. On Linux, we could probably use open() and getdents64() to traverse, but obviously that won't work everywhere. It _might_ be worth having some kind of compat/ wrapper here, to let supported systems do as good a job as they can. But it's not like there aren't already cases where we might leave the tmp-objdir directory around (say, SIGKILL), so this is really just extending the existing problem to more signals.

I was going to suggest we should do a better job of cleaning up these directories via git-gc. But it looks like b3cecf49ea (tmp-objdir: new API for creating temporary writable databases, 2021-12-06) changed the default name such that a regular git-gc should do so. So I think we're covered there.

The main case we care about is normal exit when index-pack or a hook sees an error, in which case we should still be cleaning up via the atexit() handler.

So I think your patch is going in the right direction, but...

We skip calling opendir() entirely, so "dir" will still be NULL. But we immediately start looking at errno, which will have some undefined value (based on some previous syscall).

If we set "errno" to "EACCES" in this case, then we'd at least hit the rmdir() below:

         if (!dir) {
                  if (errno == ENOENT)
                          return keep_toplevel ? -1 : 0;
                  else if (errno == EACCES && !keep_toplevel)
                          /*
                           * An empty dir could be removable even if it
                           * is unreadable:
                           */
                          return rmdir(path->buf);
                  else
                          return -1;
          }

but we know it won't really do anything for our proposed caller, since it will have files inside the directory that need to be removed before rmdir() can work.

Moreover, if you were to combine REMOVE_DIR_SIGNAL with REMOVE_DIR_KEEP_NESTED_GIT, I suspect that the call to resolve_gitlink_ref() would end up with similar deadlocks. Obviously nobody is proposing to do that, but it is a pitfall in the API.

So all of that makes me think we should not add a new flag here, but instead just avoid calling the function entirely from tmp_objdir_destroy_1().

But then we can observe that tmp_objdir_destroy_1() is basically doing nothing if on_signal is set. So there is really no point in setting up the signal handler at all. We should just set up the atexit() handler. I.e., something like: