304044 - missing scalar() for some parameters

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Some parameters passed to routines should be evaluated in a scalar context, but
unfortunately they are not. These regressions were introduced when doing
deFORMication. Mea culpa, I was the reviewer. :(

Till now, these regressions were hidden by prototypes which forced these
parameters to be evaluated in a scalar context. But with bug 303669 and the
removal of prototypes, some of these regressions are now clearly visible, see
bug 303669 comment 11.

I will try to submit a patch asap.

Here are the following cases which I won't consider:

1.1) if ($cgi->param('foo') eq $bar)
1.2) if ($cgi->param('foo') && $cgi->param('bar'))
2) Foo($cgi->param('bar')) # only 1 parameter

In these cases, I think it doesn't hurt what the context is.

The only cases which I will consider are:

3.1) Foo($cgi->param('bar'), $cgi->param('baz'))
3.2) Foo($cgi->param('bar'), $baz)
3.3) Foo($bar, $cgi->param('baz'))

Comment 1

[Frédéric Buclin]

also, when there is a clear check of the form:

if (defined $cgi->param('foo')) {
    Bar($cgi->param('foo'), $baz);
}

I don't write scalar() as we can reasonably expect $cgi->param('foo') to be
evaluated correctly.

Comment 2

[Frédéric Buclin]

Please don't tell me SQL statements are also affected. :(

$dbh->do("INSERT INTO foo (bar1, bar2) VALUES (?, ?)", undef,
         ($cgi->param('baz1'), $baz2))

Comment 3

[Frédéric Buclin]

Here the regexp I'm going to use for this patch:

grep -nPR '(\(|,)\s*\$cgi-\>param' *

This may help the reviewer to understand how I proceeded.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

This patch and the patch about removing prototypes have to be applied together
due to ValidateDependencies() in post_bug.cgi which now takes only 2
parameters, while the prototype says 3 (runtests.pl complains if you let the
prototype). I let it as is to prevent to bitrot mkanat's patch.

Most of the time, parameters are checked before being passed to routines. In
process_bug.cgi, most parameters are checked using check_form_field and
check_form_field_defined... except comments if commentonxxx is turned off and
privacy, explaining why AppendComment failed miserably.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 192151 [details] [diff] [review]
patch, v1

Yeah, this is good. And it's good to have, too, because anything that became a
method (as GetFormat and AppendComment will become) would have hit this anyway.

Comment 6

[Max Kanat-Alexander]

We aren't removing prototypes on the 2.20 branch, so this doesn't need to be
targeted at 2.20.

Comment 7

[Frédéric Buclin]

Checking in chart.cgi;
/cvsroot/mozilla/webtools/bugzilla/chart.cgi,v  <--  chart.cgi
new revision: 1.13; previous revision: 1.12
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.122; previous revision: 1.121
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.275; previous revision: 1.274
done
Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v  <--  report.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.85; previous revision: 1.84
done