[PATCH] gitweb: suppress 'Use of uninitialized value' error

[PATCH] gitweb: suppress 'Use of uninitialized value' error

[Giuseppe Bilotta]

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

The patch could have been a one-liner by adding the defined check at
line 943, but that pushed the line to 120 char, so I decided for this
slightly more complex form.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7fbd5ff..c7f257e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -940,10 +940,13 @@ sub href {
 			if (defined $params{'hash_parent_base'}) {
 				$href .= esc_url($params{'hash_parent_base'});
 				# skip the file_parent if it's the same as the file_name
-				delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
-				if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
-					$href .= ":/".esc_url($params{'file_parent'});
-					delete $params{'file_parent'};
+				if (defined $params{'file_parent'}) {
+					if ($params{'file_parent'} eq $params{'file_name'}) {
+						delete $params{'file_parent'};
+					} else if ($params{'file_parent'} !~ /\.\./) {
+						$href .= ":/".esc_url($params{'file_parent'});
+						delete $params{'file_parent'};
+					}
 				}
 				$href .= "..";
 				delete $params{'hash_parent'};
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error

[Jakub Narebski]

On Thu, 30 July 2009, Giuseppe Bilotta wrote:

Thanks for catching this.

However... First, the subject should be more specific and less generic.  
At least provide where 'Use of uninitialized' value was generated, e.g.:

  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' error in href()

or a bit shorter

  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' in href()

Second, it would be nice to have more detailed description in the body
of a commit message, in this case stating when mentioned error occur:

  This warning was generated when 'hash_parent_base' parameter was
  defined, and either 'file_name' or 'file_parent' was not defined.

Note that from above description you can notice that you protect against
'file_parent' being not defined, but not against 'file_name', so you
should I think write why it is not a problem:

  In gitweb code 'file_parent' is used only if 'file_name' is filled.

Well... almost.  This does not cover href(..., -replay=>1) with hand
crafted broken/invalid gitweb URL.  BTW. a question for you: how did
you detect/found this breakage?  I don't think gitweb generates such
broken links (with 'hash_parent_base' but not 'file_parent') normally,
but I might be mistaken.

Third, we would probably want to have additional case in t/t9500 test
to protect against regression here.  But that is not as important,
I think.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Otherwise, for what it is worth:

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> The patch could have been a one-liner by adding the defined check at
> line 943, but that pushed the line to 120 char, so I decided for this
> slightly more complex form.

That is IMHO a good solution.  Better not abuse 'if' modifier form.

> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7fbd5ff..c7f257e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -940,10 +940,13 @@ sub href {
>  			if (defined $params{'hash_parent_base'}) {
>  				$href .= esc_url($params{'hash_parent_base'});
>  				# skip the file_parent if it's the same as the file_name
> -				delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
> -				if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
> -					$href .= ":/".esc_url($params{'file_parent'});
> -					delete $params{'file_parent'};
> +				if (defined $params{'file_parent'}) {
> +					if ($params{'file_parent'} eq $params{'file_name'}) {
> +						delete $params{'file_parent'};
> +					} else if ($params{'file_parent'} !~ /\.\./) {
> +						$href .= ":/".esc_url($params{'file_parent'});
> +						delete $params{'file_parent'};
> +					}
>  				}
>  				$href .= "..";
>  				delete $params{'hash_parent'};
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error

[Giuseppe Bilotta]

2009/7/31 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 30 July 2009, Giuseppe Bilotta wrote:
>
> Thanks for catching this.
>
> However... First, the subject should be more specific and less generic.
> At least provide where 'Use of uninitialized' value was generated, e.g.:
>
>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' error in href()
>
> or a bit shorter
>
>  Subject: [PATCH] gitweb: Fix 'Use of uninitialized value' in href()

Agreed.

> Second, it would be nice to have more detailed description in the body
> of a commit message, in this case stating when mentioned error occur:
>
>  This warning was generated when 'hash_parent_base' parameter was
>  defined, and either 'file_name' or 'file_parent' was not defined.
>
> Note that from above description you can notice that you protect against
> 'file_parent' being not defined, but not against 'file_name', so you
> should I think write why it is not a problem:
>
>  In gitweb code 'file_parent' is used only if 'file_name' is filled.
>
> Well... almost.  This does not cover href(..., -replay=>1) with hand
> crafted broken/invalid gitweb URL.  BTW. a question for you: how did
> you detect/found this breakage?

I was looking at the error log of my webserver and spotted the lines,
coming from a spider indexing my git pages. An URL that caused it is
http://git.oblomov.eu/git/commit/32ae83194b0f287a9b6644cdad175c56417c31f3
(the tree link, I suspect).

> I don't think gitweb generates such
> broken links (with 'hash_parent_base' but not 'file_parent') normally,
> but I might be mistaken.

Do you think it would be worth to protect against this case?

> Third, we would probably want to have additional case in t/t9500 test
> to protect against regression here.  But that is not as important,
> I think.

I'll see if I can cook that up.

>>  gitweb/gitweb.perl |   11 +++++++----
>>  1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> The patch could have been a one-liner by adding the defined check at
>> line 943, but that pushed the line to 120 char, so I decided for this
>> slightly more complex form.
>
> That is IMHO a good solution.  Better not abuse 'if' modifier form.
>
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 7fbd5ff..c7f257e 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -940,10 +940,13 @@ sub href {
>>                       if (defined $params{'hash_parent_base'}) {
>>                               $href .= esc_url($params{'hash_parent_base'});
>>                               # skip the file_parent if it's the same as the file_name
>> -                             delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
>> -                             if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
>> -                                     $href .= ":/".esc_url($params{'file_parent'});
>> -                                     delete $params{'file_parent'};
>> +                             if (defined $params{'file_parent'}) {
>> +                                     if ($params{'file_parent'} eq $params{'file_name'}) {
>> +                                             delete $params{'file_parent'};
>> +                                     } else if ($params{'file_parent'} !~ /\.\./) {

And I'm an idiot 'cause this should be 'elsif'.

Resend coming soon.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: suppress 'Use of uninitialized value' error

[Jakub Narebski]

Giuseppe Bilotta wrote:
> 2009/7/31 Jakub Narebski <jnareb@gmail.com>:

> > I don't think gitweb generates such
> > broken links (with 'hash_parent_base' but not 'file_parent') normally,
> > but I might be mistaken.
> 
> Do you think it would be worth to protect against this case?

Yes, of course it is worth protecting against this case (better checking
if both sides of 'eq' comparison are defined).  We want to return error
message from gitweb if URL is borked, not have Perl error in logs.

-- 
Jakub Narebski
Poland

[PATCH] gitweb: fix 'Use of uninitialized value' error in href()

[Giuseppe Bilotta]

Equality between file_parent and file_name was being checked without a
preliminary check for existence of the parameters.

Fix by wrapping the equality check in appropriate if (defined ...),
rearranging the lines to prevent excessive length.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 gitweb/gitweb.perl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

The funny thing is that I seem to get the error for something as simple
as a commit view (_any_ commit view, for the matter), but I wasn't able
to reproduce it from the shell, which is why I'm not adding a testcase.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7fbd5ff..37120a3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -940,10 +940,13 @@ sub href {
 			if (defined $params{'hash_parent_base'}) {
 				$href .= esc_url($params{'hash_parent_base'});
 				# skip the file_parent if it's the same as the file_name
-				delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
-				if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
-					$href .= ":/".esc_url($params{'file_parent'});
-					delete $params{'file_parent'};
+				if (defined $params{'file_parent'}) {
+					if (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {
+						delete $params{'file_parent'};
+					} elsif ($params{'file_parent'} !~ /\.\./) {
+						$href .= ":/".esc_url($params{'file_parent'});
+						delete $params{'file_parent'};
+					}
 				}
 				$href .= "..";
 				delete $params{'hash_parent'};
-- 
1.6.3.rc1.192.gdbfcb

Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()

[Jakub Narebski]

On Fri, 31 July 2009, Giuseppe Bilotta wrote:

> Equality between file_parent and file_name was being checked without a
> preliminary check for existence of the parameters.
> 
> Fix by wrapping the equality check in appropriate if (defined ...),
> rearranging the lines to prevent excessive length.
> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

For what it is worth:

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
>  gitweb/gitweb.perl |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> The funny thing is that I seem to get the error for something as simple
> as a commit view (_any_ commit view, for the matter), but I wasn't able
> to reproduce it from the shell, which is why I'm not adding a testcase.

Gaaah! A Heisenbug! ;-)

Well, it worth fixing if it is real case scenario.

> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7fbd5ff..37120a3 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -940,10 +940,13 @@ sub href {
>  			if (defined $params{'hash_parent_base'}) {
>  				$href .= esc_url($params{'hash_parent_base'});
>  				# skip the file_parent if it's the same as the file_name
> -				delete $params{'file_parent'} if $params{'file_parent'} eq $params{'file_name'};
> -				if (defined $params{'file_parent'} && $params{'file_parent'} !~ /\.\./) {
> -					$href .= ":/".esc_url($params{'file_parent'});
> -					delete $params{'file_parent'};
> +				if (defined $params{'file_parent'}) {
> +					if (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {

or, to reduce line length even more:

  +					if (defined $params{'file_name'} &&
  +					    $params{'file_parent'} eq $params{'file_name'}) {

> +						delete $params{'file_parent'};
> +					} elsif ($params{'file_parent'} !~ /\.\./) {
> +						$href .= ":/".esc_url($params{'file_parent'});
> +						delete $params{'file_parent'};
> +					}
>  				}
>  				$href .= "..";
>  				delete $params{'hash_parent'};
> -- 
> 1.6.3.rc1.192.gdbfcb
> 
> 

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()

[Giuseppe Bilotta]

2009/7/31 Jakub Narebski <jnareb@gmail.com>:
> On Fri, 31 July 2009, Giuseppe Bilotta wrote:
>>
>> The funny thing is that I seem to get the error for something as simple
>> as a commit view (_any_ commit view, for the matter), but I wasn't able
>> to reproduce it from the shell, which is why I'm not adding a testcase.
>
> Gaaah! A Heisenbug! ;-)

Well, I can reproduce it consistently on the gitweb repository, with
any shortlog or commit view. I also think I understand why it happens
(i.e. in some cases the check for file_parent eq file_name is done
before calling href). The question, of course, is why wouldn't it
happen in the test repository.

>> +                             if (defined $params{'file_parent'}) {
>> +                                     if (defined $params{'file_name'} && $params{'file_parent'} eq $params{'file_name'}) {
>
> or, to reduce line length even more:
>
>  +                                     if (defined $params{'file_name'} &&
>  +                                         $params{'file_parent'} eq $params{'file_name'}) {

Oh right ... should I resend?

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> 2009/7/31 Jakub Narebski <jnareb@gmail.com>:
>> On Fri, 31 July 2009, Giuseppe Bilotta wrote:
>>>
>>> The funny thing is that I seem to get the error for something as simple
>>> as a commit view (_any_ commit view, for the matter), but I wasn't able
>>> to reproduce it from the shell, which is why I'm not adding a testcase.
>>
>> Gaaah! A Heisenbug! ;-)
>
> Well, I can reproduce it consistently on the gitweb repository, with
> any shortlog or commit view. I also think I understand why it happens
> (i.e. in some cases the check for file_parent eq file_name is done
> before calling href). The question, of course, is why wouldn't it
> happen in the test repository.

That sounds like something worth figuring out, independent of the issue
this patch addresses.

Re: [PATCH] gitweb: fix 'Use of uninitialized value' error in href()

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Acked-by: Jakub Narebski <jnareb@gmail.com>

Thanks, both.