Re: [PATCH 01/11] sha1_file: do not leak `lock_file`

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 05:26 UTC
Message-ID: <20171002052659.hgzkhz4sgez3mkgz@sigill.intra.peff.net>
In-Reply-To: <9725917d9854a54e4daeda70940c3bb0ff5d9201.1506862824.git.martin.agren@gmail.com>

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

Show 8 quoted lines

> There is no longer any need to allocate and leak a `struct lock_file`.
> Initialize it on the stack instead.
> 
> Instead of setting `lock = NULL` to signal that we have already rolled
> back, and that we should not do any more work, check with
> `is_lock_file_locked()`. Since we take the lock with
> `LOCK_DIE_ON_ERROR`, it evaluates to false exactly when we have called
> `rollback_lock_file()`.

This looks correct (and is a good direction, as it drops a leak).

The original code is actually a bit confusing/unsafe, as we set the "found" flag early and rollback here:

Show 10 quoted lines

> @@ -481,17 +481,15 @@ void add_to_alternates_file(const char *reference)
>  		strbuf_release(&line);
>  		fclose(in);
>  
> -		if (found) {
> -			rollback_lock_file(lock);
> -			lock = NULL;
> -		}
> +		if (found)
> +			rollback_lock_file(&lock);

and that leaves the "out" filehandle dangling. It's correct because of the later "do we still have the lock" check:

> -	if (lock) {
> +	if (is_lock_file_locked(&lock)) {
>  		fprintf_or_die(out, "%s\n", reference);

but the two spots must remain in sync. If I were writing it from scratch I might have bumped "found" to the scope of the whole function, and then replaced this final "do we still have the lock" with:

  if (found)
	rollback_lock_file(&lock);
  else {
	fprintf_or_die(out, "%s\n", reference);
	if (commit_lock_file(&lock))
	...etc...
  }

I don't know if it's worth changing now or not.

-Peff