PHP code throws warnings unnecessarily in several places
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
amavis-stats (Ubuntu) |
New
|
Undecided
|
Unassigned |
Bug Description
Summary of changes:
The code to which this comment refers causes numerous PHP warnings to be issued each time the script is executed on systems on which PHP open_basedir restrictions are in effect. If the location of the rrdtool executable were defined as a configuration directive, a) the warnings would disappear and b) the application would perform more efficiently (the filesystem is not queried for directories that will not exist).
------------------
105c105,109
<
---
>
> # XXX TODO
> # This path should be defined as a configuration directive, so that
> # open_basedir warnings are not thrown when the paths don't exist.
>
------------------
Wrapping this block in conditional logic prevents code from being executed when its expected inputs are not set (i.e., when an rrdtool graph is not generated for some reason).
------------------
130,143c134,149
<
< if (preg_match(
< $retval["xsize"] = $matches[1];
< $retval["ysize"] = $matches[2];
< } else {
< $rrd_error_val = $output[0];
< return false;
< }
<
< array_shift(
<
< $retval["calcpr"] = array();
< foreach ($output as $index => $value) {
< $retval[
---
>
> if (is_array($output) && !empty($output)) {
> if (preg_match(
> $retval["xsize"] = $matches[1];
> $retval["ysize"] = $matches[2];
> } else {
> $rrd_error_val = $output[0];
> return false;
> }
>
> array_shift(
>
> $retval["calcpr"] = array();
> foreach ($output as $index => $value) {
> $retval[
> }
------------------
Suppressing warnings and errors in this way is discouraged, as it makes debugging the application extremely difficult. There are other instances of this approach elsewhere within the code, but they are less problematic because application-
------------------
292c299
< $readfile = @file($seenfile);
---
> $readfile = file($seenfile);
------------------
This change addresses the mysterious output just below the "Generated by amavis-stats..." line:
amavis-
This output is caused in /usr/share/
<td align="
If we trace this variable back to its origin, we find that it is set in /usr/share/
'OUT_MSG' => $out_msg ? $out_msg : NULL)
Now we must hunt-down $out_msg, which turns-out to be set in /usr/share/
function asErr($txt = "") {
global $as_pkg, $out_msg;
$out_msg .= "$as_pkg::error: $txt<br>\n";
}
After some addition running-around, we find that the true origin of this message is in index.php, line 893:
$ret = pre_graph($img, $opts, count($opts));
$infected = 0;
if (!is_array($ret)) {
$msg = rrd_error();
asErr(
return false;
}
We can see that the author's pre_graph() function is returning something other than an array: boolean false. Reading the relevant comments within the source code reveals that this message is likely the result of there being no virus data to graph (which is why the function returns false). This theory coincides with a message that is output just below the graph image: NO VIRUS DATA TO GRAPH.
In summary, this is more of a warning than an error. The code should be modified such that the warning is printed only when rrdtool actually returns an error, in which case calling PHP's rrd_error function yields something other than null:
------------------
899c906,908
< asErr("rrd_graph(): $msg");
---
> if (!empty($msg)) {
> asErr("rrd_graph(): $msg");
> }
------------------
This change addresses the fact that the original code does not exclude the '..' directory operator that PHP's readdir() function returns, which is problematic in this context.
------------------
1053c1062
< if ($file != "." && is_dir(
---
> if ($file != "." && $file != '..' && is_dir(
1069c1078
< if ($file != "." && is_dir(
---
> if ($file != "." && $file != '..' && is_dir(
------------------
This addresses an unset variable warning.
------------------
1091a1101,1103
> }
> else {
> $legend_msg = '';
------------------
------------------ SYSTEM INFORMATION ------------------
# lsb_release -rd
Description: Ubuntu 10.04.4 LTS
Release: 10.04
# apt-cache policy amavis-stats
amavis-stats:
Installed: 0.1.22-1
Candidate: 0.1.22-1
Version table:
*** 0.1.22-1 0
500 http://
100 /var/lib/
This patch resolves the issues identified in this report.