missing scalar() for some parameters

RESOLVED FIXED in Bugzilla 2.22

Status

()

--
major
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({regression})

2.20
Bugzilla 2.22
regression
Bug Flags:
approval +

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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'))
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 1

13 years ago
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.
(Assignee)

Comment 2

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

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

Comment 3

13 years ago
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.
(Assignee)

Comment 4

13 years ago
Created attachment 192151 [details] [diff] [review]
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.
Attachment #192151 - Flags: review?(mkanat)

Comment 5

13 years ago
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.
Attachment #192151 - Flags: review?(mkanat) → review+

Comment 6

13 years ago
We aren't removing prototypes on the 2.20 branch, so this doesn't need to be
targeted at 2.20.
Flags: approval?
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Flags: approval? → approval+
(Assignee)

Comment 7

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.