Re: [PATCH 07/22] lock_file(): always add lock_file object to lock_file_list

[Jeff King <peff@peff.net>]

On Tue, Apr 01, 2014 at 05:58:15PM +0200, Michael Haggerty wrote:

> diff --git a/lockfile.c b/lockfile.c
> index e679e4c..c989f6c 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
>  	 */
>  	static const size_t max_path_len = sizeof(lk->filename) - 5;
>  
> +	if (!lock_file_list) {
> +		/* One-time initialization */
> +		sigchain_push_common(remove_lock_file_on_signal);
> +		atexit(remove_lock_file);
> +	}
> +
> +	lk->owner = getpid();
> +	if (!lk->on_list) {
> +		/* Initialize *lk and add it to lock_file_list: */
> +		lk->fd = -1;
> +		lk->on_list = 1;
> +		lk->filename[0] = 0;
> +		lk->next = lock_file_list;
> +		lock_file_list = lk;
> +	}

Initializing here is good, since we might be interrupted by a signal at
any time. But what about during the locking procedure? We do:

    strcpy(lk->filename, path);
    if (!(flags & LOCK_NODEREF))
            resolve_symlink(lk->filename, max_path_len);
    strcat(lk->filename, ".lock");

So for a moment, lk->filename contains the name of the valuable file we
are locking.  If we get a signal at that moment, do we accidentally
delete it in remove_lock_file?

I think the answer is "no", because we check lk->owner before deleting,
which will not match our pid (it should generally be zero due to xcalloc
or static initialization, though perhaps we should clear it here).

But that makes me wonder about the case of a reused lock. It will have
lk->owner set from a previous invocation, and would potentially suffer
from this problem. In other words, I think the change you are
introducing does not have the problem, but the existing code does. :-/

I didn't reproduce it experimentally, though.  We should be able to just

    lk->owner = 0;

before the initial strcpy to fix it, I would think.

-Peff