Re: [PATCH] commit: ensure correct permissions of the commit message

From: Torsten Bögershausen <tboegi@web.de>
Date: Dec 20, 2015, 22:57 UTC
Message-ID: <56773264.2050806@web.de>
In-Reply-To: <alpine.DEB.2.20.1512201519590.6483@virtualbox>

On 2015-12-20 15.21, Johannes Schindelin wrote:

Show 43 quoted lines

> Hi Peff,
> 
> On Sun, 20 Dec 2015, Jeff King wrote:
> 
>> On Sat, Dec 19, 2015 at 07:21:59PM +0100, Johannes Schindelin wrote:
>>
>>> It was pointed out by Yaroslav Halchenko that the file containing the
>>> commit message had the wrong permissions in a shared setting.
>>>
>>> Let's fix that.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> I think this is probably a step forward, but I have to wonder how many
>> other files are in a similar situation (e.g., git-am state files, etc).
> 
> True.
> 
>> I think people generally haven't noticed because shared repositories are
>> generally about a shared bare rendezvous repo. So refs and objects are
>> important, but we don't expect people to commit.
>>
>> So I don't have any real problem with this, but I suspect it's just the
>> tip of the iceberg. We might want something like:
>>
>>   FILE *fopen_shared(const char *path, const char *mode)
>>   {
>> 	FILE *ret = fopen(path, mode);
>> 	if (!ret)
>> 		return NULL;
>> 	if (adjust_shared_perm(path)) {
>> 		fclose(ret);
>> 		return NULL;
>> 	}
>> 	return ret;
>>   }
>>
>> but of course the hard part is auditing all of the existing fopen()
>> calls to see who needs to use it. :)
> 
> In principle, I agree, but I have to point out that the
> adjust_shared_perm() call must come after the *fclose()* call, to avoid
> modifying files to which we currently have open file handles.

I had the same concern, but couldn't find anything that gives a hint that we can't adjust the permissions on an open file.

(In opposite: We can't rename an open file (under Windows))

In fact we have this in tempfile.c

int create_tempfile(struct tempfile *tempfile, const char *path)
{
	prepare_tempfile_object(tempfile);

	strbuf_add_absolute_path(&tempfile->filename, path);
	tempfile->fd = open(tempfile->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
	if (tempfile->fd < 0) {
		strbuf_reset(&tempfile->filename);
		return -1;
	}
	tempfile->owner = getpid();
	tempfile->active = 1;
	if (adjust_shared_perm(tempfile->filename.buf)) {
[snip] Error out

(And this seems to be the same in git.git and git-for-windows)