[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[list]

From: Christian Hesse <mail at eworm.de>

Signed-off-by: Christian Hesse <mail at eworm.de>
---
 scan-tree.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scan-tree.c b/scan-tree.c
index 2e87999..bc17d14 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -121,7 +121,11 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
 	config_fn = fn;
 	if (ctx.cfg.enable_git_config) {
 		strbuf_addstr(path, "config");
-		git_config_from_file(gitconfig_config, path->buf, NULL);
+		if (git_config_from_file(gitconfig_config, path->buf, NULL)) {
+			fprintf(stderr, "Error reading config %s: %s (%d)\n",
+				path->buf, strerror(errno), errno);
+			return;
+		}
 		strbuf_setlen(path, pathlen);
 	}
 
-- 
2.7.2

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[Jason]

Are repos without .git/config necessarily invalid?

zx2c4 at thinkpad ~ $ mkdir abcd
zx2c4 at thinkpad ~ $ cd abcd
zx2c4 at thinkpad ~/abcd $ git init
Initialized empty Git repository in /home/zx2c4/abcd/.git/
zx2c4 at thinkpad ~/abcd $ touch file
zx2c4 at thinkpad ~/abcd $ git add file
zx2c4 at thinkpad ~/abcd $ git commit -m first
[master (root-commit) 55749f6] first
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
zx2c4 at thinkpad ~/abcd $ rm .git/config
zx2c4 at thinkpad ~/abcd $ git status
On branch master
nothing to commit, working directory clean

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[john]

On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:
> From: Christian Hesse <mail at eworm.de>
> 
> Signed-off-by: Christian Hesse <mail at eworm.de>

Is this solving a particular problem or did you just notice that the
return value is ignored?

I don't think returning when this fails is correct because we've already
added the repository to the list by this point and a lot of the
remaining code in this function will do something sensible even if
git_config_from_file() fails.

In fact, git_config_from_file() sets the die_on_error flag for
do_config_from() so the only case that gives us an error here is if the
config file cannot be opened.  I don't think it's unreasonable to print
an error if that happens but bailing out of the function at this point
is wrong.

> ---
>  scan-tree.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/scan-tree.c b/scan-tree.c
> index 2e87999..bc17d14 100644
> --- a/scan-tree.c
> +++ b/scan-tree.c
> @@ -121,7 +121,11 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
>  	config_fn = fn;
>  	if (ctx.cfg.enable_git_config) {
>  		strbuf_addstr(path, "config");
> -		git_config_from_file(gitconfig_config, path->buf, NULL);
> +		if (git_config_from_file(gitconfig_config, path->buf, NULL)) {
> +			fprintf(stderr, "Error reading config %s: %s (%d)\n",
> +				path->buf, strerror(errno), errno);
> +			return;
> +		}
>  		strbuf_setlen(path, pathlen);
>  	}
>  
> -- 
> 2.7.2

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[list]

John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:
> On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:
> > From: Christian Hesse <mail at eworm.de>
> > 
> > Signed-off-by: Christian Hesse <mail at eworm.de>  
> 
> Is this solving a particular problem or did you just notice that the
> return value is ignored?
> 
> I don't think returning when this fails is correct because we've already
> added the repository to the list by this point and a lot of the
> remaining code in this function will do something sensible even if
> git_config_from_file() fails.
> 
> In fact, git_config_from_file() sets the die_on_error flag for
> do_config_from() so the only case that gives us an error here is if the
> config file cannot be opened.  I don't think it's unreasonable to print
> an error if that happens but bailing out of the function at this point
> is wrong.

Ok, probably you are right...

Actually I do have a particular problem, but it is not solved by
this patch. :-p Just stumbled and thought it is a good idea.

I have a repository that has a config with bad permissions, so http server's
user can not read it. cgit does not print http headers and http server bails
out with error 500. What path does it take?
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160308/5dbaf578/attachment.asc>

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[john]

On Tue, Mar 08, 2016 at 03:26:23PM +0100, Christian Hesse wrote:
> John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:
> > On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:
> > > From: Christian Hesse <mail at eworm.de>
> > > 
> > > Signed-off-by: Christian Hesse <mail at eworm.de>  
> > 
> > Is this solving a particular problem or did you just notice that the
> > return value is ignored?
> > 
> > I don't think returning when this fails is correct because we've already
> > added the repository to the list by this point and a lot of the
> > remaining code in this function will do something sensible even if
> > git_config_from_file() fails.
> > 
> > In fact, git_config_from_file() sets the die_on_error flag for
> > do_config_from() so the only case that gives us an error here is if the
> > config file cannot be opened.  I don't think it's unreasonable to print
> > an error if that happens but bailing out of the function at this point
> > is wrong.
> 
> Ok, probably you are right...
> 
> Actually I do have a particular problem, but it is not solved by
> this patch. :-p Just stumbled and thought it is a good idea.
> 
> I have a repository that has a config with bad permissions, so http server's
> user can not read it. cgit does not print http headers and http server bails
> out with error 500. What path does it take?

Hmm... bad permissions should result in fopen(2) failing, which would
take the path altered in your patch.

Can you run cgit as the http server's user from a terminal like this:

	CGIT_CONFIG=/path/to/cgitrc QUERY_STRING=url=/ cgit

?  It might produce an error message that your http server isn't
logging.

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[list]

John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:36:
> On Tue, Mar 08, 2016 at 03:26:23PM +0100, Christian Hesse wrote:
> > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:  
> > > On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:  
> > > > From: Christian Hesse <mail at eworm.de>
> > > > 
> > > > Signed-off-by: Christian Hesse <mail at eworm.de>    
> > > 
> > > Is this solving a particular problem or did you just notice that the
> > > return value is ignored?
> > > 
> > > I don't think returning when this fails is correct because we've already
> > > added the repository to the list by this point and a lot of the
> > > remaining code in this function will do something sensible even if
> > > git_config_from_file() fails.
> > > 
> > > In fact, git_config_from_file() sets the die_on_error flag for
> > > do_config_from() so the only case that gives us an error here is if the
> > > config file cannot be opened.  I don't think it's unreasonable to print
> > > an error if that happens but bailing out of the function at this point
> > > is wrong.  
> > 
> > Ok, probably you are right...
> > 
> > Actually I do have a particular problem, but it is not solved by
> > this patch. :-p Just stumbled and thought it is a good idea.
> > 
> > I have a repository that has a config with bad permissions, so http
> > server's user can not read it. cgit does not print http headers and http
> > server bails out with error 500. What path does it take?  
> 
> Hmm... bad permissions should result in fopen(2) failing, which would
> take the path altered in your patch.
> 
> Can you run cgit as the http server's user from a terminal like this:
> 
> 	CGIT_CONFIG=/path/to/cgitrc QUERY_STRING=url=/ cgit
> 
> ?  It might produce an error message that your http server isn't
> logging.

It gives:

Error reading config /path/to/repository.git/config: Permission denied (13)
fatal: unable to access '/path/to/repository.git/config': Permission denied

Looks like this is fatal in git...
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160308/4a3065a3/attachment.asc>

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[john]

On Tue, Mar 08, 2016 at 03:54:22PM +0100, Christian Hesse wrote:
> John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:36:
> > On Tue, Mar 08, 2016 at 03:26:23PM +0100, Christian Hesse wrote:
> > > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:  
> > > > On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:  
> > > > > From: Christian Hesse <mail at eworm.de>
> > > > > 
> > > > > Signed-off-by: Christian Hesse <mail at eworm.de>    
> > > > 
> > > > Is this solving a particular problem or did you just notice that the
> > > > return value is ignored?
> > > > 
> > > > I don't think returning when this fails is correct because we've already
> > > > added the repository to the list by this point and a lot of the
> > > > remaining code in this function will do something sensible even if
> > > > git_config_from_file() fails.
> > > > 
> > > > In fact, git_config_from_file() sets the die_on_error flag for
> > > > do_config_from() so the only case that gives us an error here is if the
> > > > config file cannot be opened.  I don't think it's unreasonable to print
> > > > an error if that happens but bailing out of the function at this point
> > > > is wrong.  
> > > 
> > > Ok, probably you are right...
> > > 
> > > Actually I do have a particular problem, but it is not solved by
> > > this patch. :-p Just stumbled and thought it is a good idea.
> > > 
> > > I have a repository that has a config with bad permissions, so http
> > > server's user can not read it. cgit does not print http headers and http
> > > server bails out with error 500. What path does it take?  
> > 
> > Hmm... bad permissions should result in fopen(2) failing, which would
> > take the path altered in your patch.
> > 
> > Can you run cgit as the http server's user from a terminal like this:
> > 
> > 	CGIT_CONFIG=/path/to/cgitrc QUERY_STRING=url=/ cgit
> > 
> > ?  It might produce an error message that your http server isn't
> > logging.
> 
> It gives:
> 
> Error reading config /path/to/repository.git/config: Permission denied (13)
> fatal: unable to access '/path/to/repository.git/config': Permission denied
> 
> Looks like this is fatal in git...

Is that when listing repositories or when trying to display that
specific repo?  I suspect that's a result of prepare_repo_cmd() setting
up for a specific repository.

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[list]

John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:59:
> On Tue, Mar 08, 2016 at 03:54:22PM +0100, Christian Hesse wrote:
> > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:36:  
> > > On Tue, Mar 08, 2016 at 03:26:23PM +0100, Christian Hesse wrote:  
> > > > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:    
> > > > > On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:    
> > > > > > From: Christian Hesse <mail at eworm.de>
> > > > > > 
> > > > > > Signed-off-by: Christian Hesse <mail at eworm.de>      
> > > > > 
> > > > > Is this solving a particular problem or did you just notice that the
> > > > > return value is ignored?
> > > > > 
> > > > > I don't think returning when this fails is correct because we've
> > > > > already added the repository to the list by this point and a lot of
> > > > > the remaining code in this function will do something sensible even
> > > > > if git_config_from_file() fails.
> > > > > 
> > > > > In fact, git_config_from_file() sets the die_on_error flag for
> > > > > do_config_from() so the only case that gives us an error here is if
> > > > > the config file cannot be opened.  I don't think it's unreasonable
> > > > > to print an error if that happens but bailing out of the function
> > > > > at this point is wrong.    
> > > > 
> > > > Ok, probably you are right...
> > > > 
> > > > Actually I do have a particular problem, but it is not solved by
> > > > this patch. :-p Just stumbled and thought it is a good idea.
> > > > 
> > > > I have a repository that has a config with bad permissions, so http
> > > > server's user can not read it. cgit does not print http headers and
> > > > http server bails out with error 500. What path does it take?    
> > > 
> > > Hmm... bad permissions should result in fopen(2) failing, which would
> > > take the path altered in your patch.
> > > 
> > > Can you run cgit as the http server's user from a terminal like this:
> > > 
> > > 	CGIT_CONFIG=/path/to/cgitrc QUERY_STRING=url=/ cgit
> > > 
> > > ?  It might produce an error message that your http server isn't
> > > logging.  
> > 
> > It gives:
> > 
> > Error reading config /path/to/repository.git/config: Permission denied
> > (13) fatal: unable to access '/path/to/repository.git/config': Permission
> > denied
> > 
> > Looks like this is fatal in git...  
> 
> Is that when listing repositories or when trying to display that
> specific repo?  I suspect that's a result of prepare_repo_cmd() setting
> up for a specific repository.

Listing repositories works. This is when displaying the repository with
bad/denied config file.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160308/d3490c1e/attachment.asc>

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[john]

On Tue, Mar 08, 2016 at 04:01:59PM +0100, Christian Hesse wrote:
> John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:59:
> > On Tue, Mar 08, 2016 at 03:54:22PM +0100, Christian Hesse wrote:
> > > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:36:  
> > > > On Tue, Mar 08, 2016 at 03:26:23PM +0100, Christian Hesse wrote:  
> > > > > John Keeping <john at keeping.me.uk> on Tue, 2016/03/08 14:11:    
> > > > > > On Tue, Mar 08, 2016 at 02:51:46PM +0100, Christian Hesse wrote:    
> > > > > > > From: Christian Hesse <mail at eworm.de>
> > > > > > > 
> > > > > > > Signed-off-by: Christian Hesse <mail at eworm.de>      
> > > > > > 
> > > > > > Is this solving a particular problem or did you just notice that the
> > > > > > return value is ignored?
> > > > > > 
> > > > > > I don't think returning when this fails is correct because we've
> > > > > > already added the repository to the list by this point and a lot of
> > > > > > the remaining code in this function will do something sensible even
> > > > > > if git_config_from_file() fails.
> > > > > > 
> > > > > > In fact, git_config_from_file() sets the die_on_error flag for
> > > > > > do_config_from() so the only case that gives us an error here is if
> > > > > > the config file cannot be opened.  I don't think it's unreasonable
> > > > > > to print an error if that happens but bailing out of the function
> > > > > > at this point is wrong.    
> > > > > 
> > > > > Ok, probably you are right...
> > > > > 
> > > > > Actually I do have a particular problem, but it is not solved by
> > > > > this patch. :-p Just stumbled and thought it is a good idea.
> > > > > 
> > > > > I have a repository that has a config with bad permissions, so http
> > > > > server's user can not read it. cgit does not print http headers and
> > > > > http server bails out with error 500. What path does it take?    
> > > > 
> > > > Hmm... bad permissions should result in fopen(2) failing, which would
> > > > take the path altered in your patch.
> > > > 
> > > > Can you run cgit as the http server's user from a terminal like this:
> > > > 
> > > > 	CGIT_CONFIG=/path/to/cgitrc QUERY_STRING=url=/ cgit
> > > > 
> > > > ?  It might produce an error message that your http server isn't
> > > > logging.  
> > > 
> > > It gives:
> > > 
> > > Error reading config /path/to/repository.git/config: Permission denied
> > > (13) fatal: unable to access '/path/to/repository.git/config': Permission
> > > denied
> > > 
> > > Looks like this is fatal in git...  
> > 
> > Is that when listing repositories or when trying to display that
> > specific repo?  I suspect that's a result of prepare_repo_cmd() setting
> > up for a specific repository.
> 
> Listing repositories works. This is when displaying the repository with
> bad/denied config file.

I don't think we can work around that without adding a load of extra
checks before calling setup_git_directory_gently(), which would allow us
to at least print a more obvious error message.  The problem is that
git_config_early() dies if the file cannot be accessed and there is no
way for us to read from a repository without going through that
function.

[PATCH 1/1] scan-tree: handle error in git_config_from_file()

[list]

Christian Hesse <list at eworm.de> on Tue, 2016/03/08 16:01:
> > > It gives:
> > > 
> > > Error reading config /path/to/repository.git/config: Permission denied
> > > (13) fatal: unable to access '/path/to/repository.git/config':
> > > Permission denied
> > > 
> > > Looks like this is fatal in git...    
> > 
> > Is that when listing repositories or when trying to display that
> > specific repo?  I suspect that's a result of prepare_repo_cmd() setting
> > up for a specific repository.  
> 
> Listing repositories works. This is when displaying the repository with
> bad/denied config file.

Looks like it dies in cgit.c line 577 in:

setup_git_directory_gently(&nongit);
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160308/8fd980fa/attachment.asc>

[PATCH v2 1/1] scan-tree: handle error in git_config_from_file()

[list]

From: Christian Hesse <mail at eworm.de>

We can live without config, but print an error if it is not readable.
---
 scan-tree.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scan-tree.c b/scan-tree.c
index 2e87999..bc17d14 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -121,7 +121,10 @@ static void add_repo(const char *base, struct strbuf *path, repo_config_fn fn)
 	config_fn = fn;
 	if (ctx.cfg.enable_git_config) {
 		strbuf_addstr(path, "config");
-		git_config_from_file(gitconfig_config, path->buf, NULL);
+		if (git_config_from_file(gitconfig_config, path->buf, NULL)) {
+			fprintf(stderr, "Error reading config %s: %s (%d)\n",
+				path->buf, strerror(errno), errno);
+		}
 		strbuf_setlen(path, pathlen);
 	}
 
-- 
2.7.2

[PATCH v2 1/1] scan-tree: handle error in git_config_from_file()

[Jason]

Are repos without .git/config necessarily invalid?

zx2c4 at thinkpad ~ $ mkdir abcd
zx2c4 at thinkpad ~ $ cd abcd
zx2c4 at thinkpad ~/abcd $ git init
Initialized empty Git repository in /home/zx2c4/abcd/.git/
zx2c4 at thinkpad ~/abcd $ touch file
zx2c4 at thinkpad ~/abcd $ git add file
zx2c4 at thinkpad ~/abcd $ git commit -m first
[master (root-commit) 55749f6] first
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file
zx2c4 at thinkpad ~/abcd $ rm .git/config
zx2c4 at thinkpad ~/abcd $ git status
On branch master
nothing to commit, working directory clean

[PATCH v2 1/1] scan-tree: handle error in git_config_from_file()

[list]

"Jason A. Donenfeld" <Jason at zx2c4.com> on Wed, 2016/03/09 12:05:
> Are repos without .git/config necessarily invalid?

No. But repositories with config file available but permission denied are.
Looks like we can not see the difference here...
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160311/2d166f4d/attachment.asc>