-
We provide a reopen_tempfile() function, which is in turn used by reopen_lockfile(). The idea is that a caller may want to rewrite the tempfile without letting go of the lock. And that's what our one caller does: after running add--interactive, "commit -p" will update the cache-tree extension of the index and write out the result, all while holding the lock. However, because we open the file with only the O_WRONLY flag, the existing index content is left in place, and we overwrite it starting at position 0. If the new index after updating the cache-tree is smaller than the original, those final bytes are not overwritten and remain in the file. This results in a corrupt index, since those cruft bytes are interpreted as part of the trailing hash (or even as an extension, if there are enough bytes). This bug actually pre-dates reopen_tempfile(); the original code from 9c4d6c02 (cache-tree: Write updated cache-tree after commit, 2014-07-13) has the same bug, and those lines were eventually refactored into the tempfile module. Nobody noticed until now for two reasons: - the bug can only be triggered in interactive mode ("commit -p" or "commit -i") - the size of the index must shrink after updating the cache-tree, which implies a non-trivial deletion. Notice that the included test actually has to create a 2-deep hierarchy. A single level is not enough to actually cause shrinkage. The fix is to truncate the file before writing out the second index. We can do that at the caller by using ftruncate(). But we shouldn't have to do that. There is no other place in Git where we want to open a file and overwrite bytes, making reopen_tempfile() a confusing and error-prone interface. Let's pass O_TRUNC there, which gives callers the same state they had after initially opening the file or lock. It's possible that we could later add a caller that wants something else (e.g., to open with O_APPEND). But this is the only caller we've had in the history of the codebase. Let's punt on doing anything more clever until another one comes along. Reported-by:
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Signed-off-by:
Jeff King <peff@peff.net> Signed-off-by:
Junio C Hamano <gitster@pobox.com>
6c003d6f
