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

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 06:15 UTC
Message-ID: <20171002061529.xaiqccwjrthc74gf@sigill.intra.peff.net>
In-Reply-To: <6d36f0a8e3c7b81e8c815782fb67fc69f86f2837.1506862824.git.martin.agren@gmail.com>

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

Show 6 quoted lines

> 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. :(

Show 10 quoted lines

> 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