Memory corruption in index handling

[geofft]

The following setup pretty reliably produces error messages that are indicative of memory corruption somewhere in Index or IndexEntry, I suspect related to a use-after-free of Index. Save the following file as test.mjs:

import NodeGit from "nodegit";

async function overwriteIndexFromFile(index, path) {
    const newIndex = await NodeGit.Index.open(path);
    await index.removeAll();
    for (const e of newIndex.entries()) {
        await index.add(e);
    }
}

const path = "testrepo/.git/index";
const index = await NodeGit.Index.open(path);
for (var i = 0; i < 100; i++) {
    await overwriteIndexFromFile(index, path);
}

and then set up a test repo with a fairly large index (it is possible, but much rarer, to hit it with a smaller index):

$ git init testrepo
$ cd testrepo
$ for i in {1..20000}; do touch file"$i"; done
$ git add .
$ git commit -m "Initial commit"
$ cd ..
$ yarn install nodegit
$ node test.mjs
node:internal/process/esm_loader:94
    internalBinding('errors').triggerUncaughtException(
                              ^

[Error: invalid path: ''] { errno: -1, errorFunction: 'Index.add' }

This is a minimized version of the overwriteIndexFromFile function in twosigma/git-meta. See twosigma/git-meta#882 for a few more of my notes if you're curious - notably, leaking newIndex (by appending it to a global array) appears to avoid the problem, as does running GC in some spots. I think the overwriteIndexFromFile function is validly written and doesn't do anything unusual, but If the answer is "you're holding it wrong," that's still helpful (though I'd still claim that it shouldn't be possible to cause memory corruption by holding it wrong :) ).

The specific errors vary depending on your luck, at least in the un-minimized version: it's usually an invalid path or an invalid mode. If you are careful to log the IndexEntry's contents you will notice other corruption in the filename (@ signs and other weirdness clobbering bytes from the original filename) that doesn't actually trigger a validation error in libgit2 and presumably goes ahead and constructs an erroneous index. And exactly one time, I got a segfault in strlen as called from v8::String::NewFromUtf8.

If there's anything I can provide or try out to be helpful for debugging this, let me know!

System information

• node version: v16.14.2 (also reproduced on v18.9.0; we have not seen this on 8.x, but it may just be luck)
• npm or yarn version: yarn 1.22.18 (also reproduced on 1.23.2)
• OS/version/architecture: Debian 11 x86_64 (also reproduced on Debian 10 x86_64)
• Applicable nodegit version: 0.27.0 (also reproduced on 0.25.1, 0.28.0-alpha.3, 0.28.0-alpha.4, and 0.28.0-alpha.20; haven't tried HEAD yet)

[geofft]

I'm also able to reproduce this in a source checkout of HEAD with both Debug and Release builds.

[geofft]

Running valgrind on the test case above reports a bunch of errors like this:

==1681== Invalid read of size 8
==1681==    at 0x13C5E21A: git_index_add (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13AF6BE6: GitIndex::AddWorker::Execute() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13A3682D: ThreadPool::RunEventQueue() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x4909EA6: start_thread (pthread_create.c:477)
==1681==    by 0x5456DEE: clone (clone.S:95)
==1681==  Address 0x1bc23a60 is 64 bytes inside a block of size 90 free'd
==1681==    at 0x47859AB: free (vg_replace_malloc.c:538)
==1681==    by 0x13C600BC: git_index_clear (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13C60189: git_index_free (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13B00F48: NodeGitWrapper<GitIndexTraits>::~NodeGitWrapper() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13AF0FC1: GitIndex::~GitIndex() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0xE623DE: v8::internal::GlobalHandles::InvokeFirstPassWeakCallbacks() (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xEBE42C: v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [clone .constprop.1307] (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xEBEEAF: v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xEBFFE6: v8::internal::Heap::FinalizeIncrementalMarkingIfComplete(v8::internal::GarbageCollectionReason) (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xEC3AFF: v8::internal::IncrementalMarkingJob::Task::RunInternal() (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xDB59DA: non-virtual thunk to v8::internal::CancelableTask::Run() (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==    by 0xB77523: node::PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<v8::Task, std::default_delete<v8::Task> >) (in /root/.nvm/versions/node/v16.14.2/bin/node)
==1681==  Block was alloc'd at
==1681==    at 0x4786B65: calloc (vg_replace_malloc.c:760)
==1681==    by 0x13CC77FC: stdalloc__calloc (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13C5CA45: index_entry_create.constprop.0 (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13C607AB: git_index_read (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13C613E5: git_index_open (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13AF0E77: GitIndex::OpenWorker::Execute() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x13A3682D: ThreadPool::RunEventQueue() (in /root/node-git-repro/node_modules/nodegit/build/Release/nodegit.node)
==1681==    by 0x4909EA6: start_thread (pthread_create.c:477)
==1681==    by 0x5456DEE: clone (clone.S:95)

The exact end of the stack trace in the invalid read changes a bit (sometimes there's another frame with strlen or git_path_isvalid) but the allocation and free stacks are the same each time.