Re: [PATCH 10/11] read-cache: don't leave dangling pointer in `do_write_index()`

[Jeff King <peff@peff.net>]

On Sun, Oct 01, 2017 at 04:56:11PM +0200, Martin Ågren wrote:

> If `do_write_index(..., struct tempfile *, ...)` fails to close the
> temporary file, it deletes it. This resets the pointer to NULL, but only
> the pointer which is local to `do_write_index()`, not the pointer that
> the caller holds. If the caller ever dereferences its pointer, we have
> undefined behavior (most likely a crash). One of the two callers tries
> to delete the temporary file on error, so it will dereference it.

Hrm. I think this was introduced by my lockfile series, since before
that the memory would have remained valid (but with its "active" flag
unset, which is enough for delete_tempfile() to happily return early).

Part of my reason for switching delete_tempfile() to take a
pointer-to-pointer was so that we had to investigate each such call. But
obviously I failed to analyze this case correctly. :(

> We could change the function to take a `struct tempfile **` instead. But
> we have another problem here. The other caller, `write_locked_index()`,
> passes in `lock->tempfile`. So if we close the temporary file and reset
> `lock->tempfile` to NULL, we have effectively rolled back the lock. That
> caller is `write_locked_index()` and if it is used with the
> `CLOSE_LOCK`-file, it means the lock is being rolled back against the
> wishes of the caller. (`write_locked_index()` used to call
> `close_lockfile()`, which would have rolled back on error. Commit
> 83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05)
> changed to not rolling back.)

I'm not sure I follow here. It seems like write_locked_index() has three
outcomes:

  - if there was an error, the lock is rolled back

  - if we were successful and the caller asked for CLOSE_LOCK, we remain
    locked

  - if we were successful and the caller asked for COMMIT_LOCK, we
    commit the lock

It sounds like you are considering the first outcome a bug, but I think
it is intentional. Without that, every caller of write_locked_index()
would need to release the lock themselves. That's especially cumbersome
if they called with COMMIT_LOCK, as they otherwise can assume that
write_locked_index() has released the lock one way or another.

So I actually think that just switching to a "struct tempfile **" here
is a reasonable solution (I'd also be fine with doing this and then
having do_write_locked_index() rollback the lock itself on error).

Or am I missing something?

-Peff