Re: [PATCH v2] Improve the filemode trustability check

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2014, 17:55 UTC
Message-ID: <xmqq61e94uza.fsf@gitster.dls.corp.google.com>
In-Reply-To: <546DD52F.90302@web.de>

Torsten Bögershausen <tboegi@web.de> writes:

Show 16 quoted lines

> Some file systems do not support the executable bit:
> a) The user executable bit is always 0, e.g. VFAT mounted with -onoexec
> b) The user executable bit is always 1, e.g. cifs mounted with -ofile_mode=0755
> c) There are system where user executable bit is 1 even if it should be 0
>    like b), but the file mode can be maintained locally. chmod -x changes the
>    file mode from 0766 to 0666, until the file system is unmounted and
>    remounted and the file mode is 0766 again.
>    This been observed when a Windows machine with NTFS exports a share to
>    Mac OS X via smb or afp.
>
> Case a) and b) are handled by the current code.
> Case c) qualifies as "non trustable executable bit" and core.filemode
> should be false, but this is currently not done.
>
> Detect when ".git/config" has the user executable bit set after
> creat(".git/config", 0666) and set core.filemode to false.

Is this codepath reached _only_ and immediately after creat(0666) of the config file in the same process? The function has a local variable reinit, which is returned to the caller to help it decide if we are re-initializing an existing repository, so I suspect that the call to git_config_set() before this part of the code may not necessarily be creating the file [*1*].

I _think_ in the reinit case that makes us rewrite an existing config file, the mode bits are propagated to the new file we just wrote from the old one; checking st1.st_mode is therefore seeing not what create(0666) gave us but whatever random bits the user had on the original.

Which is probably not a useful source of information to gauge the characterisic of the filesystem, no?

[Footnote]

*1* We may have been asked to reinitialize and update an existing repository that was created with the same or older repository format number.

Show 22 quoted lines

> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> Changes since V1:
> - Improved commit msg (hopefully)
> - Simplified the patch
>  builtin/init-db.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index aab44d2..195a88b 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -252,7 +252,8 @@ static int create_default_files(const char *template_path)
>  	filemode = TEST_FILEMODE;
>  	if (TEST_FILEMODE && !lstat(path, &st1)) {
>  		struct stat st2;
> -		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
> +		filemode = (!(st1.st_mode & S_IXUSR) &&
> +				!chmod(path, st1.st_mode ^ S_IXUSR) &&
>  				!lstat(path, &st2) &&
>  				st1.st_mode != st2.st_mode &&
>  				!chmod(path, st1.st_mode));