No fchmd. was: Re: [PATCH 00/14] Add submodule test harness

From: Torsten Bögershausen <tboegi@web.de>
Date: Jul 10, 2014, 06:22 UTC
Message-ID: <53BE3127.8020805@web.de>
In-Reply-To: <xmqqion69ovj.fsf@gitster.dls.corp.google.com>

On 07/09/2014 11:57 PM, Junio C Hamano wrote:

Show 45 quoted lines

> Eric Wong <normalperson@yhbt.net> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>>> Johannes Sixt <j6t@kdbg.org> writes:
>>>> Am 08.07.2014 21:34, schrieb Jens Lehmann:
>>>>>> And Msysgit complains
>>>>>> error: fchmod on c:/xxxt/trash directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock failed: Function not implemented
>>>>> I'm not sure what this is about, seems to happen during the "cp -R" of
>>>>> the repo under .git/modules into the submodule.
>>>> No. It happens because fchmod() is not implemented in our Windows port.
>>>>
>>>> Please see my band-aid patch at
>>>> http://thread.gmane.org/gmane.comp.version-control.git/248154/focus=20266
>>>> The sub-thread ended inconclusive.
>>> We need to start somewhere, and a no-op fchmod() in your patch may
>>> be as a good place to start as anything.  At least we would then
>>> keep the old behaviour without introducing any new failure.
>> Right, this likely makes the most sense for single-user systems or
>> systesm without a *nix-like permission system.
>>
>>> An alternative might be to use chmod() after we are done writing to
>>> the config.lock in order to avoid the use of fchmod() altogether,
>>> which I think can replace the existing two callsites of fchmod().
>>> That approach might be a more expedient, but may turn out to be
>>> undesirable in the longer term.
>> In that case, we would need to open with mode=0600 to avoid a window
>> where the file may be world-readable with any data in it.
> Yes, of course.
>
> To elaborate what I was alluding to at the end of the message you
> are responding to a bit more, if we were to move this "grab perms
> from existing file (if there is any) and propagate to the new one"
> into the lockfile API,
>
>   - in hold_lock_file_for_update(), we would record the permission of
>     the original file, if any, to a new field in "struct lock_file";
>   - open with 0600 or tighter in lock_file(), and
>
>   - either before closing the file use fchmod() or after closing and
>     moving the file use chmod() to propagate the permission.
>
> If the original did not exist, we would pass 0666 to open as before
> in lock_file() and do not bother chmod/fchmod at the end.
>
> Or something like that, perhaps.

Isn't the whole problem starting here: in config.c:

     fd = hold_lock_file_for_update(lock, config_filename, 0);
In lockfile.c:
   /* This should return a meaningful errno on failure */
   int hold_lock_file_for_update(struct lock_file *lk, const char *path, 
int flags)
   {
       int fd = lock_file(lk, path, flags);
which leads to
   static int lock_file(struct lock_file *lk, const char *path, int flags)
     []
     lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);

There is no way to tell which permissions the new lockfile should have. That is somewhat unlucky.

On the other hand, shouldn't we call adjust_shared_perm(const char *path) from path.c on the config file?

And to all files which are fiddled through the lock_file API? In other words, the lockfile could be created with the restrictive permissions 600, and once the lockfile had been closed and renamed into the final name we apply adjust_shared_perm() on it ?

Or probably directly after close() ?

I think there are 2 different things missing here:

- Be able to specify permissions to hold_lock_file_for_update(),
    especially restrictive ones, like 600 and not 666.

- Adjust the permissions for "shared files" in a shared repo.
   This is probably needed for a shared repo, when the user itself
    has a umask which is too restrictive and adjust_shared_perm()
    must be run to widen the permissions.

Do I miss something ?