sparse-index: fix crash in status #395
Conversation
Copy the `index_state->dir_hash` back to the real istate after expanding a sparse index. A crash was observed in `git status` during some hashmap lookups with corrupted hashmap entries. During an index expansion, new cache-entries are added to the `index_state->name_hash` and the `dir_hash` in a temporary `index_state` variable `full`. However, only the `name_hash` hashmap from this temp variable was copied back into the real `istate` variable. The original copy of the `dir_hash` was incorrectly preserved. If the table in the `full->dir_hash` hashmap were realloced, the stale version (in `istate`) would be corrupted. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
| @@ -295,6 +295,7 @@ void ensure_full_index(struct index_state *istate) | |||
|
|
|||
| /* Copy back into original index. */ | |||
| memcpy(&istate->name_hash, &full->name_hash, sizeof(full->name_hash)); | |||
| memcpy(&istate->dir_hash, &full->dir_hash, sizeof(full->dir_hash)); | |||
There was a problem hiding this comment.
Wow. Thanks for finding this missing memory copy. This applies to the version of sparse-index in upstream's master, so we should send it up as its own bugfix (independent of other enhancements).
There was a problem hiding this comment.
I wonder whether it would be worth running t1091, t1092, t7519 and t7817 through valgrind on Linux and macOS, which may have caught this?
There was a problem hiding this comment.
I tried to set up a GitHub workflow to do this: https://github.com/dscho/git/runs/2998707560?check_suite_focus=true
What a rabbit hole. Apparently, brew install valgrind fails because it says it requires Linux... but then I found an unofficial fork that builds on macOS, except it fails because of a missing syscall (I reported this).
On the Linux side, it's just really slow. I also suspect that it runs the tests over and over again because it is still running, at the time of writing it ran for over 45 minutes (granted, some 4-6 minutes of that is the build, not the test).
There was a problem hiding this comment.
Ah, so it took ~25 minutes to run the tests (in parallel, t1092 took this long on its own), and it ran things twice because linux-gcc runs the test suite twice, once with default options, and then with a ton of options overridden (such as testing SHA-256 instead of SHA-1). But it did finish eventually.
I'm not sure what to do about testing on macOS, though.
The reason we didn't identify the memory problem in #395 was because our manual testing is too simplistic: We never build within one sparse-checkout definition and then switch to another one. If we did that, then we might have noticed that `git sparse-checkout set` will leave the ignored files alone within those newly-sparse directories. This is somewhat unexpected from the user point of view: they say they don't want that directory anymore, but we are keeping all untracked files around! This only applies to ignored files, since we refuse to adjust the sparse-checkout definition over a modified or new (unignored) file. Leaving these ignored files where they are removes any chance that the sparse index can get its correct performance benefits. For now, this behavior change is limited to the sparse index, so users can disable it by disabling the sparse index. We will want to consult with upstream about this behavior before moving too far down this path. However, it might be a good idea to try this out on the experimental release. **Bonus:** There was a bug in `find_cache_entry()` that is now fixed.
This fixes a crash that was observed in
git statusduring some hashmap lookups with corrupted hashmap entries.