{"thread":{"id":"8092","subject":"[PATCH] gitweb: Avoid \"Use of uninitialized value\" errors (written to logs)","startedAt":"2007-05-11T23:35:29Z","lastAt":"2007-05-13T19:12:48Z","messageCount":7,"participants":["Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"41875","messageId":"200705120135.30150.jnareb@gmail.com","threadId":"8092","inReplyTo":null,"subject":"[PATCH] gitweb: Avoid \"Use of uninitialized value\" errors (written to logs)","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-11T23:35:29Z","receivedAt":"2007-05-11T23:35:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Try to avoid \"Use of uninitialized value ...\" errors, due to bad\nrevision, incorrect filename, wrong object id, bad file etc. (wrong\nvalue of 'h', 'hb', 'f', etc. parameters). This avoids polluting web\nserver errors log.\n\nSigned-off-by: Jakub Narebski \n---\nThis is a bit of \"bandaid\" patch, as if possible the callers should\nbe corrected, and should check if there is something to pass along.\n\n gitweb/gitweb.perl | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 21864c6..afa0056 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -594,6 +594,9 @@ sub esc_html ($;%) {\n \tmy $str = shift;\n \tmy %opts = @_;\n \n+\t# empty or undefined\n+\treturn $str unless $str;\n+\n \t$str = decode_utf8($str);\n \t$str = $cgi->escapeHTML($str);\n \tif ($opts{'-nbsp'}) {\n@@ -1059,6 +1062,7 @@ sub git_get_hash_by_path {\n \t\tor die_error(undef, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n+\t$line or return undef;\n \n \t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/;\n@@ -1377,7 +1381,7 @@ sub parse_commit_text {\n \tpop @commit_lines; # Remove '\\0'\n \n \tmy $header = shift @commit_lines;\n-\tif (!($header =~ m/^[0-9a-fA-F]{40}/)) {\n+\tif (!defined $header || $header !~ m/^[0-9a-fA-F]{40}/) {\n \t\treturn;\n \t}\n \t($co{'id'}, my @parents) = split ' ', $header;\n-- \n1.5.1.3\n"},{"id":"41881","messageId":"7vzm4a7sv3.fsf@assigned-by-dhcp.cox.net","threadId":"8092","inReplyTo":"200705120135.30150.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Avoid \"Use of uninitialized value\" errors (written to logs)","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-12T01:06:40Z","receivedAt":"2007-05-12T01:06:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Try to avoid \"Use of uninitialized value ...\" errors, due to bad\n> revision, incorrect filename, wrong object id, bad file etc. (wrong\n> value of 'h', 'hb', 'f', etc. parameters). This avoids polluting web\n> server errors log.\n>\n> Signed-off-by: Jakub Narebski \n> ---\n> This is a bit of \"bandaid\" patch, as if possible the callers should\n> be corrected, and should check if there is something to pass along.\n\nIf the bad values come from the end user (via the browser), I\nsuspect that should be checked and rejected far earlier than\nthis sub on the output path is called. Are there code that\ninternally needs to pass bogus values to this function?\n\n@@ -594,6 +594,9 @@ sub esc_html ($;%) {\n \tmy $str = shift;\n \tmy %opts = @_;\n \n+\t# empty or undefined\n+\treturn $str unless $str;\n+\n\nI think this is wrong, as the callers of esc_html typically\nconcatenate the return value with other strings. If $str could\nbe undef, the caller would end up getting the warning when doing\nthe concatenation, so you did not solve anything.\n\n$str could be '0' (literal string constant whose length is 1 and\nhas character *zero* in it), in which case you return it intact.\nIt is safe only because esc_html('0') is '0' itself; use of\n\"unless $str\" here is a very bad style.\n\nA more straightforward way obviously is:\n\n\tif (!defined $str) {\n \treturn '';\n\t}\n\n@@ -1059,6 +1062,7 @@ sub git_get_hash_by_path {\n \t\tor die_error(undef, \"Open git-ls-tree failed\");\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n+\t$line or return undef;\n \n \t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/;\n\nI think this one means well but not quite to my taste; path may\nbe given by the end user and we need to run ls-tree to find out\nif that path exists or not here. $line would be undefined if\npath does not exist, so...\n\n\tif (!defined $line) {\n \treturn undef;\n\t}\n\nNote that it is very unlikely that $line consists of single '0'\nhere, so \"$line or ...\" would probably not break in practice.\nMy preference to use defined is more style and discipline thing.\n\n@@ -1377,7 +1381,7 @@ sub parse_commit_text {\n \tpop @commit_lines; # Remove '\\0'\n \n \tmy $header = shift @commit_lines;\n-\tif (!($header =~ m/^[0-9a-fA-F]{40}/)) {\n+\tif (!defined $header || $header !~ m/^[0-9a-fA-F]{40}/) {\n \t\treturn;\n \t}\n \t($co{'id'}, my @parents) = split ' ', $header;\n\nI would prefer checking the length of @commit_linse before\nblindly shifting it out.\n\n\tif (!@commit_lines) {\n \treturn;\n\t}\n\tmy $header = shift @commit_lines;\n ...\n\nI am Ok with \"return if (!@commit_lines);\" if you feel it is\nmore Perl-ish.\n\nOne final note.\n\nIf you think using postfix \"Statement Modifiers\" somehow makes\nyour program look more Perl-ish, I think you should reconsider.\nIMHO, coding more carefully to distinguish undef and other forms\nof falsehood where the difference matters would make your code\nlook much more Perl-ish.\n"},{"id":"41908","messageId":"200705121242.33156.jnareb@gmail.com","threadId":"8092","inReplyTo":"200705120135.30150.jnareb@gmail.com","subject":"[PATCH] gitweb: Test if $from_id and $to_id are defined before comparison","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-12T10:42:32Z","receivedAt":"2007-05-12T10:42:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Get rid of \"Use of uninitialized value in string eq at\ngitweb/gitweb.perl line 2320\" warning caused by the fact that \"empty\"\npatches, consisting only of extended git diff header and with patch\nbody empty, such as patch for pure rename, does not have \"index\" line\nin extended diff header. For such patches $from_id and $to_id, filled\nfrom parsing extended diff header, are undefined. But such patches\ncannot be continuation patches.\n\nTest if $from_id and $to_id are defined before comparing them with\n$diffinfo.\n\nSigned-off-by: Jakub Narebski \n---\nThis also fixes \"Use of uninitialized value ...\" error/warning, but this\ntime it is caused by something else than non-existent object (wrong value\nof parameter).\n\n gitweb/gitweb.perl | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex afa0056..2b39502 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -2664,9 +2664,10 @@ sub git_patchset_body {\n \t\t# check if current patch belong to current raw line\n \t\t# and parse raw git-diff line if needed\n \t\tif (defined $diffinfo &&\n+\t\t defined $from_id && defined $to_id &&\n \t\t from_ids_eq($diffinfo->{'from_id'}, $from_id) &&\n \t\t $diffinfo->{'to_id'} eq $to_id) {\n-\t\t\t# this is split patch\n+\t\t\t# this is continuation of a split patch\n \t\t\tprint \"
\\n\";\n \t\t} else {\n \t\t\t# advance raw git-diff output if needed\n-- \n1.5.1.4\n"},{"id":"41986","messageId":"200705122116.34486.jnareb@gmail.com","threadId":"8092","inReplyTo":"7vzm4a7sv3.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH (amend)] gitweb: Check if requested object exists","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-12T19:16:34Z","receivedAt":"2007-05-12T19:16:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Try to avoid \"Use of uninitialized value ...\" errors caused by bad\nrevision, incorrect filename, wrong object id, bad file etc. (wrong\nvalue of 'h', 'hb', 'f', etc. parameters). This avoids polluting web\nserver errors log.\n\nCorrect git_get_hash_by_path and parse_commit_text (and, in turn,\nparse_commit) to return undef if object does not exist. Check in\ngit_tag if requested tag exists.\n\nSigned-off-by: Jakub Narebski \n---\nThis patch is replacement of earlier \"bandaid\" patch\n\n [PATCH] gitweb: Avoid \"Use of uninitialized value\" errors (written to logs)\n Message-Id: <200705120135.30150.jnareb@gmail.com>\n\nThis one tries to cure causes, not put bandaid over symptoms.\nIt also passes my gitweb test.\n\nOne thing that is left is to fix \"Use of initialized value...\" warnings\nfor empty repositories (initialized, but without any commits). But I\ndon't think that this corner case is terribly important.\n\n gitweb/gitweb.perl | 16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 21864c6..74556f7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1060,6 +1060,11 @@ sub git_get_hash_by_path {\n \tmy $line = <$fd>;\n \tclose $fd or return undef;\n \n+\tif (!defined $line) {\n+\t\t# there is no tree or hash given by $path at $base\n+\t\treturn undef;\n+\t}\n+\n \t#'100644 blob 0fa3f3a66fb6a137f6ec2c19351ed4d807070ffa\tpanic.c'\n \t$line =~ m/^([0-9]+) (.+) ([0-9a-fA-F]{40})\\t/;\n \tif (defined $type && $type ne $2) {\n@@ -1376,8 +1381,12 @@ sub parse_commit_text {\n \n \tpop @commit_lines; # Remove '\\0'\n \n+\tif (! @commit_lines) {\n+\t\treturn;\n+\t}\n+\n \tmy $header = shift @commit_lines;\n-\tif (!($header =~ m/^[0-9a-fA-F]{40}/)) {\n+\tif ($header !~ m/^[0-9a-fA-F]{40}/) {\n \t\treturn;\n \t}\n \t($co{'id'}, my @parents) = split ' ', $header;\n@@ -3409,6 +3418,11 @@ sub git_tag {\n \tgit_header_html();\n \tgit_print_page_nav('','', $head,undef,$head);\n \tmy %tag = parse_tag($hash);\n+\n+\tif (! %tag) {\n+\t\tdie_error(undef, \"Unknown tag object\");\n+\t}\n+\n \tgit_print_header_div('commit', esc_html($tag{'name'}), $hash);\n \tprint \"
\\n\" .\n \t \"\\n\" .\n-- \n1.5.1.4\n"},{"id":"41951","messageId":"7vabw9zvt8.fsf@assigned-by-dhcp.cox.net","threadId":"8092","inReplyTo":"200705121242.33156.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Test if $from_id and $to_id are defined before comparison","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-12T19:27:47Z","receivedAt":"2007-05-12T19:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"42066","messageId":"200705131239.23353.jnareb@gmail.com","threadId":"8092","inReplyTo":"200705122116.34486.jnareb@gmail.com","subject":"[PATCH] gitweb: Fix \"Use of unitialized value\" warnings in empty repository","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-05-13T10:39:22Z","receivedAt":"2007-05-13T10:39:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Fix it so gitweb doesn't write \"Use of unitialized value...\" warnings\n(which gets written in web server logs) for empty (no commits)\nrepository.\n\nIn empty repository \"last change\" (last activity) doesn't make sense;\nalso there is no sense in parsing commits which aren't there.\n\nIn projects list for empty repositories gitweb now writes \"No commits\"\nusing \"noage\" class, instead of leaving cell empty, in the last change\ncolumn.\n\nSigned-off-by: Jakub Narebski \n---\nJakub Narebski wrote:\n> One thing that is left is to fix \"Use of initialized value...\" warnings\n> for empty repositories (initialized, but without any commits). But I\n> don't think that this corner case is terribly important.\n\nThis patch fixes this issue.\n\n gitweb/gitweb.perl | 30 +++++++++++++++++++-----------\n 1 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex d467bf3..c2eeca9 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -728,7 +728,9 @@ sub chop_str {\n sub age_class {\n \tmy $age = shift;\n \n-\tif ($age < 60*60*2) {\n+\tif (!defined $age) {\n+\t\treturn \"noage\";\n+\t} elsif ($age < 60*60*2) {\n \t\treturn \"age0\";\n \t} elsif ($age < 60*60*24*2) {\n \t\treturn \"age1\";\n@@ -1258,7 +1260,8 @@ sub git_get_last_activity {\n \t 'refs/heads') or return;\n \tmy $most_recent = <$fd>;\n \tclose $fd or return;\n-\tif ($most_recent =~ / (\\d+) [-+][01]\\d\\d\\d$/) {\n+\tif (defined $most_recent &&\n+\t $most_recent =~ / (\\d+) [-+][01]\\d\\d\\d$/) {\n \t\tmy $timestamp = $1;\n \t\tmy $age = time - $timestamp;\n \t\treturn ($age, age_string($age));\n@@ -2983,7 +2986,7 @@ sub git_project_list_body {\n \t\t esc_html($pr->{'descr'})) . \"\\n\" .\n \t\t \"\\n\";\n \t\tprint \"\\n\" .\n+\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n \t\t \"
\" . chop_str($pr->{'owner'}, 15) . \"{'age'}) . \"\\\">\" .\n-\t\t $pr->{'age_string'} . \"\" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\")}, \"summary\") . \" | \" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"shortlog\")}, \"shortlog\") . \" | \" .\n@@ -3335,7 +3338,7 @@ sub git_project_index {\n sub git_summary {\n \tmy $descr = git_get_project_description($project) || \"none\";\n \tmy %co = parse_commit(\"HEAD\");\n-\tmy %cd = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n+\tmy %cd = %co ? parse_date($co{'committer_epoch'}, $co{'committer_tz'}) : ();\n \tmy $head = $co{'id'};\n \n \tmy $owner = git_get_project_owner($project);\n@@ -3358,8 +3361,11 @@ sub git_summary {\n \tprint \"
 
\\n\";\n \tprint \"\\n\" .\n \t \"\\n\" .\n-\t \"\\n\" .\n-\t \"\\n\";\n+\t \"\\n\";\n+\tif (defined $cd{'rfc2822'}) {\n+\t\tprint \"\\n\";\n+\t}\n+\n \t# use per project git URL list in $projectroot/$project/cloneurl\n \t# or make project git URL from git base URL and project name\n \tmy $url_tag = \"URL\";\n@@ -3382,11 +3388,13 @@ sub git_summary {\n \n \t# we need to request one more than 16 (0..15) to check if\n \t# those 16 are all\n-\tmy @commitlist = parse_commits($head, 17);\n-\tgit_print_header_div('shortlog');\n-\tgit_shortlog_body(\\@commitlist, 0, 15, $refs,\n-\t $#commitlist <= 15 ? undef :\n-\t $cgi->a({-href => href(action=>\"shortlog\")}, \"...\"));\n+\tmy @commitlist = $head ? parse_commits($head, 17) : ();\n+\tif (@commitlist) {\n+\t\tgit_print_header_div('shortlog');\n+\t\tgit_shortlog_body(\\@commitlist, 0, 15, $refs,\n+\t\t $#commitlist <= 15 ? undef :\n+\t\t $cgi->a({-href => href(action=>\"shortlog\")}, \"...\"));\n+\t}\n \n \tif (@taglist) {\n \t\tgit_print_header_div('tags');\n-- \n1.5.1.4\n"},{"id":"42046","messageId":"7v4pmgsfkf.fsf@assigned-by-dhcp.cox.net","threadId":"8092","inReplyTo":"200705122116.34486.jnareb@gmail.com","subject":"Re: [PATCH (amend)] gitweb: Check if requested object exists","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-13T19:12:48Z","receivedAt":"2007-05-13T19:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}
description\" . esc_html($descr) . \"
owner$owner
last change$cd{'rfc2822'}
owner$owner
last change$cd{'rfc2822'}