{"thread":{"id":"60713","subject":"Problem in gitweb.cgi","startedAt":"2024-01-10T00:44:01Z","lastAt":"2024-01-11T02:15:23Z","messageCount":5,"participants":["Marcelo Roberto Jimenez","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486480","messageId":"CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@mail.gmail.com","threadId":"60713","inReplyTo":null,"subject":"Problem in gitweb.cgi","fromName":"Marcelo Roberto Jimenez","fromEmail":"marcelo.jimenez@gmail.com","sentAt":"2024-01-10T00:43:33Z","receivedAt":"2024-01-10T00:44:01Z","isPatch":false,"sender":{"key":"marcelo.jimenez@gmail.com","avatar":"https://avatars.githubusercontent.com/u/292484?v=4"},"body":"Hi,\n\nI have recently run into a problem with gitweb that was triggered by\nmy distribution, OpenSUSE Tumbleweed. Due to a misconfiguration of the\napplication AppArmour, \"git instaweb\" was having problems in accessing\nthe configuration file \"/etc/gitweb-common.conf\". That was half of the\nproblem and has been reported downstream here:\nhttps://bugzilla.suse.com/show_bug.cgi?id=1218664\n\nThe other half of the problem is in gitweb.cgi itself. There is a\nlogic to select which configuration file is going to be used. That\nlogic is ok, although a bit unclear from the documentation. Reading\nthe code I understood that $GITWEB_CONFIG_COMMON (usually\n/etc/gitweb-common.conf) should always be read if it exists, and\n$GITWEB_CONFIG_SYSTEM (usually /etc/gitweb.conf) will never be read if\n$GITWEB_CONFIG has been read before.\n\nThe problem is that $GITWEB_CONFIG_COMMON was never read, even though\nthe code that reads it was being called before the code that reads the\nother two files. As I said before, the problem was caused by a lack of\npermission in AppArmour, but gitweb should have been able to catch the\nerror. By not signaling it properly, users get lost because the\nproblem is a little tricky to debug.\n\nAfter some \"printf\" debugging, I converged to this routine, line 728\nof gitweb.cgi:\n\n# read and parse gitweb config file given by its parameter.\n# returns true on success, false on recoverable error, allowing\n# to chain this subroutine, using first file that exists.\n# dies on errors during parsing config file, as it is unrecoverable.\nsub read_config_file {\n        my $filename = shift;\n        return unless defined $filename;\n        # die if there are errors parsing config file\n        if (-e $filename) {\n                do $filename;\n                die $@ if $@;\n                return 1;\n        }\n        return;\n}\n\nPerl uses two different variables to manage errors from a \"do\". One is\n\"$@\", which is set in this case when do is unable to compile the file.\nThe other is $!, which is set in case \"do\" cannot read the file. By\nprinting the value of $! I found out that it was set to \"Permission\ndenied\". Since the script does not currently test for $!, the error\ngoes unnoticed.\n\nI suppose a proper fix would be to put a line before the test for $@,\nlike \"die $! if $!\".\n\nFor the curious, I have a report explaining the full problem here, but\nthe part relevant to gitweb is in this e-mail:\nhttps://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n\n\nBest regards,\nMarcelo.\n"},{"id":"486568","messageId":"20240110220506.27491-1-marcelo.jimenez@gmail.com","threadId":"60713","inReplyTo":"CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@mail.gmail.com","subject":"[PATCH] gitweb: Fixes error handling when reading configuration","fromName":"Marcelo Roberto Jimenez","fromEmail":"marcelo.jimenez@gmail.com","sentAt":"2024-01-10T22:05:06Z","receivedAt":"2024-01-10T22:05:14Z","isPatch":true,"sender":{"key":"marcelo.jimenez@gmail.com","avatar":"https://avatars.githubusercontent.com/u/292484?v=4"},"body":"This patch fixes a possibility of a permission to access error go\nunnoticed.\n\nPerl uses two different variables to manage errors from a do. One\nis $@, which is set in this case when do is unable to compile the\nfile. The other is $!, which is set in case do cannot read the file.\nBy printing the value of $! I found out that it was set to Permission\ndenied. Since the script does not currently test for $!, the error\ngoes unnoticed.\n\nPerl do block documentation: https://perldoc.perl.org/functions/do\n\nSigned-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>\n---\n gitweb/gitweb.perl | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e66eb3d9ba..47577ec566 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -728,9 +728,11 @@ sub filter_and_validate_refs {\n sub read_config_file {\n \tmy $filename = shift;\n \treturn unless defined $filename;\n-\t# die if there are errors parsing config file\n \tif (-e $filename) {\n \t\tdo $filename;\n+\t\t#die if there is a problem accessing the file\n+\t\tdie $! if $!;\n+\t\t# die if there are errors parsing config file\n \t\tdie $@ if $@;\n \t\treturn 1;\n \t}\n-- \n2.43.0\n\n"},{"id":"486575","messageId":"20240110225709.30168-1-marcelo.jimenez@gmail.com","threadId":"60713","inReplyTo":"CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@mail.gmail.com","subject":"[PATCH v2] gitweb: Fixes error handling when reading configuration","fromName":"Marcelo Roberto Jimenez","fromEmail":"marcelo.jimenez@gmail.com","sentAt":"2024-01-10T22:57:09Z","receivedAt":"2024-01-10T22:57:15Z","isPatch":true,"sender":{"key":"marcelo.jimenez@gmail.com","avatar":"https://avatars.githubusercontent.com/u/292484?v=4"},"body":"This patch fixes a possibility of a permission to access error go\nunnoticed.\n\nPerl uses two different variables to manage errors from a do. One\nis $@, which is set in this case when do is unable to compile the\nfile. The other is $!, which is set in case do cannot read the file.\nBy printing the value of $! I found out that it was set to Permission\ndenied. Since the script does not currently test for $!, the error\ngoes unnoticed.\n\nPerl do block documentation: https://perldoc.perl.org/functions/do\n\nSigned-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>\n---\n gitweb/gitweb.perl | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex e66eb3d9ba..5d0122020f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -728,9 +728,11 @@ sub filter_and_validate_refs {\n sub read_config_file {\n \tmy $filename = shift;\n \treturn unless defined $filename;\n-\t# die if there are errors parsing config file\n \tif (-e $filename) {\n \t\tdo $filename;\n+\t\t# die if there is a problem accessing the file\n+\t\tdie $! if $!;\n+\t\t# die if there are errors parsing config file\n \t\tdie $@ if $@;\n \t\treturn 1;\n \t}\n\nbase-commit: 564d0252ca632e0264ed670534a51d18a689ef5d\n-- \n2.43.0\n\n"},{"id":"486581","messageId":"xmqqzfxc7mfh.fsf@gitster.g","threadId":"60713","inReplyTo":"20240110225709.30168-1-marcelo.jimenez@gmail.com","subject":"Re: [PATCH v2] gitweb: Fixes error handling when reading configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-11T00:17:54Z","receivedAt":"2024-01-11T00:17:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> writes:\n\n> This patch fixes a possibility of a permission to access error go\n> unnoticed.\n>\n> Perl uses two different variables to manage errors from a do. One\n> is $@, which is set in this case when do is unable to compile the\n> file. The other is $!, which is set in case do cannot read the file.\n> By printing the value of $! I found out that it was set to Permission\n> denied. Since the script does not currently test for $!, the error\n> goes unnoticed.\n\nWell explained how the current code behaves and why.\n\nWith my devil's advocate hat on, I suspect that the current\nbehaviour comes from the wish to see \"file exists but unreadable\"\nand \"the named file does not exist\" behave the same way.  If you\npass the name of a configuration file that does not exist, however,\nthe codeblock to die does not trigger at all.  If a file does exist\nbut unreadable, to gitweb, it is just as good as having no file to\nread configuration data from---either way, it cannot use anything\nuseful from the named file.  So returning silently, which is the\n\"bug\" you are fixing, does not sound too bad.\n\nI dunno.  Let's queue and see what others would say.\n\nThanks.\n\n> Perl do block documentation: https://perldoc.perl.org/functions/do\n>\n> Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>\n> ---\n>  gitweb/gitweb.perl | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index e66eb3d9ba..5d0122020f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -728,9 +728,11 @@ sub filter_and_validate_refs {\n>  sub read_config_file {\n>  \tmy $filename = shift;\n>  \treturn unless defined $filename;\n> -\t# die if there are errors parsing config file\n>  \tif (-e $filename) {\n>  \t\tdo $filename;\n> +\t\t# die if there is a problem accessing the file\n> +\t\tdie $! if $!;\n> +\t\t# die if there are errors parsing config file\n>  \t\tdie $@ if $@;\n>  \t\treturn 1;\n>  \t}\n"},{"id":"486588","messageId":"CACjc_5rgO5Zcxndawm=3eRUX1Tuqh5LT05LhK+4V45RmUWmQHw@mail.gmail.com","threadId":"60713","inReplyTo":"xmqqzfxc7mfh.fsf@gitster.g","subject":"Re: [PATCH v2] gitweb: Fixes error handling when reading configuration","fromName":"Marcelo Roberto Jimenez","fromEmail":"marcelo.jimenez@gmail.com","sentAt":"2024-01-11T02:14:55Z","receivedAt":"2024-01-11T02:15:23Z","isPatch":true,"sender":{"key":"marcelo.jimenez@gmail.com","avatar":"https://avatars.githubusercontent.com/u/292484?v=4"},"body":"Hi Junio,\n\nOk, I'll put on my devil's prosecutor's hat then :)\n\nI am not sure you read the first message in this thread (I know, I\nmessed up, sorry), but there I describe the real case that happened to\nme, and the current behavior made it much harder to find out what was\nhappening.\n\nThat is the one:\nhttps://lore.kernel.org/git/CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@mail.gmail.com/T/#u\n\nIf the configuration file is there, but it behaves as it is not, one\nwill quickly point its fingers to the software that reads it.\n\nIn the end, if I had seen the message \"Permission denied.\" that was\nstored in \"$!\", I would have had a better clue that the problem was\nwith AppArmor, and not Gitweb.\n\nThere were a few unanswered questions in stack overflow regarding the\nissue of enabling features on Gitweb when using \"git instaweb\" in a\npersistent way, some of them might have been linked to that.\n\n- gitweb refusing to blame:\nhttps://serverfault.com/questions/551762/gitweb-refusing-to-blame\n- How do I enable \"blame\" when using git instaweb?:\nhttps://stackoverflow.com/questions/66688084/how-do-i-enable-blame-when-using-git-instaweb/77793405\n- My own question: Problem with `git instaweb` on OpenSUSE Tumbleweed:\n/etc/gitweb-common.conf is not being read:\nhttps://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n\n\nLet's see if someone comes up with a good reason to find the devil not guilty :)\n\nRegards,\nMarcelo.\n\nOn Wed, Jan 10, 2024 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> writes:\n>\n> > This patch fixes a possibility of a permission to access error go\n> > unnoticed.\n> >\n> > Perl uses two different variables to manage errors from a do. One\n> > is $@, which is set in this case when do is unable to compile the\n> > file. The other is $!, which is set in case do cannot read the file.\n> > By printing the value of $! I found out that it was set to Permission\n> > denied. Since the script does not currently test for $!, the error\n> > goes unnoticed.\n>\n> Well explained how the current code behaves and why.\n>\n> With my devil's advocate hat on, I suspect that the current\n> behaviour comes from the wish to see \"file exists but unreadable\"\n> and \"the named file does not exist\" behave the same way.  If you\n> pass the name of a configuration file that does not exist, however,\n> the codeblock to die does not trigger at all.  If a file does exist\n> but unreadable, to gitweb, it is just as good as having no file to\n> read configuration data from---either way, it cannot use anything\n> useful from the named file.  So returning silently, which is the\n> \"bug\" you are fixing, does not sound too bad.\n>\n> I dunno.  Let's queue and see what others would say.\n>\n> Thanks.\n>\n> > Perl do block documentation: https://perldoc.perl.org/functions/do\n> >\n> > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>\n> > ---\n> >  gitweb/gitweb.perl | 4 +++-\n> >  1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index e66eb3d9ba..5d0122020f 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -728,9 +728,11 @@ sub filter_and_validate_refs {\n> >  sub read_config_file {\n> >       my $filename = shift;\n> >       return unless defined $filename;\n> > -     # die if there are errors parsing config file\n> >       if (-e $filename) {\n> >               do $filename;\n> > +             # die if there is a problem accessing the file\n> > +             die $! if $!;\n> > +             # die if there are errors parsing config file\n> >               die $@ if $@;\n> >               return 1;\n> >       }\n"}]}