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
