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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/01/2014 10:16 PM, Jeff King wrote:
> 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. :-/

Good point.  Yes, I agree that this is a problem in the existing code
and that it wasn't improved by my work.

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

I think that using the owner field to avoid this problem is a bit
indirect, so I will soon submit a fix that involves adding a flag to
lock_file objects indicating whether the filename field currently
contains the name of a file that needs to be deleted.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/