• Jeff King's avatar
    reopen_tempfile(): truncate opened file · 6c003d6f
    Jeff King authored
    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
    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's avatarLuc Van Oostenryck <luc.vanoostenryck@gmail.com>
    Signed-off-by: 's avatarJeff King <peff@peff.net>
    Signed-off-by: 's avatarJunio C Hamano <gitster@pobox.com>
tempfile.c 8.32 KB