Use-after-free (?) in overwriteIndexFromFile

[geofft]

This is documented in more detail internally at http://jira/SDLC-37809, and I'll try to extract a public test case, but after switching to Node 18, we're seeing what appears to be memory corruption in libgit2, which as best as I can tell is because overwriteIndexFromFile in node/lib/util/git_util.js opens up a NodeGit Index object (newIndex), stores the pointers to the IndexEntry objects it contains, and then allows the Index object to be freed. This mostly manifests as libgit2 complaining the file mode is zero, sometimes as libgit2 complaining a path is invalid, and at least once as a segfault in strcmp.

I suspect this isn't happening in the public repo's CI because we're not triggering the same memory-allocation patterns - the internal repo has about 15K submodules and is also using a pre-commit hook, both of which appear to be necessary.

Modifying the code to intentionally leak newIndex seems to avoid the memory corruption. I'm not actually sure how to properly do this with the exposed nodegit API, since there doesn't appear to be a deep-copy function on IndexEntry.

[geofft]

OK, I can reproduce this with nothing more than my clone of git-meta itself:

geofft@vm:~/git-meta/node$ node
Welcome to Node.js v16.14.2.
Type ".help" for more information.
> const NodeGit = require("nodegit")
undefined
> const git_util = require("./lib/util/git_util.js")
undefined
> const index = await NodeGit.Index.open("../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
undefined
> await git_util.overwriteIndexFromFile(index, "../.git/index")
Uncaught:
[Error: invalid entry mode] { errno: -1, errorFunction: 'Index.add' }

[geofft]

I still get this with the latest node-git release (0.28.0-alpha.20), which is a little frustrating - I was hoping it was something like nodegit/nodegit#1833 but apparently not (or maybe it secretly is, and that fix wasn't quite sufficient).

Some other notes:

• If you add some debug logging in the for loop in overwriteIndexFromFile, it's clear you're getting corruption on the reads from the index object, i.e., a deep-copy wouldn't help us because the data is already corrupt. That is, I think my original diagnosis is slightly incorrect and this isn't precisely about reusing IndexEntry objects.
• If you stick a global.gc() basically anywhere in overwriteIndexFromFile other than the inside of the for loop (and call git-meta with node --expose-gc), the corruption seems to go away. Also, some gdb logging indicates that an Index object is being freed during the for loop in the cases where there's corruption. So between those two (and the intentional-leak workaround) it seems pretty clear that this is, nonetheless, a UAF of some sort (and it's definitely in this function).
• Besides an invalid mode, the most common other complaint is an invalid path, from when it sticks a NUL after a slash. You can get other corruptions in the path name, but they don't cause libgit2 to error out. (This is pretty concerning to me!)