Re: Issue with insufficient permission for adding an object to repository database .git/objects

[Junio C Hamano <gitster@pobox.com>]

Chris Torek <chris.torek@gmail.com> writes:

> What Git does is this:
>
>  * form the name of a file that we expect not to exist
>  * use an open() system call in this way:
>
>      fd = open(path, O_CREAT | O_EXCL | O_RDWR, 0444)
>
> Note that this is a single, atomic system call that asks
> the OS to:
>
>  1. make sure the path does not exist currently -- if it
>     does, return an error;
>  2. create that path and open the resulting file for
>     reading and writing, but make sure that no one else
>     may write to that path.
>
> On a local file system, this really is a single atomic
> operation: either the path does not exist *and* you are
> allowed to create it *and* the creation succeeds *and*
> you now have a writable file-handle for a read-only file;
> or, any one of the "and"s above has failed (file already
> existed, you aren't allowed to create here, etc).

That's a very nice and clealy written analysis.  What you outlined
above is exactly why we use these flags and create the file
read-only from the get go.

Having said that, most of these "create atomically" that are coming
via the tempfile API could use a looser 0644, as the primary reason
why we are careful is not to protect against bad actors but to
protect against ourselves (another process or another thread) racing
to create the same object---in other words, we are happy as long as
O_CREAT|O_EXCL is honored, and "link into the final place and then
unlink the tempfile" or "rename into the final place" patterns that
are used to conclude the temporary file obtained via the tempfile
API are reliable on the target filesystem.

The resulting file being read-only with 0444 (limited further by
umask) is not all that important.  It is certainly possible to
destroy an owner-writable file by redirecting into it, and making
the files read-only may prevent such an accident from happening, but
immediate parent directories of these files have to be at least
owner-writable, so it is just as easy to destroy such a read-only
file by unlinking it from its parent directory.

I may be forgetting important reasons why we insist read-only files
in $GIT_DIR for objects, packs and commit-graph files (there may be
others), but otherwise I do not have a strong objection to a patch
with a well written log message that loosens these 0444 modes that
are (eventually) given to git_mkstemp_mode() to 0644.

Thanks.