[Bug] Permissions of temp file created in git's sha1_file.c correct?

Hello,

I recently (very recently: yesterday) started using git. I switched from Bazaar, that required me to have a file system that supports the executable flag (as it always stores this flag based on what the file system says). FAT does not support the executable flag.

To make a long story short: I modified Apple's FAT kernel extension to support both, executable and the writable flag. With this file system, I was not able to push a change to the repository that is stored on a volume using the file system. I tracked the problem down to one changeset: <http://git.kernel.org/?p=git/git.git;a=commit;h=5256b006312e4d06e11b49a8b128e9e550e54f31> introduces the line "fd = git_mkstemp_mode(buffer, 0444);" in sha1_file.c.

This line creates a temporary file that has reading rights for everyone but no writing rights. I was astonished that it is possible to actually write to a file you do not have write permission on. My file system driver does not allow any change on read-only files except changing rights. Especially it refuses to rename or delete the file. Both is tried some lines after the file creation (move_temp_to_file, called when returning from write_loose_object), resulting in an error that aborts the push.

I think that my file system is right (of course ;-) ): A file that is not writable must not be renamed (arguable as the name could be seen as the content of the containing directory) and it must not be deleted.

Do anyone agree? If so, my suggestion is to replace both occurrences of "0444" in sha1_file.c by "0644" (or better "S_IRUSR|S_IWUSR|S_IWGRP|S_IWOTH", or even "0600"/"S_IRUSR|S_IWUSR"). The changed rights are changed at the end of move_temp_to_file anyways and allowing the owner to write to the file is probably not a security issue.

Bye,
   Daniel

Daniel Höpfl <daniel@hoepfl.de> writes:

That is why there is `core.fileMode` config variable, see git-config(1) manpage:

   core.fileMode
         If false, the executable bit differences between the index  and  the
         working copy are ignored; useful on broken filesystems like FAT. See
         git-update-index(1).

         The default is true, except git-clone(1) or git-init(1)  will  probe
         and  set  core.fileMode  false if appropriate when the repository is
         created.

Didin't git detect that it was on such filesystem?

[...]

Not true. Whether file can be renamed or deleted is governed by permissions in directory that contains given file, not the file itself (at least on POSIX). If I understand things correctly, of course.

Note that git doesn't store full permissions, in particular read/write permissions, only executable bit and "is symlink" thing.

On Wed, 02 Feb 2011 04:47:20 -0800 (PST), Jakub Narebski <jnareb@gmail.com> wrote:

I saw (and liked) that option but I do not want to use the executable bit on my "FAT-compatible" file system for other things anyways.

I did not create the repository on the device, it was simply copied onto the volume. One `git init` later: Git detects that the filesystem knows about the executable flag and sets filemode = true. (On a real FAT, it would be set to false, of course).

OK, then that's a reason to fix my implementation. (Done.)

I still think that a file that is to be written to should not be created without the right to do so.

Executable is all I need. Since there was only one bit left in the FAT directory entries, my file system uses this bit to store the executable flag. Writeable is derived from/mapped to !readonly. That's all I changed, compared to a normal FAT.

Thanks for the very fast answer,
   Daniel