gitweb: Avoid "Use of uninitialized value" errors (written to logs)
PATCH 1/1 gitweb: Avoid "Use of uninitialized value" errors (written to logs)
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.
---
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(-)
gitweb/gitweb.perl +5 -1
@@ -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;
Signature · 1 line
1.5.1.3
PATCH 1/1 gitweb: Test if $from_id and $to_id are defined before comparison
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.
---
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(-)
gitweb/gitweb.perl +2 -1
@@ -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
Signature · 1 line
1.5.1.4
Re: [PATCH] gitweb: Test if $from_id and $to_id are defined before comparison
Thanks.
Quoted 9 lines · 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 <[email protected]>---
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 1/1 gitweb: Check if requested object exists
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.
---
This patch is replacement of earlier "bandaid" patch
[PATCH] gitweb: Avoid "Use of uninitialized value" errors (written to logs)
Message-Id: <[email protected]>
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(-)
gitweb/gitweb.perl +15 -1
@@ -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" .
Signature · 1 line
1.5.1.4
PATCH 1/1 gitweb: Fix "Use of unitialized value" warnings in empty repository
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.
---
Quoted 3 lines · Jakub Narebski
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(-)
gitweb/gitweb.perl +19 -11
@@ -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\"> </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');
Signature · 1 line
1.5.1.4
Re: [PATCH (amend)] gitweb: Check if requested object exists
In reply to Jakub NarebskiThanks.