298510 - Many uninitialized value errors in editwhines involving length function

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)

At 16 locations in editwhines.cgi, the length operation is used on a variable
that might not be defined.  This generates "Use of uninitialized value in length
at ..." errors when run.

The errors are generated for lines 233-240 and lines 332-339 of editwhines.cgi
(the tip version).

Presumably, this could be fixed by replacing length(...) with defined(...) at
the locations specificed above.

Reproducible: Sometimes

Steps to Reproduce:




These lines gave me unitialized value warnings only on the first time I set up a
whine, with the exception of line 339, which gave me a warning every time I set
up a new whine.

For reference, here are lines 233-240 of editwhines.cgi:

> $o_day         = '' unless length($o_day);
> $o_time        = '' unless length($o_time);
> $o_mailto      = '' unless length($o_mailto);
> $o_mailto_type = '' unless length($o_mailto_type);
> $day           = '' unless length($day);
> $time          = '' unless length($time);
> $mailto        = '' unless length($mailto);
> $mailto_type   = '' unless length($mailto_type);

Also, here are lines 332-339 of the same file:

> $o_sort          = '' unless length($o_sort);
> $o_queryname     = '' unless length($o_queryname);
> $o_title         = '' unless length($o_title);
> $o_onemailperbug = '' unless length($o_onemailperbug);
> $sort            = '' unless length($sort);
> $queryname       = '' unless length($queryname);      
> $title           = '' unless length($title);     
> $onemailperbug   = '' unless length($onemailperbug);

Comment 1

[A. Karl Kornel]

Attachment: Change calls from 'length' to 'defined' on certain lines

I'm operating under the assumption that this is a bug, and that it can be fixed
by replacing calls to length() with calls to defined() for the lines mentioned
in the initial description.  This attachment chances "length" to "defined" for
those lines.

Of course, if either assumption above is wrong, then this attachment is
probably review- and this bug may be invalid.

Comment 2

[A. Karl Kornel]

Comment on attachment 187057 [details] [diff] [review]
Change calls from 'length' to 'defined' on certain lines

It's been 7+ days since the request was raised.  Requesting review from
LpSolit...

Comment 3

[Frédéric Buclin]

Comment on attachment 187057 [details] [diff] [review]
Change calls from 'length' to 'defined' on certain lines

>                     my $o_mailto_type = lc $cgi->param("orig_mailto_type_$sid");
>                     my $mailto_type   = $cgi->param("mailto_type_$sid");
> 
>-                    $o_day         = '' unless length($o_day);
>-                    $o_time        = '' unless length($o_time);
>-                    $o_mailto      = '' unless length($o_mailto);
>-                    $o_mailto_type = '' unless length($o_mailto_type);
>-                    $day           = '' unless length($day);
>-                    $time          = '' unless length($time);
>-                    $mailto        = '' unless length($mailto);
>-                    $mailto_type   = '' unless length($mailto_type);
>+                    $o_day         = '' unless defined($o_day);
>+                    $o_time        = '' unless defined($o_time);
>+                    $o_mailto      = '' unless defined($o_mailto);
>+                    $o_mailto_type = '' unless defined($o_mailto_type);
>+                    $day           = '' unless defined($day);
>+                    $time          = '' unless defined($time);
>+                    $mailto        = '' unless defined($mailto);
>+                    $mailto_type   = '' unless defined($mailto_type);

There is a better way to do this. Write:
my $o_day = $cgi->param("orig_day_$sid") || ''; for strings and:
my $mailto_type = $cgi->param("mailto_type_$sid") || 0; for numeric fields
(mailto_type and o_mailto_type are the only ones).


>-                    $o_sort          = '' unless length($o_sort);
>-                    $o_queryname     = '' unless length($o_queryname);
>-                    $o_title         = '' unless length($o_title);
>-                    $o_onemailperbug = '' unless length($o_onemailperbug);
>-                    $sort            = '' unless length($sort);
>-                    $queryname       = '' unless length($queryname);
>-                    $title           = '' unless length($title);
>-                    $onemailperbug   = '' unless length($onemailperbug);
>+                    $o_sort          = '' unless defined($o_sort);
>+                    $o_queryname     = '' unless defined($o_queryname);
>+                    $o_title         = '' unless defined($o_title);
>+                    $o_onemailperbug = '' unless defined($o_onemailperbug);
>+                    $sort            = '' unless defined($sort);
>+                    $queryname       = '' unless defined($queryname);
>+                    $title           = '' unless defined($title);
>+                    $onemailperbug   = '' unless defined($onemailperbug);

Ditto here. Note that $sort and $onemailperbug (and their o_xxx counterparts)
are numerical. This also means that the test has to be updated, using "!="
instead of "ne" for these two fields.


>                     if ($onemailperbug eq 'on') {
>                         $onemailperbug = 1;

$onemailperbug is the value returned by a checkbox, i.e. either 0 or 1. This IF
block here is meaningless. Remove it as $onemailperbug is already correctly
set.

Comment 4

[A. Karl Kornel]

Attachment: Patch v2

Modifies attachment #187057 [details] [diff] [review] with respect to comment 3:
(In reply to comment #3)
> There is a better way to do this. Write:
> my $o_day = $cgi->param("orig_day_$sid") || ''; for strings and:
> my $mailto_type = $cgi->param("mailto_type_$sid") || 0; for numeric fields
> (mailto_type and o_mailto_type are the only ones).

I like that!  Updated.

> Ditto here. Note that $sort and $onemailperbug (and their o_xxx counterparts)

> are numerical. This also means that the test has to be updated, using "!="
> instead of "ne" for these two fields.

Updated.

> $onemailperbug is the value returned by a checkbox, i.e. either 0 or 1. This 

> IF block here is meaningless. Remove it as $onemailperbug is already
correctly
> set.

From basic experimentation it seems that 'on' may actually be returned. 
Modifying the test to simply check for defined/undefined.

Comment 5

[Frédéric Buclin]

Comment on attachment 188075 [details] [diff] [review]
Patch v2

>+                    my $o_mailto_type = $cgi->param("orig_mailto_type_$sid")
>+                                        || 0;

Nit: let || 0 on the previous line; this makes things look prettier (I first
thought you missed this one).


>+                    my $o_sort      = $cgi->param("orig_query_sort_$qid")
>+                                      || 0;

Nit: same remark here and in subsequent lines (also for || '').


>-                    if ( ($o_sort ne $sort) ||
>+                    if ( ($o_sort != $sort) ||
>                          ($o_queryname ne $queryname) ||
>                          ($o_onemailperbug xor $onemailperbug) ||
>                          ($o_title ne $title) ){

Nit: as $onemailperbug and $o_onemailperbug are either 0 or 1, we could use
"!=" instead of "xor".


Nit: as your patch is about the "length" function, could you make me a favour
and remove them completely? For example, I can read:

trick_taint($onemailperbug) if length $onemailperbug;

First of all, $onemailperbug is numeric, so it should be
detaint_natural($onemailperbug) instead of trick_taint(). Second, I'm not sure
the length function makes sense when applied to integers instead of strings. My
opinion is that you can remove all these "if length()" completely and call
detaint_natural() and trick_taint() in all cases (it's not a performance
issue).

Comment 6

[A. Karl Kornel]

Attachment: Patch v3

modification of attachment 188075 [details] [diff] [review] with respect to comment 5
> Nit: same remark here and in subsequent lines (also for || '').

Pushes it over the 80-characters per line limit, but opting for readability.

> Nit: as $onemailperbug and $o_onemailperbug are either 0 or 1, we could use
> "!=" instead of "xor".

No problems with that!

> Nit: as your patch is about the "length" function, could you make me a favour

> and remove them completely? For example, I can read:
> 
> trick_taint($onemailperbug) if length $onemailperbug;
> 
> First of all, $onemailperbug is numeric, so it should be
> detaint_natural($onemailperbug) instead of trick_taint(). Second, I'm not
sure
> the length function makes sense when applied to integers instead of strings.
My
> opinion is that you can remove all these "if length()" completely and call
> detaint_natural() and trick_taint() in all cases (it's not a performance
> issue).

I think I've got it all.

Comment 7

[A. Karl Kornel]

Attachment: Patch v4

Modification of attachment 188116 [details] [diff] [review].  Detaints moved back into original
positions.  Also removes a conflict with the patch to bug 298508.

Comment 8

[Frédéric Buclin]

Comment on attachment 188129 [details] [diff] [review]
Patch v4

r=LpSolit

Comment 9

[Frédéric Buclin]

Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.8; previous revision: 1.7
done