PHP code throws warnings unnecessarily in several places

Bug #963254 reported by Ben Johnson
8
This bug affects 1 person
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("/(\d+)x(\d+)/", $output[0], $matches)) {
< $retval["xsize"] = $matches[1];
< $retval["ysize"] = $matches[2];
< } else {
< $rrd_error_val = $output[0];
< return false;
< }
<
< array_shift($output);
<
< $retval["calcpr"] = array();
< foreach ($output as $index => $value) {
< $retval["calcpr"][$index] = $value;
---
>
> if (is_array($output) && !empty($output)) {
> if (preg_match("/(\d+)x(\d+)/", $output[0], $matches)) {
> $retval["xsize"] = $matches[1];
> $retval["ysize"] = $matches[2];
> } else {
> $rrd_error_val = $output[0];
> return false;
> }
>
> array_shift($output);
>
> $retval["calcpr"] = array();
> foreach ($output as $index => $value) {
> $retval["calcpr"][$index] = $value;
> }
------------------

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-specific errors are still triggered.
------------------
292c299
< $readfile = @file($seenfile);
---
> $readfile = file($seenfile);
------------------

This change addresses the mysterious output just below the "Generated by amavis-stats..." line:

amavis-stats::error: rrd_graph():

This output is caused in /usr/share/amavis-stats/templates/standard/overall_footer.tpl, line 10:

<td align="left">{OUT_MSG}</td>

If we trace this variable back to its origin, we find that it is set in /usr/share/amavis-stats/includes/page_tail.php, line 35:

'OUT_MSG' => $out_msg ? $out_msg : NULL)

Now we must hunt-down $out_msg, which turns-out to be set in /usr/share/amavis-stats/index.php, line 250:

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("rrd_graph(): $msg");
 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("$libdir/$file")) {
---
> if ($file != "." && $file != '..' && is_dir("$libdir/$file")) {
1069c1078
< if ($file != "." && is_dir("$libdir/$file")) {
---
> if ($file != "." && $file != '..' && is_dir("$libdir/$file")) {
------------------

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://us.archive.ubuntu.com/ubuntu/ lucid/universe Packages
        100 /var/lib/dpkg/status

Tags: patch
Revision history for this message
Ben Johnson (a03-6eo-chg) wrote :

This patch resolves the issues identified in this report.

Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "Patch to resolve PHP warnings in amavis-stats" of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

tags: added: patch
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.