Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config

From: Torsten Bögershausen <tboegi@web.de>
Date: Nov 17, 2014, 09:08 UTC
Message-ID: <5469BAFA.7070709@web.de>
In-Reply-To: <CAPig+cQ6j-3_Ng8DVT3FYk8T6DippEbYDhQq5v3DTJhGgBhPDQ@mail.gmail.com>

On 11/17/2014 02:40 AM, Eric Sunshine wrote:

Show 43 quoted lines

> On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Since time immemorial, the test of whether to set "core.filemode" has
>> been done by trying to toggle the u+x bit on $GIT_DIR/config and then
>> testing whether the change "took". It is somewhat odd to use the
>> config file for this test, but whatever.
>>
>> The test code didn't set the u+x bit back to its original state
>> itself, instead relying on the subsequent call to git_config_set() to
>> re-write the config file with correct permissions.
>>
>> But ever since
>>
>>      daa22c6f8d config: preserve config file permissions on edits (2014-05-06)
>>
>> git_config_set() copies the permissions from the old config file to
>> the new one. This is a good change in and of itself, but it interacts
>> badly with create_default_files()'s sloppiness, causing "git init" to
>> leave the executable bit set on $GIT_DIR/config.
>>
>> So change create_default_files() to reset the permissions on
>> $GIT_DIR/config after its test.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> Should this patch include a test in t1300 to ensure that this bug does
> not resurface (and to prove that this patch indeed fixes it)?
>
>>   builtin/init-db.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 56f85e2..4c8021d 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -255,6 +255,7 @@ static int create_default_files(const char *template_path)
>>                  filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>>                                  !lstat(path, &st2) &&
>>                                  st1.st_mode != st2.st_mode);
>> +               filemode &= !chmod(path, st1.st_mode);
>>          }
>>          git_config_set("core.filemode", filemode ? "true" : "false");
>>
>> --

Sorry for the late reply, I actually had prepared a complete different patch for a different problem, but it touches the very same lines of code.

(And we may want to add a !chmod(path, st1.st_mode & ~S_IXUSR) at the end of the operation)

There are systems when a file created with 666 have 766, and we do not handle this situation yet.

Michael, if there is a chance that you integrate my small code changes in your patch ?

-------------

commit 3228bedef6d45bfaf8986b6367f9388738476345
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Sun Oct 19 00:15:00 2014 +0200

     Improve the filemode trustability check

     Some file systems do not fully support the executable bit:
     a) The user executable bit is always 0
     b) The user executable bit is always 1
     c) Is similar to b):
        When a file is created with mode 0666 the file mode on disc is 766
        and the user executable bit is 1 even if it should be 0 like b)

        There are smbfs implementations where the file mode can be 
maintained
        locally and chmod -x changes the file mode from 0766 to 0666.
        When the file system is unmounted and remounted,
        the file mode is 0766 and executable bit is 1 again.

     A typical example for a) is a VFAT drive mounted with -onoexec,
     or cifs with -ofile_mode=0644.
     b) is VFAT mounted with -oexec or cifs is mounted with -ofile_mode=0755

     The behaviour described in c) has been observed when a Windows 
machine with
     NTFS exports a share via smb (or afp) to Mac OS X.
     (CIFS is an enhanced version of SMB
      The command to mount on the command line may differ,
      e.g mount.cifs under Linux or mount_smbfs under Mac OS X)

     Case a) and b) are detected by the current code.
     Case c) qualifies as "non trustable executable bit",
     and core.filemode should be false by default.

     Solution:
     Detect when ".git/config" has the user executable bit set after
     creat(".git/config", 0666) and set core.filemode to false.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 587a505..d3e4fb3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -249,13 +249,11 @@ static int create_default_files(const char 
*template_path)
      strcpy(path + len, "config");

      /* Check filemode trustability */
-    filemode = TEST_FILEMODE;
-    if (TEST_FILEMODE && !lstat(path, &st1)) {
-        struct stat st2;
-        filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
-                !lstat(path, &st2) &&
-                st1.st_mode != st2.st_mode);
-    }
+    filemode = TEST_FILEMODE &&
+        !lstat(path, &st1) &&    !(st1.st_mode & S_IXUSR) &&
+        !chmod(path, st1.st_mode | S_IXUSR) &&
+        !lstat(path, &st1) && (st1.st_mode & S_IXUSR);
+
      git_config_set("core.filemode", filemode ? "true" : "false");

      if (is_bare_repository())