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

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

[Johannes Schindelin]

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>
---
 builtin/commit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..3bfd457 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -905,6 +905,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	strbuf_release(&committer_ident);
 
 	fclose(s->fp);
+	adjust_shared_perm(git_path(commit_editmsg));
 
 	/*
 	 * Reject an attempt to record a non-merge empty commit without
-- 
2.6.3.windows.1.300.g1c25e49

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

[Jeff King]

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).
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. :)

-Peff

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Torsten Bögershausen]

On 2015-12-20 15.21, Johannes Schindelin wrote:
> 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)

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

Hi Torsten,

On Sun, 20 Dec 2015, Torsten Bögershausen wrote:

> On 2015-12-20 15.21, Johannes Schindelin wrote:
>
> > 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.

Oops, okay! I misremembered. This makes our task much easier now.

Thanks!
Dscho

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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;
>   }

Actually, we do not even _need_ a sharedness for this ephemeral
file.  The additional "adjust-shared-perm" is merely a workaround
for the fact the next person cannot write into it when it is left
behind, and because we do not want to remove it when we are done.

That does not measn that the next person cannot remove it when she
finds there is a file there left behind.  So alternatively, we could
do something like this, perhaps?

        FILE *fopen_forcibly(const char *path, const char *mode)
        {
                FILE *ret = fopen(path, mode);

                if (!ret && errno == EPERM) {
                        if (!unlink(path))
                                ret = fopen(path, mode);
                        else
                                errno = EPERM;
                }
                return ret;
        }

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

[Jeff King]

On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:

> Actually, we do not even _need_ a sharedness for this ephemeral
> file.  The additional "adjust-shared-perm" is merely a workaround
> for the fact the next person cannot write into it when it is left
> behind, and because we do not want to remove it when we are done.
> 
> That does not measn that the next person cannot remove it when she
> finds there is a file there left behind.  So alternatively, we could
> do something like this, perhaps?
> 
>         FILE *fopen_forcibly(const char *path, const char *mode)
>         {
>                 FILE *ret = fopen(path, mode);
> 
>                 if (!ret && errno == EPERM) {
>                         if (!unlink(path))
>                                 ret = fopen(path, mode);
>                         else
>                                 errno = EPERM;
>                 }
>                 return ret;
>         }

Yeah, I think that is a much nicer solution for this case. It should
work even in a shared repo, since we set the permissions for the
surrounding $GIT_DIR appropriately[1].

I guess it would not apply to any files that do not want to truncate the
existing contents. Probably it should drop the "mode" parameter at all,
since anything but "w" would be crazy?

-Peff

[1] Assuming you use something like "git init --shared=true". I would
    not be surprised if there are many repos where somebody just set
    "core.sharedrepository" after the fact, but there is not much we can
    do. They _have_ to fix up the permissions in that case.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:
>
>> Actually, we do not even _need_ a sharedness for this ephemeral
>> file.  The additional "adjust-shared-perm" is merely a workaround
>> for the fact the next person cannot write into it when it is left
>> behind, and because we do not want to remove it when we are done.
>> 
>> That does not measn that the next person cannot remove it when she
>> finds there is a file there left behind.  So alternatively, we could
>> do something like this, perhaps?
>> 
>>         FILE *fopen_forcibly(const char *path, const char *mode)
>>         {
>>                 FILE *ret = fopen(path, mode);
>> 
>>                 if (!ret && errno == EPERM) {
>>                         if (!unlink(path))
>>                                 ret = fopen(path, mode);
>>                         else
>>                                 errno = EPERM;
>>                 }
>>                 return ret;
>>         }
>
> Yeah, I think that is a much nicer solution for this case. It should
> work even in a shared repo, since we set the permissions for the
> surrounding $GIT_DIR appropriately[1].
>
> I guess it would not apply to any files that do not want to truncate the
> existing contents. Probably it should drop the "mode" parameter at all,
> since anything but "w" would be crazy?

Absolutely.  Thanks for spotting.

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

[Johannes Schindelin]

Hi,

On Mon, 21 Dec 2015, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Sun, Dec 20, 2015 at 05:31:48PM -0800, Junio C Hamano wrote:
> >
> >> we could do something like this, perhaps?
> >> 
> >>         FILE *fopen_forcibly(const char *path, const char *mode)
> >>         {
> >>                 FILE *ret = fopen(path, mode);
> >> 
> >>                 if (!ret && errno == EPERM) {
> >>                         if (!unlink(path))
> >>                                 ret = fopen(path, mode);
> >>                         else
> >>                                 errno = EPERM;
> >>                 }
> >>                 return ret;
> >>         }
> >
> > Yeah, I think that is a much nicer solution for this case. It should
> > work even in a shared repo, since we set the permissions for the
> > surrounding $GIT_DIR appropriately[1].
> >
> > I guess it would not apply to any files that do not want to truncate the
> > existing contents. Probably it should drop the "mode" parameter at all,
> > since anything but "w" would be crazy?
> 
> Absolutely.  Thanks for spotting.

So maybe

	fcreate_or_truncate(const char *path)
	{
		 FILE *ret = fopen(path, "w");

		 if (!ret && errno == EPERM) {
			 if (!unlink(path))
				 ret = fopen(path, "w");
			 else
				 errno = EPERM;
		 }
		 return ret;
	 }

?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> So maybe
>
> 	fcreate_or_truncate(const char *path)
> 	{
> 		 FILE *ret = fopen(path, "w");
>
> 		 if (!ret && errno == EPERM) {
> 			 if (!unlink(path))
> 				 ret = fopen(path, "w");
> 			 else
> 				 errno = EPERM;
> 		 }
> 		 return ret;
> 	 }
>
> ?

I do not know fcreate_or_truncate() is a good name, though.  Looking
for "fcreate(3)" seems to find something quite obscure and unrelated
to what we want (http://linux.die.net/man/3/fcreate).  Also we call
a function X_or_Y to mean "try to do our primary thing X, and do Y
instead when it fails", but what we want to do here is not that.  We
are not interested in creating (i.e. we can happily live with an
existing file opened for writing), and we do not just truncate if we
cannot open the original for writing (i.e. we do return a writable
file handle) [*1*].

In any case, obviously the return value of the function needs to be
of type "FILE *".

Other than that, the code inside the function looks exactly like
what I had in mind.

Thanks.


[Footnote]

*1* We tend to call a function sane_X() when we are fixing an
    undesirable behaviour in the underlying function X (e.g.
    sane_execvp()).  In that sense it is very tempting to call this
    sane_fopen() but our use is limited to fopen(path, "w"), i.e.
    "open for writing, while truncating if existing" use case of
    fopen().

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

[Johannes Schindelin]

Hi Junio,

On Wed, 30 Dec 2015, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > So maybe
> >
> > 	fcreate_or_truncate(const char *path)
> > 	{
> > 		 FILE *ret = fopen(path, "w");
> >
> > 		 if (!ret && errno == EPERM) {
> > 			 if (!unlink(path))
> > 				 ret = fopen(path, "w");
> > 			 else
> > 				 errno = EPERM;
> > 		 }
> > 		 return ret;
> > 	 }
> >
> > ?
> 
> I do not know fcreate_or_truncate() is a good name, though.

So what would be a good name?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I do not know fcreate_or_truncate() is a good name, though.
>
> So what would be a good name?

Have been thinking about it, but I did not come up with anything.  I
just know fcreate_or_truncate() is not a good name for multiple
reasons I already explained X-<.  sane_fopen_for_writing() perhaps?

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

[Johannes Schindelin]

Hi Junio,

On Mon, 4 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I do not know fcreate_or_truncate() is a good name, though.
> >
> > So what would be a good name?
> 
> Have been thinking about it, but I did not come up with anything.  I
> just know fcreate_or_truncate() is not a good name for multiple
> reasons I already explained X-<.  sane_fopen_for_writing() perhaps?

That implies that fopen() is insane, though ;-) Let's just drop the sane_
prefix?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Mon, 4 Jan 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> >> I do not know fcreate_or_truncate() is a good name, though.
>> >
>> > So what would be a good name?
>> 
>> Have been thinking about it, but I did not come up with anything.  I
>> just know fcreate_or_truncate() is not a good name for multiple
>> reasons I already explained X-<.  sane_fopen_for_writing() perhaps?
>
> That implies that fopen() is insane, though ;-)

Well, sane_X() does not imply X() is "insane".  The reason why we
introduce sane_X() wrapper often is because X is inconvenient to use
sanely as-is.

Look at sane_istest(), sane_case(), etc.  We have one byte and want
to see if that is space, digit, etc., but we have to cast (possibly
signed) char to unsigned char and always doing so in the callers is
inconvenient.  Look at sane_grep.  We have a string and want to see
that appears in its input, but some implementations of "grep" can
be configured in an inconvenient/unsuitable way to be used in
scripts via environment variables, so we disable it not in the
callers but in a wrapper.

All we want to do here is to get a writable filehandle FILE * for a
path and store the contents we will generate without having to worry
about the possibility that the path is already there and we may have
to first unlink it to use fopen(3) to do so.

So in the sense, fopen(3) is inconvenient to use sanely as-is.  You
can call it insane, too ;-)

If we want to follow the X_or_Y() pattern, fopen_or_create() may
describe what it does better.  I do not have strong preference
either way, but again I am not good at naming things (and I suspect
you aren't either), so...

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

[Johannes Schindelin]

Hi Junio,

On Tue, 5 Jan 2016, Junio C Hamano wrote:

> If we want to follow the X_or_Y() pattern, fopen_or_create() may
> describe what it does better.  I do not have strong preference either
> way, but again I am not good at naming things (and I suspect you aren't
> either), so...

Heh... You got that right...

Let's let it simmer for a couple more days, maybe somebody else chimes in
with a brilliant idea... :-)

Ciao,
Dscho

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

[Jeff King]

On Wed, Jan 06, 2016 at 09:20:34AM +0100, Johannes Schindelin wrote:

> Hi Junio,
> 
> On Tue, 5 Jan 2016, Junio C Hamano wrote:
> 
> > If we want to follow the X_or_Y() pattern, fopen_or_create() may
> > describe what it does better.  I do not have strong preference either
> > way, but again I am not good at naming things (and I suspect you aren't
> > either), so...
> 
> Heh... You got that right...
> 
> Let's let it simmer for a couple more days, maybe somebody else chimes in
> with a brilliant idea... :-)

I can be the anti-brilliant and just shoot down what has been said. :)

I think fopen_or_create is confusing; it implies to me that we'll open
an existing file or create it if it's not there. But we are always about
truncating/replacing the existing file.

I think fopen_for_writing() is fine, or fopen_truncate().

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Wed, 6 Jan 2016, Jeff King wrote:

> I think fopen_for_writing() is fine, or fopen_truncate().

I like fopen_for_writing() better than fopen_truncate() because it states
the intention more clearly.

Thanks!
Dscho

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> Actually, we do not even _need_ a sharedness for this ephemeral
> file.  The additional "adjust-shared-perm" is merely a workaround
> for the fact the next person cannot write into it when it is left
> behind, and because we do not want to remove it when we are done.

Do we really want to keep such a file after we are done?

 From all the cases where $GIT_EDITOR is run as part of an operation  
to edit a temporary file, that temporary file is deleted on success  
most of the time, except the already mentioned COMMIT_EDITMSG,  
REPLACE_EDITOBJ for 'git replace --edit', and BRANCH_DESCRIPTION for  
'git branch --edit-description'.  I don't see why these files should  
be handled differently than the others and why should they be kept  
after the operation finished, since at that point the contents is  
already stored in its proper place (in the commit object, in the  
replacement object, or in the config file), making these files  
redundant and unnecessary.

If the operation fails for whatever reason, then that's a different  
matter, of course.  In that case we definitely want to keep these  
files to avoid losing whatever the user has written.

Gábor

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> Actually, we do not even _need_ a sharedness for this ephemeral
>> file.  The additional "adjust-shared-perm" is merely a workaround
>> for the fact the next person cannot write into it when it is left
>> behind, and because we do not want to remove it when we are done.
>
> Do we really want to keep such a file after we are done?

There is no strong reason we should want to remove them, either.

As long as the lazy garbage collection works, there is no incentive
to change things.  I do not have strong objection against such a
change, as long as you do not break people's existing practices,
either, though.

Whoever is designing such a change must carefully define what "after
we are done" exactly means.  Removing such a file immediately after
the command read from it is likely to be a bad idea, for example, if
the command supports hooks that are invoked after the command reads
and uses the contents of it, as the hooks may be depending on the
presence of it and the ability to read from it.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

Hi,

On Thu, 14 Jan 2016, Junio C Hamano wrote:

> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > Quoting Junio C Hamano <gitster@pobox.com>:
> >
> >> Actually, we do not even _need_ a sharedness for this ephemeral file.
> >> The additional "adjust-shared-perm" is merely a workaround for the
> >> fact the next person cannot write into it when it is left behind, and
> >> because we do not want to remove it when we are done.
> >
> > Do we really want to keep such a file after we are done?
> 
> There is no strong reason we should want to remove them, either.
> 
> As long as the lazy garbage collection works, there is no incentive
> to change things.  I do not have strong objection against such a
> change, as long as you do not break people's existing practices,
> either, though.
> 
> Whoever is designing such a change must carefully define what "after
> we are done" exactly means.  Removing such a file immediately after
> the command read from it is likely to be a bad idea, for example, if
> the command supports hooks that are invoked after the command reads
> and uses the contents of it, as the hooks may be depending on the
> presence of it and the ability to read from it.

... and do not forget scripts that call `git commit`. They can expect the
presence of that file after a successful operation right now. You will
have to warn any potential user of such a script that you are going to
break their setup.

Ciao,
Dscho

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> Quoting Junio C Hamano <gitster@pobox.com>:
>>
>>> Actually, we do not even _need_ a sharedness for this ephemeral
>>> file.  The additional "adjust-shared-perm" is merely a workaround
>>> for the fact the next person cannot write into it when it is left
>>> behind, and because we do not want to remove it when we are done.
>>
>> Do we really want to keep such a file after we are done?
>
> There is no strong reason we should want to remove them, either.
>
> As long as the lazy garbage collection works, there is no incentive
> to change things.

I'm not sure what you mean by "lazy garbage collection"; I suppose not  
that I sometimes delete a stale COMMIT_EDITMSG or two by hand...

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

Hi Gábor,

On Fri, 15 Jan 2016, SZEDER Gábor wrote:

> Quoting Junio C Hamano <gitster@pobox.com>:
> 
> >SZEDER Gábor <szeder@ira.uka.de> writes:
> >
> > >Quoting Junio C Hamano <gitster@pobox.com>:
> > >
> > > >Actually, we do not even _need_ a sharedness for this ephemeral
> > > >file.  The additional "adjust-shared-perm" is merely a workaround
> > > >for the fact the next person cannot write into it when it is left
> > > >behind, and because we do not want to remove it when we are done.
> > >
> > >Do we really want to keep such a file after we are done?
> >
> >There is no strong reason we should want to remove them, either.
> >
> >As long as the lazy garbage collection works, there is no incentive
> >to change things.
> 
> I'm not sure what you mean by "lazy garbage collection"; I suppose not
> that I sometimes delete a stale COMMIT_EDITMSG or two by hand...

You do not delete a stale COMMIT_EDITMSG *by hand*, but essentially it is
deleted the next time you commit.

Ciao,
Dscho

[PATCH v2 0/2] Correctly handle transient files in shared repositories

[Johannes Schindelin]

Transient files, e.g. commit messages, are writable only by the owner,
even in shared repositories, to avoid interference between competing
users working on the same files.

These files are typically not deleted after use. As a consequence, we
have to delete such files before writing when they are owned by someone
else than the current user.

Reported-by: Yaroslav Halchenko <yoh@onerussian.com>


Johannes Schindelin (2):
  commit: allow editing the commit message even in shared repos
  Handle more file writes correctly in shared repos

 builtin/commit.c      |  2 +-
 builtin/fast-export.c |  2 +-
 builtin/fetch.c       |  2 +-
 git-compat-util.h     |  1 +
 wrapper.c             | 13 +++++++++++++
 5 files changed, 17 insertions(+), 3 deletions(-)

Interdiff vs v1:

 diff --git a/builtin/commit.c b/builtin/commit.c
 index 3bfd457..89bf6ad 100644
 --- a/builtin/commit.c
 +++ b/builtin/commit.c
 @@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
  		hook_arg2 = "";
  	}
  
 -	s->fp = fopen(git_path(commit_editmsg), "w");
 +	s->fp = fopen_for_writing(git_path(commit_editmsg));
  	if (s->fp == NULL)
  		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
  
 @@ -905,7 +905,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
  	strbuf_release(&committer_ident);
  
  	fclose(s->fp);
 -	adjust_shared_perm(git_path(commit_editmsg));
  
  	/*
  	 * Reject an attempt to record a non-merge empty commit without
 diff --git a/builtin/fast-export.c b/builtin/fast-export.c
 index d9ac5d8..2471297 100644
 --- a/builtin/fast-export.c
 +++ b/builtin/fast-export.c
 @@ -880,7 +880,7 @@ static void export_marks(char *file)
  	FILE *f;
  	int e = 0;
  
 -	f = fopen(file, "w");
 +	f = fopen_for_writing(file);
  	if (!f)
  		die_errno("Unable to open marks file %s for writing.", file);
  
 diff --git a/builtin/fetch.c b/builtin/fetch.c
 index 586840d..33f04c1 100644
 --- a/builtin/fetch.c
 +++ b/builtin/fetch.c
 @@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)
  static int truncate_fetch_head(void)
  {
  	const char *filename = git_path_fetch_head();
 -	FILE *fp = fopen(filename, "w");
 +	FILE *fp = fopen_for_writing(filename);
  
  	if (!fp)
  		return error(_("cannot open %s: %s\n"), filename, strerror(errno));
 diff --git a/git-compat-util.h b/git-compat-util.h
 index 2da0a75..e8f2867 100644
 --- a/git-compat-util.h
 +++ b/git-compat-util.h
 @@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);
  extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
  extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
  extern char *xgetcwd(void);
 +extern FILE *fopen_for_writing(const char *path);
  
  #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
  
 diff --git a/wrapper.c b/wrapper.c
 index b43d437..29a45d2 100644
 --- a/wrapper.c
 +++ b/wrapper.c
 @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)
  	return stream;
  }
  
 +FILE *fopen_for_writing(const char *path)
 +{
 +	FILE *ret = fopen(path, "w");
 +
 +	if (!ret && errno == EPERM) {
 +		if (!unlink(path))
 +			ret = fopen(path, "w");
 +		else
 +			errno = EPERM;
 +	}
 +	return ret;
 +}
 +
  int xmkstemp(char *template)
  {
  	int fd;

-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v2 1/2] commit: allow editing the commit message even in shared repos

[Johannes Schindelin]

It was pointed out by Yaroslav Halchenko that the file containing the
commit message is writable only by the owner, which means that we have
to rewrite it from scratch in a shared repository.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/commit.c  |  2 +-
 git-compat-util.h |  1 +
 wrapper.c         | 13 +++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..89bf6ad 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	s->fp = fopen(git_path(commit_editmsg), "w");
+	s->fp = fopen_for_writing(git_path(commit_editmsg));
 	if (s->fp == NULL)
 		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 2da0a75..e8f2867 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);
 extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
 extern char *xgetcwd(void);
+extern FILE *fopen_for_writing(const char *path);
 
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
 
diff --git a/wrapper.c b/wrapper.c
index b43d437..29a45d2 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)
 	return stream;
 }
 
+FILE *fopen_for_writing(const char *path)
+{
+	FILE *ret = fopen(path, "w");
+
+	if (!ret && errno == EPERM) {
+		if (!unlink(path))
+			ret = fopen(path, "w");
+		else
+			errno = EPERM;
+	}
+	return ret;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 1/2] commit: allow editing the commit message even in shared repos

[Jeff King]

On Wed, Jan 06, 2016 at 02:09:43PM +0100, Johannes Schindelin wrote:

> It was pointed out by Yaroslav Halchenko that the file containing the
> commit message is writable only by the owner, which means that we have
> to rewrite it from scratch in a shared repository.
> [...]
> diff --git a/wrapper.c b/wrapper.c
> index b43d437..29a45d2 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)
>  	return stream;
>  }
>  
> +FILE *fopen_for_writing(const char *path)
> +{
> +	FILE *ret = fopen(path, "w");
> +
> +	if (!ret && errno == EPERM) {
> +		if (!unlink(path))
> +			ret = fopen(path, "w");
> +		else
> +			errno = EPERM;
> +	}
> +	return ret;
> +}

Thanks, this looks good to me. Having seen the implementation, it really
is just "try harder to fopen()".  I guess calling it "fopen_me_harder()"
would be too obscure. :)

-Peff

Re: [PATCH v2 1/2] commit: allow editing the commit message even in shared repos

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 06, 2016 at 02:09:43PM +0100, Johannes Schindelin wrote:
>
>> It was pointed out by Yaroslav Halchenko that the file containing the
>> commit message is writable only by the owner, which means that we have
>> to rewrite it from scratch in a shared repository.
>> [...]
>> diff --git a/wrapper.c b/wrapper.c
>> index b43d437..29a45d2 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)
>>  	return stream;
>>  }
>>  
>> +FILE *fopen_for_writing(const char *path)
>> +{
>> +	FILE *ret = fopen(path, "w");
>> +
>> +	if (!ret && errno == EPERM) {
>> +		if (!unlink(path))
>> +			ret = fopen(path, "w");
>> +		else
>> +			errno = EPERM;
>> +	}
>> +	return ret;
>> +}
>
> Thanks, this looks good to me. Having seen the implementation, it really
> is just "try harder to fopen()".  I guess calling it "fopen_me_harder()"
> would be too obscure. :)

obscure or you meant some other obsxxxx?  fopen_harder() does not
sound too bad, but fopen_for_writing() is OK to me.

And its use in this patch looks sensible.

Thanks.

[PATCH v2 2/2] Handle more file writes correctly in shared repos

[Johannes Schindelin]

In shared repositories, we have to be careful when writing files whose
permissions do not allow users other than the owner to write them.

In particular, we force the marks file of fast-export and the FETCH_HEAD
when fetching to be rewritten from scratch.

This commit does not touch the following users of fopen() that want to
write files:

- git am, when splitting mails (git-am correctly cleans up its directory
  after finishing, so there is no need to share those files between users)

- git apply, when writing rejected hunks

- git fsck, when writing lost&found blobs

- git merge-file, when writing merged files (when Git itself calls
  merge-file, the file in question was already there, with shared
  permissions).

- git submodule clone, when writing the .git file, because the file will
  not be overwritten

- git_terminal_prompt() in compat/terminal.c, because it is not writing to
  a file at all

- git diff --output, because the output file is clearly not intended to be
  shared between the users of the current repository

- git fast-import, when writing a crash report

- mailinfo() in mailinfo.c, because the output is clearly not intended to
  be shared between the users of the current repository

- check_or_regenerate_marks() in remote-testsvn.c, because this is only
  used for Git's internal testing

- git rerere, when writing resolved files, because the files in question
  were already written with the correct permissions

Note that this patch does not touch callers of write_file() and
write_file_gently(), which would benefit from the same scrutiny as to
usage in shared repositories. Most notable users: branch, daemon,
submodule & worktree, and a worrisome call in transport.c when updating
one ref (which ignores the shared flag).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 2 +-
 builtin/fetch.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d9ac5d8..2471297 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -880,7 +880,7 @@ static void export_marks(char *file)
 	FILE *f;
 	int e = 0;
 
-	f = fopen(file, "w");
+	f = fopen_for_writing(file);
 	if (!f)
 		die_errno("Unable to open marks file %s for writing.", file);
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..33f04c1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)
 static int truncate_fetch_head(void)
 {
 	const char *filename = git_path_fetch_head();
-	FILE *fp = fopen(filename, "w");
+	FILE *fp = fopen_for_writing(filename);
 
 	if (!fp)
 		return error(_("cannot open %s: %s\n"), filename, strerror(errno));
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Jeff King]

On Wed, Jan 06, 2016 at 02:09:49PM +0100, Johannes Schindelin wrote:

> In shared repositories, we have to be careful when writing files whose
> permissions do not allow users other than the owner to write them.
> 
> In particular, we force the marks file of fast-export and the FETCH_HEAD
> when fetching to be rewritten from scratch.
> 
> This commit does not touch the following users of fopen() that want to
> write files:

The patch looks good, and thanks for cataloguing the other cases.

> - git am, when splitting mails (git-am correctly cleans up its directory
>   after finishing, so there is no need to share those files between users)
> 
> - git apply, when writing rejected hunks
> 
> - git fsck, when writing lost&found blobs

For these latter two, I had to ask myself "why not?". You gave such nice
reasons for the other items in the list, I wondered what your reasoning
was here. In the second case, for example, I think it would prevent a
second shared user from running `git fsck --lost-found`, because we are
likely to find the same "old" dangling objects and fail to write them.

I also wondered why we do not do the usual write-to-temp and rename in
some of these cases, but that is not really relevant to your patch.

-Peff

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Johannes Schindelin]

Hi Peff,

On Thu, 7 Jan 2016, Jeff King wrote:

> On Wed, Jan 06, 2016 at 02:09:49PM +0100, Johannes Schindelin wrote:
> 
> > - git am, when splitting mails (git-am correctly cleans up its
> > directory after finishing, so there is no need to share those files
> > between users)
> > 
> > - git apply, when writing rejected hunks
> > 
> > - git fsck, when writing lost&found blobs
> 
> For these latter two, I had to ask myself "why not?". You gave such nice
> reasons for the other items in the list, I wondered what your reasoning
> was here.

My bad. My reading on both rejected hunks and lost & found blobs is that
the files should be cleaned up by the user who generated them. And if
another user can interfere with that cleaning up, that is bad. So I left
them non-shared to avoid such an interference.

> I also wondered why we do not do the usual write-to-temp and rename in
> some of these cases, but that is not really relevant to your patch.

Oh yeah, I'd love to stop here (I already extended this rather simple
one-line patch to something vastly larger, and that was not my intention).

Ciao,
Dscho

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> - git apply, when writing rejected hunks

Today I may try to apply and leave hello.c.rej; tomorrow you may try
to apply a different patch and get rejection for the same file.  And
you would not be able to if my umask is 077.

I do not know "intended to be shared" is a good criteria, unless it
is a synomym to "what is under $GIT_DIR".  core.sharedRepository is
about the shared-ness of $GIT_DIR and never about the working tree,
so I consider it is correct that not adjusting the permission is the
right thing to do when 'git apply' writes .rej files, though.

> - git fsck, when writing lost&found blobs

So this _is_ conceptually a problem, but writing anything indexed
with the object name is an idempotent operation, so this will not
matter in practice, I think.

> - git merge-file, when writing merged files (when Git itself calls
>   merge-file, the file in question was already there, with shared
>   permissions).

Again, this is not a problem (i.e. not touching it in this patch is
the right thing to do) as this is about files in the working tree.

> - git fast-import, when writing a crash report

I am not sure about this one.  Is the crash report designed to get
unique filename every time you run?  Otherwise, the fixed name
inside $GIT_DIR/ is a shared resource, so I suspect it would want to
overwrite.  "Not overwriting the crash report is safer, because the
existence of it is a sign that the earlier crash hasn't been dealt
with" is also a valid position to take, but then it shouldn't even
overwrite my own crash report from an earlier run.

So I am inclined to say that this should be changed (either
consistently overwrite using fopen_harder(), or consistently fail
when my earlier crash report is already there).  I however do not
think that change belongs to this topic.

> - mailinfo() in mailinfo.c, because the output is clearly not intended to
>   be shared between the users of the current repository

This is more because "not intended to be run multiple simultaneously
using the same filename" plus "the callers clean up after they are
done" (similar to what you wrote for "git am"), I think.

> - git rerere, when writing resolved files, because the files in question
>   were already written with the correct permissions

This again is more because the result goes to the working tree, not
$GIT_DIR.

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Johannes Schindelin]

Hi Junio,

On Thu, 7 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > - git apply, when writing rejected hunks
> 
> Today I may try to apply and leave hello.c.rej; tomorrow you may try to
> apply a different patch and get rejection for the same file.  And you
> would not be able to if my umask is 077.

As I just wrote in my reply to Peff: my experience with .rej files is that
I want to inspect them, and then delete them once I addressed them. I do
not want anybody to interfere with that, as the presence of .rej files
serves also as a TODO list.

> > - git fast-import, when writing a crash report
> 
> I am not sure about this one.  Is the crash report designed to get
> unique filename every time you run?

Yes, the pid is part of the file name. So we're golden.

Ciao,
Dscho

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 7 Jan 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > - git apply, when writing rejected hunks
>> 
>> Today I may try to apply and leave hello.c.rej; tomorrow you may try to
>> apply a different patch and get rejection for the same file.  And you
>> would not be able to if my umask is 077.
>
> As I just wrote in my reply to Peff: my experience with .rej files is that
> I want to inspect them, and then delete them once I addressed them. I do
> not want anybody to interfere with that, as the presence of .rej files
> serves also as a TODO list.

That argues for protecting .rej files against overwriting by myself,
too, which means (1) we do not want to loosen it by using
fopen_for_writing(), and (2) relying on permission bits and
ownership is not sufficient, i.e. just using regular fopen(3) is
wrong.

I think it is correct not to touch this codepath in this patch,
because of the above two reasons, but more simply and generally, it
is correct not to touch this codepath because core.sharedRepository
is not about working tree files, and .rej is a file you use in your
working tree.

As the log messages are often used to guide future developers, I
think the log message of this commit should mention that criterion.
It would cover multiple codepaths you listed in your proposed log
message in a more generic way, helping other people to reason about
when they see other instances of fopen(..., "w") and wonder if it
should become fopen_for_writing().

Thanks.

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Johannes Schindelin]

Hi Junio,

On Fri, 8 Jan 2016, Junio C Hamano wrote:

> I think it is correct not to touch this codepath in this patch,
> because of the above two reasons, but more simply and generally, it
> is correct not to touch this codepath because core.sharedRepository
> is not about working tree files, and .rej is a file you use in your
> working tree.

I am happy to adjust the log message, but I am pretty certain that the
`core.sharedRepository` setting actually also affects the working tree. At
least in my hands, calling

	git clone -c core.sharedRepository=group . test-shared

results in all of the working tree files being group-writable.

Therefore I am not convinced this is a sound line of reasoning.

Ciao,
Dscho

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 8 Jan 2016, Junio C Hamano wrote:
>
>> I think it is correct not to touch this codepath in this patch,
>> because of the above two reasons, but more simply and generally, it
>> is correct not to touch this codepath because core.sharedRepository
>> is not about working tree files, and .rej is a file you use in your
>> working tree.
>
> I am happy to adjust the log message, but I am pretty certain that the
> `core.sharedRepository` setting actually also affects the working tree. At
> least in my hands, calling
>
> 	git clone -c core.sharedRepository=group . test-shared
>
> results in all of the working tree files being group-writable.

Interesting.  I have a suspicion that "clone" does not honor the
configuration given that way, though.

 $ umask 077
 $ git clone -c core.sharedRepository=group ~/w/git.git sharedtest
 $ cd sharedtest
 $ ls -l COPYING .git/index
 -rw------- 1 jch eng 18765 Jan 11 07:43 COPYING
 -rw------- 1 jch eng 272037 Jan 11 07:43 .git/index

Notice that the permission bits in the working tree is correct, but
in the resulting .git/ they are bogus, so from this we cannot
clearly see the reason why COPYING is not group-readable is because
the checkout codepath (write_entry(), I think) is correctly omitting
the call to adjust_perm(), or simply the configuration is ignored
during the clone.

With a workaround to ensure that checkout happens definitely after
the configuration gets in effect by doing config and pull/checkout
as two separate steps:

 $ rm -fr sharedtest
 $ umask 077
 $ git init sharedtest && cd sharedtest
 $ git config core.sharedRepository group
 $ git pull ~/w/git.git/ master
 $ ls -l COPYING .git/index
 -rw------- 1 jch eng 18765 Jan 11 07:48 COPYING
 -rw-rw---- 1 jch eng 272037 Jan 11 07:48 .git/index

we can see that the configuration affects only the $GIT_DIR/ files
and not working tree.

So you found a bug in clone, I think ;-)

Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Fri, 8 Jan 2016, Junio C Hamano wrote:
>>
>>> I think it is correct not to touch this codepath in this patch,
>>> because of the above two reasons, but more simply and generally, it
>>> is correct not to touch this codepath because core.sharedRepository
>>> is not about working tree files, and .rej is a file you use in your
>>> working tree.
>>
>> I am happy to adjust the log message, but I am pretty certain that the
>> `core.sharedRepository` setting actually also affects the working tree. At
>> least in my hands, calling
>>
>> 	git clone -c core.sharedRepository=group . test-shared
>>
>> results in all of the working tree files being group-writable.
>
> Interesting.  I have a suspicion that "clone" does not honor the
> configuration given that way, though.
>
>  $ umask 077
>  $ git clone -c core.sharedRepository=group ~/w/git.git sharedtest
>  $ cd sharedtest
>  $ ls -l COPYING .git/index
>  -rw------- 1 jch eng 18765 Jan 11 07:43 COPYING
>  -rw------- 1 jch eng 272037 Jan 11 07:43 .git/index
>
> Notice that the permission bits in the working tree is correct, but
> in the resulting .git/ they are bogus, so from this we cannot
> clearly see the reason why COPYING is not group-readable is because
> the checkout codepath (write_entry(), I think) is correctly omitting
> the call to adjust_perm(), or simply the configuration is ignored
> during the clone.
>
> With a workaround to ensure that checkout happens definitely after
> the configuration gets in effect by doing config and pull/checkout
> as two separate steps:
>
>  $ rm -fr sharedtest
>  $ umask 077
>  $ git init sharedtest && cd sharedtest
>  $ git config core.sharedRepository group
>  $ git pull ~/w/git.git/ master
>  $ ls -l COPYING .git/index
>  -rw------- 1 jch eng 18765 Jan 11 07:48 COPYING
>  -rw-rw---- 1 jch eng 272037 Jan 11 07:48 .git/index
>
> we can see that the configuration affects only the $GIT_DIR/ files
> and not working tree.
>
> So you found a bug in clone, I think ;-)

Having said all that, the above does not mean that I'll refuse to
consider changing the semantics of core.sharedRepository in a future
major version bump by doing adjust_perm() for working tree files,
which we have deliberately chosen not to do in the current code.

But that is not within the scope of the patch we are discussing, and
I am not convinced it is a good idea (I haven't heard either sides
of arguments), so based on the current design, I think "we don't do
fopen_for_writing() for working tree files" is a valid justification
that is short-and-sweet for this patch.

[PATCH v3 0/2] Correctly handle transient files in shared repositories

[Johannes Schindelin]

Transient files, e.g. commit messages, are writable only by the owner,
even in shared repositories, to avoid interference between competing
users working on the same files.

These files are typically not deleted after use. As a consequence, we
have to delete such files before writing when they are owned by someone
else than the current user.

The only change relative to v2 is that the second commit message
clarifies why apply, fsck and fast-import are left unchanged.

Reported-by: Yaroslav Halchenko <yoh@onerussian.com>


Johannes Schindelin (2):
  commit: allow editing the commit message even in shared repos
  Handle more file writes correctly in shared repos

 builtin/commit.c      |  2 +-
 builtin/fast-export.c |  2 +-
 builtin/fetch.c       |  2 +-
 git-compat-util.h     |  1 +
 wrapper.c             | 13 +++++++++++++
 5 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v3 1/2] commit: allow editing the commit message even in shared repos

[Johannes Schindelin]

It was pointed out by Yaroslav Halchenko that the file containing the
commit message is writable only by the owner, which means that we have
to rewrite it from scratch in a shared repository.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/commit.c  |  2 +-
 git-compat-util.h |  1 +
 wrapper.c         | 13 +++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d054f84..89bf6ad 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -761,7 +761,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	s->fp = fopen(git_path(commit_editmsg), "w");
+	s->fp = fopen_for_writing(git_path(commit_editmsg));
 	if (s->fp == NULL)
 		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 2da0a75..e8f2867 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -733,6 +733,7 @@ extern int xmkstemp_mode(char *template, int mode);
 extern int odb_mkstemp(char *template, size_t limit, const char *pattern);
 extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1);
 extern char *xgetcwd(void);
+extern FILE *fopen_for_writing(const char *path);
 
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
 
diff --git a/wrapper.c b/wrapper.c
index b43d437..29a45d2 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -391,6 +391,19 @@ FILE *xfdopen(int fd, const char *mode)
 	return stream;
 }
 
+FILE *fopen_for_writing(const char *path)
+{
+	FILE *ret = fopen(path, "w");
+
+	if (!ret && errno == EPERM) {
+		if (!unlink(path))
+			ret = fopen(path, "w");
+		else
+			errno = EPERM;
+	}
+	return ret;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.6.3.windows.1.300.g1c25e49

[PATCH v3 2/2] Handle more file writes correctly in shared repos

[Johannes Schindelin]

In shared repositories, we have to be careful when writing files whose
permissions do not allow users other than the owner to write them.

In particular, we force the marks file of fast-export and the FETCH_HEAD
when fetching to be rewritten from scratch.

This commit does not touch the following users of fopen() that want to
write files:

- git am, when splitting mails (git-am correctly cleans up its directory
  after finishing, so there is no need to share those files between users)

- git apply, when writing rejected hunks (to be conservative, as it is not
  clear whether to write those files in shared mode or not)

- git fsck, when writing lost&found blobs (to be conservative, as it is
  not clear whether to write those files in shared mode or not)

- git merge-file, when writing merged files (when Git itself calls
  merge-file, the file in question was already there, with shared
  permissions).

- git submodule clone, when writing the .git file, because the file will
  not be overwritten

- git_terminal_prompt() in compat/terminal.c, because it is not writing to
  a file at all

- git diff --output, because the output file is clearly not intended to be
  shared between the users of the current repository

- git fast-import, when writing a crash report, because the reports' file
  names are unique due to an embedded process ID

- mailinfo() in mailinfo.c, because the output is clearly not intended to
  be shared between the users of the current repository

- check_or_regenerate_marks() in remote-testsvn.c, because this is only
  used for Git's internal testing

- git rerere, when writing resolved files, because the files in question
  were already written with the correct permissions

Note that this patch does not touch callers of write_file() and
write_file_gently(), which would benefit from the same scrutiny as to
usage in shared repositories. Most notable users: branch, daemon,
submodule & worktree, and a worrisome call in transport.c when updating
one ref (which ignores the shared flag).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fast-export.c | 2 +-
 builtin/fetch.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d9ac5d8..2471297 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -880,7 +880,7 @@ static void export_marks(char *file)
 	FILE *f;
 	int e = 0;
 
-	f = fopen(file, "w");
+	f = fopen_for_writing(file);
 	if (!f)
 		die_errno("Unable to open marks file %s for writing.", file);
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 586840d..33f04c1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -840,7 +840,7 @@ static void check_not_current_branch(struct ref *ref_map)
 static int truncate_fetch_head(void)
 {
 	const char *filename = git_path_fetch_head();
-	FILE *fp = fopen(filename, "w");
+	FILE *fp = fopen_for_writing(filename);
 
 	if (!fp)
 		return error(_("cannot open %s: %s\n"), filename, strerror(errno));
-- 
2.6.3.windows.1.300.g1c25e49

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Jeff King]

On Mon, Jan 11, 2016 at 07:35:42PM +0100, Johannes Schindelin wrote:

> Transient files, e.g. commit messages, are writable only by the owner,
> even in shared repositories, to avoid interference between competing
> users working on the same files.
> 
> These files are typically not deleted after use. As a consequence, we
> have to delete such files before writing when they are owned by someone
> else than the current user.
> 
> The only change relative to v2 is that the second commit message
> clarifies why apply, fsck and fast-import are left unchanged.

This looks fine to me. Even if if there are sites that could also use
conversion, this is a strict improvement, and we can revisit the other
ones in the future.

Thanks.

-Peff

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Transient files, e.g. commit messages, are writable only by the owner,
> even in shared repositories, to avoid interference between competing
> users working on the same files.
>
> These files are typically not deleted after use. As a consequence, we
> have to delete such files before writing when they are owned by someone
> else than the current user.
>
> The only change relative to v2 is that the second commit message
> clarifies why apply, fsck and fast-import are left unchanged.

I do not think it clarifies to make "fsck" whose lost and found are
written to GIT_DIR and "apply"'s ".rej" share the same "we dunno"
reasoning, though.

For "apply"'s .rej and "rerere"'s resolution, the reason why we
should not touch these codepaths is because we know we should not
muck with permission bits of working tree files (i.e. checkout
refrains from doing so, and these codepaths should be consistent),
not because it is not clear.  It _is_ clear we should not, and that
is because we don't.

I said "I am not convinced" but that is for future discussion of
possibly changing the behaviour of checkout and other things that
touch the working tree files, which hasn't even started.

Other than that, the resulting tree looks good to me, though.

Thanks.

> Reported-by: Yaroslav Halchenko <yoh@onerussian.com>
>
>
> Johannes Schindelin (2):
>   commit: allow editing the commit message even in shared repos
>   Handle more file writes correctly in shared repos
>
>  builtin/commit.c      |  2 +-
>  builtin/fast-export.c |  2 +-
>  builtin/fetch.c       |  2 +-
>  git-compat-util.h     |  1 +
>  wrapper.c             | 13 +++++++++++++
>  5 files changed, 17 insertions(+), 3 deletions(-)

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> The only change relative to v2 is that the second commit message
>> clarifies why apply, fsck and fast-import are left unchanged.
>
> I do not think it clarifies to make "fsck" whose lost and found are
> written to GIT_DIR and "apply"'s ".rej" share the same "we dunno"
> reasoning, though.

I'd say we should go with this one.  I think the reasoning for
"fsck" should be a lot clearer this way.

-- >8 --
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
Date:   Mon Jan 11 19:35:54 2016 +0100

    Handle more file writes correctly in shared repos
    
    In shared repositories, we have to be careful when writing files whose
    permissions do not allow users other than the owner to write them.
    
    In particular, we force the marks file of fast-export and the FETCH_HEAD
    when fetching to be rewritten from scratch.
    
    This commit does not touch other calls to fopen() that want to
    write files:
    
     - commands that write to working tree files (core.sharedRepository
       does not affect permission bits of working tree files),
       e.g. .rej file created by "apply --reject", result of applying a
       previous conflict resolution by "rerere", "git merge-file".
    
     - git am, when splitting mails (git-am correctly cleans up its directory
       after finishing, so there is no need to share those files between users)
    
     - git fsck, when writing lost&found blobs (they are written in the
       file under its object name, so an existing file with tighter
       permission that you cannot write into is OK, because what you are
       failing to write is the same contents that the file already has
       anyway).
    
     - git submodule clone, when writing the .git file, because the file
       will not be overwritten
    
     - git_terminal_prompt() in compat/terminal.c, because it is not writing to
       a file at all
    
     - git diff --output, because the output file is clearly not intended to be
       shared between the users of the current repository
    
     - git fast-import, when writing a crash report, because the reports' file
       names are unique due to an embedded process ID
    
     - mailinfo() in mailinfo.c, because the output is clearly not intended to
       be shared between the users of the current repository
    
     - check_or_regenerate_marks() in remote-testsvn.c, because this is only
       used for Git's internal testing
    
    Note that this patch does not touch callers of write_file() and
    write_file_gently(), which would benefit from the same scrutiny as
    to usage in shared repositories.  Most notable users are branch,
    daemon, submodule & worktree, and a worrisome call in transport.c
    when updating one ref (which ignores the shared flag).
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Jeff King]

On Mon, Jan 11, 2016 at 01:22:07PM -0800, Junio C Hamano wrote:

>      - git fsck, when writing lost&found blobs (they are written in the
>        file under its object name, so an existing file with tighter
>        permission that you cannot write into is OK, because what you are
>        failing to write is the same contents that the file already has
>        anyway).

I'm not sure I buy this argument. Yes, you should not be writing
anything else, but that does not change the fact that "fsck" will
unceremoniously abort:

  $ git init
  $ echo foo | git hash-object -w --stdin
  $ git fsck --lost-found
  notice: HEAD points to an unborn branch (master)
  Checking object directories: 100% (256/256), done.
  notice: No default references
  dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99

  $ chmod -w .git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99 
  $ $ git fsck --lost-found
  notice: HEAD points to an unborn branch (master)
  Checking object directories: 100% (256/256), done.
  notice: No default references
  dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99
  fatal: Could not open '.git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99': Permission denied

So I think this would be a reasonable candidate (or alternatively, to
treat EPERM on an existing file as a soft error). I am totally fine not
to address it as part of this series, though.

-Peff

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure I buy this argument. Yes, you should not be writing
> anything else, but that does not change the fact that "fsck" will
> unceremoniously abort:
> ...
> So I think this would be a reasonable candidate (or alternatively, to
> treat EPERM on an existing file as a soft error). I am totally fine not
> to address it as part of this series, though.

Yeah, that crossed my mind (and I agree with the conclusion).

Listing what is left deliberately and why is still a good idea, as
that would force people to think twice before wasting effort to
convert blindly without thinking.  Listing what is left behind like
"git fsck" that we know we shouldn't leave behind is even better to
mark low-hanging fruits.  How do you like this one instead?

     - git fsck, when writing lost&found blobs (this probably should
       be changed, but left as a low-hanging fruit for future
       contributors).

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Jeff King]

On Mon, Jan 11, 2016 at 01:54:05PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm not sure I buy this argument. Yes, you should not be writing
> > anything else, but that does not change the fact that "fsck" will
> > unceremoniously abort:
> > ...
> > So I think this would be a reasonable candidate (or alternatively, to
> > treat EPERM on an existing file as a soft error). I am totally fine not
> > to address it as part of this series, though.
> 
> Yeah, that crossed my mind (and I agree with the conclusion).
> 
> Listing what is left deliberately and why is still a good idea, as
> that would force people to think twice before wasting effort to
> convert blindly without thinking.  Listing what is left behind like
> "git fsck" that we know we shouldn't leave behind is even better to
> mark low-hanging fruits.  How do you like this one instead?
> 
>      - git fsck, when writing lost&found blobs (this probably should
>        be changed, but left as a low-hanging fruit for future
>        contributors).

I think that's more accurate. :)

-Peff

Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

[Johannes Schindelin]

Hi,

On Mon, 11 Jan 2016, Jeff King wrote:

> On Mon, Jan 11, 2016 at 01:54:05PM -0800, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > I'm not sure I buy this argument. Yes, you should not be writing
> > > anything else, but that does not change the fact that "fsck" will
> > > unceremoniously abort:
> > > ...
> > > So I think this would be a reasonable candidate (or alternatively, to
> > > treat EPERM on an existing file as a soft error). I am totally fine not
> > > to address it as part of this series, though.
> > 
> > Yeah, that crossed my mind (and I agree with the conclusion).
> > 
> > Listing what is left deliberately and why is still a good idea, as
> > that would force people to think twice before wasting effort to
> > convert blindly without thinking.  Listing what is left behind like
> > "git fsck" that we know we shouldn't leave behind is even better to
> > mark low-hanging fruits.  How do you like this one instead?
> > 
> >      - git fsck, when writing lost&found blobs (this probably should
> >        be changed, but left as a low-hanging fruit for future
> >        contributors).
> 
> I think that's more accurate. :)

Fine by me!

Ciao,
Dscho