Re: [PATCH v2 1/2] create_default_files(): don't set u+x bit on $GIT_DIR/config
On 11/17/2014 02:40 AM, Eric Sunshine wrote:
On Sun, Nov 16, 2014 at 2:21 AM, Michael Haggerty 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
---
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
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())
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/16/2014 09:06 AM, Johannes Sixt wrote:
> Am 16.11.2014 um 08:21 schrieb Michael Haggerty:
>> @@ -559,9 +562,21 @@ int cmd_config(int argc, const char **argv, const char
>> *prefix)
>> if (given_config_source.blob)
>> die("editing blobs is not supported");
>> git_config(git_default_config, NULL);
>> -launch_editor(given_config_source.file ?
>> - given_config_source.file : git_path("config"),
>> - NULL, NULL);
>> +config_file = xstrdup(given_config_source.file ?
>> + given_config_source.file :
>> git_path("config"));
>> +launch_editor(config_file, NULL, NULL);
>> +
>> +/*
>> + * In git 2.1, there was a bug in "git init" that left
>> + * the u+x bit set on the config file. To clean up any
>> + * repositories affected by that bug, and just because
>> + * it doesn't make sense for a config file to be
>> + * executable anyway, clear any executable bits from
>> + * the file (on a "best effort" basis):
>> + */
>> +if (!lstat(config_file, &st) && (st.st_mode & 0111))
>
> At this point we cannot be sure that config_file is a regular file, can
> we? It could also be a symbolic link. Wouldn't plain stat() be more
> correct then?
You make a good point. But I'm a little nervous about following symlinks
and changing permissions on some distant file. Also, the bug that we are
trying clean up after would not have created a symlink in this place, so
I think the cleanup is not so important if "config" is a symlink.
So I suggest that we stick with lstat(), but add S_ISREG(st.st_mode) to
the && chain above. Does that sound reasonable?
>> +chmod(config_file, st.st_mode & 07666);
>> +free(config_file);
>> }
>> else if (actions == ACTION_SET) {
>> int ret;
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/16/2014 07:49 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> There is no reason for $GIT_DIR/config to be executable, plus this >> change will help clean up repositories affected by the bug that was >> fixed by the previous commit. > > I do not think we want to do this. > > It is a welcome bugfix to create $GIT_DIR/config without executable > bit when and only when we create it. It is very much in line with > "There is no reason for $GIT_DIR/config to be executable"---we do > not need to make it executable ourselves, so we shouldn't, but we > did which was a bug we want to fix in patch 1/2 you posted. > > But with the "preserve existing permissions" fix we did earlier, the > end users are now allowed to flip the executable bit on for their > own purpose, and dropping it without knowing why they are doing so > is simply rude. And honestly, Git do *not* even want to know why > the users want to flip the bit. > > So I would suggest not to spend any cycle or any code complexity to > "repair" existing repositories. Having that bit on does not hurt > anybody. Those who found it curious can flip that bit off and then > Git with "preserve existing permissions" fix will keep that bit off > from then on. I disagree. The point of "preserve existing permissions" was to allow people to make their config files more readable/writable than the default, with the main use case being to help users who want to hide secret information in their config files. I think it is really far-fetched to imagine that anybody made his config file executable on purpose. Whereas we are *sure* that we have created lots of repositories with config files that were set executable by accident. Let's redefine the "feature" that was added in 2.1 from "preserve existing permissions" to "preserve existing read/write permissions" and thereby help people clean up the mess we made. That being said, I still believe that executable config files are not a significant risk in practice, so I'm not going to lose sleep about it either way. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] gitk: po/ru.po russian translation typo fixed
Hello Max and Paul, thank you for your feedback, so what's must be my next workflow? Resend patch with "Reviewed-By:..." or somethine else? -- Best regards. 0xAX -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
