Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fetch: allow running as different users in shared repositories
@ 2020-03-19  1:03 Vadim Zeitlin
  2020-03-25 19:04 ` Johannes Schindelin
  0 siblings, 1 reply; 4+ messages in thread
From: Vadim Zeitlin @ 2020-03-19  1:03 UTC (permalink / raw)
  To: git; +Cc: Vadim Zeitlin, Johannes Schindelin

The function fopen_for_writing(), which was added in 79d7582e32 (commit:
allow editing the commit message even in shared repos, 2016-01-06) and
used for overwriting FETCH_HEAD since ea56518dfe (Handle more file
writes correctly in shared repos, 2016-01-11), didn't do it correctly in
shared repositories under Linux.

This happened because in this situation the file FETCH_HEAD has mode 644
and attempting to overwrite it when running git-fetch under an account
different from the one that was had originally created it, failed with
EACCES, and not EPERM. However fopen_for_writing() only checked for the
latter, and not the former, so it didn't even try removing the existing
file and recreating it, as it was supposed to do.

Fix this by checking for either EACCES or EPERM. The latter doesn't seem
to be ever returned in a typical situation by open(2) under Linux, but
keep checking for it as it is presumably returned under some other
platform, although it's not really clear where does this happen.

Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org>
---
I couldn't find any system that would return EPERM for a "normal"
permissions denied error, so maybe it's not worth checking for it, but I
wanted to minimize the number of changes to the existing behaviour. At the
very least, testing for EACCES is definitely necessary under Linux, where
openat(2) returns it, and not EPERM, in the situation described above, i.e.
non-writable file (even if it's in a writable directory, allowing to unlink
it without problems).
---
 wrapper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index e1eaef2e16..f5607241da 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -373,11 +373,12 @@ FILE *fopen_for_writing(const char *path)
 {
 	FILE *ret = fopen(path, "w");
 
-	if (!ret && errno == EPERM) {
+	if (!ret && (errno == EACCES || errno == EPERM)) {
+		int open_error = errno;
 		if (!unlink(path))
 			ret = fopen(path, "w");
 		else
-			errno = EPERM;
+			errno = open_error;
 	}
 	return ret;
 }
-- 
2.26.0.rc2

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fetch: allow running as different users in shared repositories
  2020-03-19  1:03 [PATCH] fetch: allow running as different users in shared repositories Vadim Zeitlin
@ 2020-03-25 19:04 ` Johannes Schindelin
  0 siblings, 0 replies; 4+ messages in thread
From: Johannes Schindelin @ 2020-03-25 19:04 UTC (permalink / raw)
  To: Vadim Zeitlin; +Cc: git

Hi Vadim,

On Thu, 19 Mar 2020, Vadim Zeitlin wrote:

> The function fopen_for_writing(), which was added in 79d7582e32 (commit:
> allow editing the commit message even in shared repos, 2016-01-06) and
> used for overwriting FETCH_HEAD since ea56518dfe (Handle more file
> writes correctly in shared repos, 2016-01-11), didn't do it correctly in
> shared repositories under Linux.
>
> This happened because in this situation the file FETCH_HEAD has mode 644

I wonder why that is. In a shared repository, it should have mode 664, I
thought.

> and attempting to overwrite it when running git-fetch under an account
> different from the one that was had originally created it, failed with
> EACCES, and not EPERM. However fopen_for_writing() only checked for the
> latter, and not the former, so it didn't even try removing the existing
> file and recreating it, as it was supposed to do.
>
> Fix this by checking for either EACCES or EPERM. The latter doesn't seem
> to be ever returned in a typical situation by open(2) under Linux, but
> keep checking for it as it is presumably returned under some other
> platform, although it's not really clear where does this happen.
>
> Signed-off-by: Vadim Zeitlin <vz-git@zeitlins.org>
> ---
> I couldn't find any system that would return EPERM for a "normal"
> permissions denied error, so maybe it's not worth checking for it, but I
> wanted to minimize the number of changes to the existing behaviour. At the
> very least, testing for EACCES is definitely necessary under Linux, where
> openat(2) returns it, and not EPERM, in the situation described above, i.e.
> non-writable file (even if it's in a writable directory, allowing to unlink
> it without problems).

That rationale makes sense to me, as does the patch.

Thanks,
Johannes

> ---
>  wrapper.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index e1eaef2e16..f5607241da 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -373,11 +373,12 @@ FILE *fopen_for_writing(const char *path)
>  {
>  	FILE *ret = fopen(path, "w");
>
> -	if (!ret && errno == EPERM) {
> +	if (!ret && (errno == EACCES || errno == EPERM)) {
> +		int open_error = errno;
>  		if (!unlink(path))
>  			ret = fopen(path, "w");
>  		else
> -			errno = EPERM;
> +			errno = open_error;
>  	}
>  	return ret;
>  }
> --
> 2.26.0.rc2
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fetch: allow running as different users in shared repositories
  2020-03-26  0:44 Vadim Zeitlin
@ 2020-03-26 14:40 ` Johannes Schindelin
  0 siblings, 0 replies; 4+ messages in thread
From: Johannes Schindelin @ 2020-03-26 14:40 UTC (permalink / raw)
  To: Vadim Zeitlin; +Cc: git

Hi Vadim,

On Thu, 26 Mar 2020, Vadim Zeitlin wrote:

> On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:
>
> JS> Hi Vadim,
>
>  Hello Johannes and thanks for your reply!
>
> JS> On Thu, 19 Mar 2020, Vadim Zeitlin wrote:
> JS>
> JS> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:
> JS> > allow editing the commit message even in shared repos, 2016-01-06) and
> JS> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file
> JS> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in
> JS> > shared repositories under Linux.
> JS> >
> JS> > This happened because in this situation the file FETCH_HEAD has mode 644
> JS>
> JS> I wonder why that is. In a shared repository, it should have mode 664, I
> JS> thought.
>
>  This file is created using a simple fopen("w") and so is subject to umask.
> With the usual default umask value (022) its mode would be 644, regardless
> of the repository settings.

Maybe we should change that to an `open()` call with the explicit `0666`
mode?

> [...snip my original description...]
> JS> That rationale makes sense to me, as does the patch.
>
>  Sorry for a possibly stupid question, but what is the next thing to do
> now? The instructions in Documentation/SubmittingPatches indicate that I
> should wait until the "list forms consensus that [...] your patch is good",
> but it's not quite clear what indicates that a consensus has been reached.
> Is your comment above enough or should I wait for something else? And
> if/when it has been reached, do I really I need to resend the patch to
> the maintainer and cc the list as written in that document? I'm a bit
> surprised by this because I don't see (most) patches being resent to this
> list.

My take is that this was waiting for a review, and I provided it (*not*
asking for any changes), and if there are no further reviews, the patch
should make it into the `pu` branch, then `next` and eventually `master`,
at which point it will be slated for the next official `.0` version.

It might make sense to ask for it to be trickled down into the `maint`
branch, too, in case a `v2.26.1` is released. I would be in favor of that,
but would not do the asking myself ;-)

Ciao,
Johannes

>
>  This is obviously very non-urgent, but I'd just like to understand what,
> if anything, is expected from me.
>
>  Thanks in advance for your guidance!
> VZ
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fetch: allow running as different users in shared repositories
@ 2020-03-26  0:44 Vadim Zeitlin
  2020-03-26 14:40 ` Johannes Schindelin
  0 siblings, 1 reply; 4+ messages in thread
From: Vadim Zeitlin @ 2020-03-26  0:44 UTC (permalink / raw)
  To: johannes.schindelin; +Cc: git

On Wed, 25 Mar 2020 20:04:09 +0100 Johannes Schindelin wrote:

JS> Hi Vadim,

 Hello Johannes and thanks for your reply!

JS> On Thu, 19 Mar 2020, Vadim Zeitlin wrote:
JS> 
JS> > The function fopen_for_writing(), which was added in 79d7582e32 (commit:
JS> > allow editing the commit message even in shared repos, 2016-01-06) and
JS> > used for overwriting FETCH_HEAD since ea56518dfe (Handle more file
JS> > writes correctly in shared repos, 2016-01-11), didn't do it correctly in
JS> > shared repositories under Linux.
JS> >
JS> > This happened because in this situation the file FETCH_HEAD has mode 644
JS> 
JS> I wonder why that is. In a shared repository, it should have mode 664, I
JS> thought.

 This file is created using a simple fopen("w") and so is subject to umask.
With the usual default umask value (022) its mode would be 644, regardless
of the repository settings.

[...snip my original description...]
JS> That rationale makes sense to me, as does the patch.

 Sorry for a possibly stupid question, but what is the next thing to do
now? The instructions in Documentation/SubmittingPatches indicate that I
should wait until the "list forms consensus that [...] your patch is good",
but it's not quite clear what indicates that a consensus has been reached.
Is your comment above enough or should I wait for something else? And
if/when it has been reached, do I really I need to resend the patch to
the maintainer and cc the list as written in that document? I'm a bit
surprised by this because I don't see (most) patches being resent to this
list.

 This is obviously very non-urgent, but I'd just like to understand what,
if anything, is expected from me.

 Thanks in advance for your guidance!
VZ

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-19  1:03 [PATCH] fetch: allow running as different users in shared repositories Vadim Zeitlin
2020-03-25 19:04 ` Johannes Schindelin
2020-03-26  0:44 Vadim Zeitlin
2020-03-26 14:40 ` Johannes Schindelin

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git