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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html