[PATCH] gitweb: Avoid "Use of uninitialized value" errors (written to logs)

[PATCH] gitweb: Avoid "Use of uninitialized value" errors (written to logs)

[Jakub Narebski]

Try to avoid "Use of uninitialized value ..." errors, due to bad
revision, incorrect filename, wrong object id, bad file etc. (wrong
value of 'h', 'hb', 'f', etc. parameters). This avoids polluting web
server errors log.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is a bit of "bandaid" patch, as if possible the callers should
be corrected, and should check if there is something to pass along.

 gitweb/gitweb.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 21864c6..afa0056 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -594,6 +594,9 @@ sub esc_html ($;%) {
 	my $str = shift;
 	my %opts = @_;
 
+	# empty or undefined
+	return $str unless $str;
+
 	$str = decode_utf8($str);
 	$str = $cgi->escapeHTML($str);
 	if ($opts{'-nbsp'}) {
@@ -1059,6 +1062,7 @@ sub git_get_hash_by_path {
 		or die_error(undef, "Open git-ls-tree failed");
 	my $line = <$fd>;
 	close $fd or return undef;
+	$line or return undef;
 
 	#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa	panic.c'
 	$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/;
@@ -1377,7 +1381,7 @@ sub parse_commit_text {
 	pop @commit_lines; # Remove '\0'
 
 	my $header = shift @commit_lines;
-	if (!($header =~ m/^[0-9a-fA-F]{40}/)) {
+	if (!defined $header || $header !~ m/^[0-9a-fA-F]{40}/) {
 		return;
 	}
 	($co{'id'}, my @parents) = split ' ', $header;
-- 
1.5.1.3

Re: [PATCH] gitweb: Avoid "Use of uninitialized value" errors (written to logs)

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Try to avoid "Use of uninitialized value ..." errors, due to bad
> revision, incorrect filename, wrong object id, bad file etc. (wrong
> value of 'h', 'hb', 'f', etc. parameters). This avoids polluting web
> server errors log.
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> This is a bit of "bandaid" patch, as if possible the callers should
> be corrected, and should check if there is something to pass along.

If the bad values come from the end user (via the browser), I
suspect that should be checked and rejected far earlier than
this sub on the output path is called.  Are there code that
internally needs to pass bogus values to this function?

@@ -594,6 +594,9 @@ sub esc_html ($;%) {
 	my $str = shift;
 	my %opts = @_;
 
+	# empty or undefined
+	return $str unless $str;
+

I think this is wrong, as the callers of esc_html typically
concatenate the return value with other strings.  If $str could
be undef, the caller would end up getting the warning when doing
the concatenation, so you did not solve anything.

$str could be '0' (literal string constant whose length is 1 and
has character *zero* in it), in which case you return it intact.
It is safe only because esc_html('0') is '0' itself; use of
"unless $str" here is a very bad style.

A more straightforward way obviously is:

	if (!defined $str) {
        	return '';
	}

@@ -1059,6 +1062,7 @@ sub git_get_hash_by_path {
 		or die_error(undef, "Open git-ls-tree failed");
 	my $line = <$fd>;
 	close $fd or return undef;
+	$line or return undef;
 
 	#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa	panic.c'
 	$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/;

I think this one means well but not quite to my taste; path may
be given by the end user and we need to run ls-tree to find out
if that path exists or not here.  $line would be undefined if
path does not exist, so...

	if (!defined $line) {
        	return undef;
	}

Note that it is very unlikely that $line consists of single '0'
here, so "$line or ..." would probably not break in practice.
My preference to use defined is more style and discipline thing.

@@ -1377,7 +1381,7 @@ sub parse_commit_text {
 	pop @commit_lines; # Remove '\0'
 
 	my $header = shift @commit_lines;
-	if (!($header =~ m/^[0-9a-fA-F]{40}/)) {
+	if (!defined $header || $header !~ m/^[0-9a-fA-F]{40}/) {
 		return;
 	}
 	($co{'id'}, my @parents) = split ' ', $header;

I would prefer checking the length of @commit_linse before
blindly shifting it out.

	if (!@commit_lines) {
        	return;
	}
	my $header = shift @commit_lines;
        ...

I am Ok with "return if (!@commit_lines);" if you feel it is
more Perl-ish.

One final note.

If you think using postfix "Statement Modifiers" somehow makes
your program look more Perl-ish, I think you should reconsider.
IMHO, coding more carefully to distinguish undef and other forms
of falsehood where the difference matters would make your code
look much more Perl-ish.

[PATCH (amend)] gitweb: Check if requested object exists

[Jakub Narebski]

Try to avoid "Use of uninitialized value ..." errors caused by bad
revision, incorrect filename, wrong object id, bad file etc. (wrong
value of 'h', 'hb', 'f', etc. parameters). This avoids polluting web
server errors log.

Correct git_get_hash_by_path and parse_commit_text (and, in turn,
parse_commit) to return undef if object does not exist.  Check in
git_tag if requested tag exists.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is replacement of earlier "bandaid" patch

  [PATCH] gitweb: Avoid "Use of uninitialized value" errors (written to logs)
  Message-Id: <200705120135.30150.jnareb@gmail.com>

This one tries to cure causes, not put bandaid over symptoms.
It also passes my gitweb test.

One thing that is left is to fix "Use of initialized value..." warnings
for empty repositories (initialized, but without any commits). But I
don't think that this corner case is terribly important.

 gitweb/gitweb.perl |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 21864c6..74556f7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1060,6 +1060,11 @@ sub git_get_hash_by_path {
 	my $line = <$fd>;
 	close $fd or return undef;
 
+	if (!defined $line) {
+		# there is no tree or hash given by $path at $base
+		return undef;
+	}
+
 	#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa	panic.c'
 	$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\t/;
 	if (defined $type && $type ne $2) {
@@ -1376,8 +1381,12 @@ sub parse_commit_text {
 
 	pop @commit_lines; # Remove '\0'
 
+	if (! @commit_lines) {
+		return;
+	}
+
 	my $header = shift @commit_lines;
-	if (!($header =~ m/^[0-9a-fA-F]{40}/)) {
+	if ($header !~ m/^[0-9a-fA-F]{40}/) {
 		return;
 	}
 	($co{'id'}, my @parents) = split ' ', $header;
@@ -3409,6 +3418,11 @@ sub git_tag {
 	git_header_html();
 	git_print_page_nav('','', $head,undef,$head);
 	my %tag = parse_tag($hash);
+
+	if (! %tag) {
+		die_error(undef, "Unknown tag object");
+	}
+
 	git_print_header_div('commit', esc_html($tag{'name'}), $hash);
 	print "<div class=\"title_text\">\n" .
 	      "<table cellspacing=\"0\">\n" .
-- 
1.5.1.4

[PATCH] gitweb: Fix "Use of unitialized value" warnings in empty repository

[Jakub Narebski]

Fix it so gitweb doesn't write "Use of unitialized value..." warnings
(which gets written in web server logs) for empty (no commits)
repository.

In empty repository "last change" (last activity) doesn't make sense;
also there is no sense in parsing commits which aren't there.

In projects list for empty repositories gitweb now writes "No commits"
using "noage" class, instead of leaving cell empty, in the last change
column.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Jakub Narebski wrote:
> One thing that is left is to fix "Use of initialized value..." warnings
> for empty repositories (initialized, but without any commits). But I
> don't think that this corner case is terribly important.

This patch fixes this issue.

 gitweb/gitweb.perl |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d467bf3..c2eeca9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -728,7 +728,9 @@ sub chop_str {
 sub age_class {
 	my $age = shift;
 
-	if ($age < 60*60*2) {
+	if (!defined $age) {
+		return "noage";
+	} elsif ($age < 60*60*2) {
 		return "age0";
 	} elsif ($age < 60*60*24*2) {
 		return "age1";
@@ -1258,7 +1260,8 @@ sub git_get_last_activity {
 	     'refs/heads') or return;
 	my $most_recent = <$fd>;
 	close $fd or return;
-	if ($most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
+	if (defined $most_recent &&
+	    $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
 		my $timestamp = $1;
 		my $age = time - $timestamp;
 		return ($age, age_string($age));
@@ -2983,7 +2986,7 @@ sub git_project_list_body {
 		                        esc_html($pr->{'descr'})) . "</td>\n" .
 		      "<td><i>" . chop_str($pr->{'owner'}, 15) . "</i></td>\n";
 		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
-		      $pr->{'age_string'} . "</td>\n" .
+		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
 		      "<td class=\"link\">" .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary")}, "summary")   . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"shortlog")}, "shortlog") . " | " .
@@ -3335,7 +3338,7 @@ sub git_project_index {
 sub git_summary {
 	my $descr = git_get_project_description($project) || "none";
 	my %co = parse_commit("HEAD");
-	my %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+	my %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();
 	my $head = $co{'id'};
 
 	my $owner = git_get_project_owner($project);
@@ -3358,8 +3361,11 @@ sub git_summary {
 	print "<div class=\"title\">&nbsp;</div>\n";
 	print "<table cellspacing=\"0\">\n" .
 	      "<tr><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
-	      "<tr><td>owner</td><td>$owner</td></tr>\n" .
-	      "<tr><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
+	      "<tr><td>owner</td><td>$owner</td></tr>\n";
+	if (defined $cd{'rfc2822'}) {
+		print "<tr><td>last change</td><td>$cd{'rfc2822'}</td></tr>\n";
+	}
+
 	# use per project git URL list in $projectroot/$project/cloneurl
 	# or make project git URL from git base URL and project name
 	my $url_tag = "URL";
@@ -3382,11 +3388,13 @@ sub git_summary {
 
 	# we need to request one more than 16 (0..15) to check if
 	# those 16 are all
-	my @commitlist = parse_commits($head, 17);
-	git_print_header_div('shortlog');
-	git_shortlog_body(\@commitlist, 0, 15, $refs,
-	                  $#commitlist <=  15 ? undef :
-	                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
+	my @commitlist = $head ? parse_commits($head, 17) : ();
+	if (@commitlist) {
+		git_print_header_div('shortlog');
+		git_shortlog_body(\@commitlist, 0, 15, $refs,
+		                  $#commitlist <=  15 ? undef :
+		                  $cgi->a({-href => href(action=>"shortlog")}, "..."));
+	}
 
 	if (@taglist) {
 		git_print_header_div('tags');
-- 
1.5.1.4

Re: [PATCH (amend)] gitweb: Check if requested object exists

[Junio C Hamano]

Thanks.

[PATCH] gitweb: Test if $from_id and $to_id are defined before comparison

[Jakub Narebski]

Get rid of "Use of uninitialized value in string eq at
gitweb/gitweb.perl line 2320" warning caused by the fact that "empty"
patches, consisting only of extended git diff header and with patch
body empty, such as patch for pure rename, does not have "index" line
in extended diff header.  For such patches $from_id and $to_id, filled
from parsing extended diff header, are undefined.  But such patches
cannot be continuation patches.

Test if $from_id and $to_id are defined before comparing them with
$diffinfo.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This also fixes "Use of uninitialized value ..." error/warning, but this
time it is caused by something else than non-existent object (wrong value
of parameter).

 gitweb/gitweb.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index afa0056..2b39502 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2664,9 +2664,10 @@ sub git_patchset_body {
 		# check if current patch belong to current raw line
 		# and parse raw git-diff line if needed
 		if (defined $diffinfo &&
+		    defined $from_id && defined $to_id &&
 		    from_ids_eq($diffinfo->{'from_id'}, $from_id) &&
 		    $diffinfo->{'to_id'} eq $to_id) {
-			# this is split patch
+			# this is continuation of a split patch
 			print "<div class=\"patch cont\">\n";
 		} else {
 			# advance raw git-diff output if needed
-- 
1.5.1.4

Re: [PATCH] gitweb: Test if $from_id and $to_id are defined before comparison

[Junio C Hamano]

Thanks.