Chris Torek <chris.torek@xxxxxxxxx> 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.
