276913 - Uninitialized value message from CGI.pl line 176

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

I see a lot of this in landfill's error_log for Apache:

[Sun Jan 02 18:59:34 2005] [error] [client 202.22.158.65] [Sun Jan  2 18:59:34
2005] show_bug.cgi: Use of uninitialized value in string eq at CGI.pl line 176.,
referer:
http://landfill.bugzilla.org/bugzilla-tip/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&product=&content=

The line is:

return if ($field eq "dependson" || $field eq "blocked");

It just needs to check for the actual *existence* of $field.

Comment 1

[Max Kanat-Alexander]

This was introduced by bug 264601, according to cvs blame.

Comment 2

[Max Kanat-Alexander]

The actually correct way to fix this bug is to set a default for $field when it
isn't passed. I'm assuming that developers are assuming bug_id when they don't
pass a field name.

I could also put "unknown."

Comment 3

[Frédéric Buclin]

I have a patch for this too. I wrote it just before 31. December. That's the
reason why this patch and some others have not been uploaded yet.

I will submit in a few minutes. Taking bug!

Comment 4

[Max Kanat-Alexander]

Attachment: Simple patch

OK, here's a very simple patch to solve this problem. Anybody may do the review
if they want, but lpsolit wrote the original code, so I wanted him to look at
it.

By the way, this code is *really* confusing. I had to actually go look at the
user-error template to understand what $field was being used for.

Comment 5

[Max Kanat-Alexander]

Oh, OK. You can upload your patch, also. :-)

Comment 6

[Max Kanat-Alexander]

I tried to clear approval2.18? but it won't let me, even though I'm the one who
set it.

Comment 7

[Frédéric Buclin]

Attachment: Check if $field is defined

Comment 8

[Max Kanat-Alexander]

Comment on attachment 170180 [details] [diff] [review]
Check if $field is defined

>-    return if ($field eq "dependson" || $field eq "blocked");
>+    return if (defined $field && ($field eq "dependson" || $field eq "blocked"));

  If you could change that to defined($field) on checkin I'd be happy.
Otherwise this is fine.

Comment 9

[Max Kanat-Alexander]

OK, *now* I'm asking for approval.

Comment 10

[Frédéric Buclin]

(In reply to comment #8)
>   If you could change that to defined($field) on checkin I'd be happy.

Not sure this is really necessary. Too many () aren't good. ;)

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> Not sure this is really necessary. Too many () aren't good. ;)

  I fully disagree. Read the perlstyle manual. (And every other
programming-style manual in existence.)

Comment 12

[Shane H. W. Travis]

Not that Max nee(In reply to comment #11)
> (In reply to comment #10)
> > Too many () aren't good. ;)
>  I fully disagree.

Not that Max needs my help, but he's right. Explicitly stating your wishes (in 
this case, by wrapping your code in () ) is far better than hoping that the 
compiler -- and every future compiler -- does things the way you want it to be 
done.

Also remember, you won't be maintaining this code forever, or even necessarily 
next month. We owe it to the future guys who have to read/decipher it to make 
things as clear as possible.

Comment 13

[Frédéric Buclin]

Attachment: backport for the 2.18 branch

The line following my patch differs between 2.18 and 2.19.1 so I need to write
the same patch for the 2.18 branch.

For consistency with my already approved patch, I don't add () around $field.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 170479 [details] [diff] [review]
backport for the 2.18 branch

Yes, I cetify that this patch is the same as the one above. :-)

Comment 15

[Shane H. W. Travis]

2.18:
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.211.2.5; previous revision: 1.211.2.4
done

Tip:
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.219; previous revision: 1.218
done

Comment 16

[Frédéric Buclin]

*** Bug 281013 has been marked as a duplicate of this bug. ***