Project

General

Profile

Bug #434

xmlparse.inc should not call die

Added by Lorenz Schori over 9 years ago. Updated almost 9 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
-
Target version:
Start date:
03/17/2010
Due date:
% Done:

0%

Estimated time:
Affected Version:
All
Affected Architecture:

Description

In pfsense-mainline:

$ git grep '\bdie\b' etc/inc/xmlparse.inc
etc/inc/xmlparse.inc:           die(sprintf("XML error: %s at line %d cannot occ
etc/inc/xmlparse.inc:           die("Error: could not open XML input\n");
etc/inc/xmlparse.inc:           die("XML error: no $rootobj object found!\n");

Client code should have a chance to handle valid XML files which simply happen to lack some required tags ($rootobj). In my opinion it would be better to return false or an error instead of killing the whole script - at least when a recoverable error occures (like in the last case).

See #431 attachement console-log-after.txt for an example where this behaviour is harmful.

Associated revisions

Revision 541989d5 (diff)
Added by Ermal Luçi over 9 years ago

Ticket #434. Do not die when parsing config since we know how to recover. Only die during packages.

History

#1 Updated by Scott Ullrich over 9 years ago

I am on the fence concerning this one. Once you reach an error condition in the xml parser just allowing it to continue such as the cannot occur error could lead to wider spread xml corruption. These safe-guards are in place to help protect the xml integrity.

#2 Updated by Lorenz Schori over 9 years ago

Ok, then we need a function which can check if a specified root-tag is contained in a file. E.g:

function xml_config_has_root($cffile, $rootobj) ...

Or perhaps something checking the doctype.

Otherwise its not possible to distinguish between <pfsensewizard>- and <packagegui>-style files near pkg-utils.inc:255.

#3 Updated by Ermal Luçi over 9 years ago

  • Status changed from New to Feedback

I think for packages is not neccessary to do this.
For config.xml i committed some fixes.

#4 Updated by Lorenz Schori over 9 years ago

Ermal Luçi wrote:

I think for packages is not neccessary to do this.
For config.xml i committed some fixes.

Sorry but this fix does not improve the situation. The problem lies in sync_package from pkg-utils.inc and not the in some package code (tinydns). I'll try to explain the situation using the following exceprt of sync_package:

320  if($sync_depends == true) {
321    $depends = get_pkg_depends($pkg_name, ".xml", "files", 1); // Call dependency handler and do a little more error checking.
322    if(is_array($depends)) {
323      foreach($depends as $item) {
324        if(!file_exists($item)) {
325          file_notice($package['name'], "The {$package['name']} package is missing required dependencies and
must be reinstalled.", "Packages", "/pkg_mgr_install.php?mode=reinstallpkg&pkg={$package['name']}", 1);
326          log_error("Could not find {$item}. Reinstalling package.");
327          install_package($pkg_name);
328          uninstall_package_from_name($pkg_name);
329          install_package($pkg_name);
330        } else {
331          $item_config = parse_xml_config_pkg($item, "packagegui");
332          if(isset($item_config['nosync'])) continue;
333          if($item_config['custom_php_command_before_form'] <> "") {
334            eval($item_config['custom_php_command_before_form']);
335          }
336          if($item_config['custom_php_resync_config_command'] <> "") {
337            eval($item_config['custom_php_resync_config_command']);
338          }
339          if($show_message == true) print " " . $item_config['name'];
340        }
341      }
342    }
343  }

The array returned by get_pkg_depends on line 321 contains every XML file specified by the additional_files_needed tag of the package master XML file (say tinydns.xml). Later on Line 331 parse_xml_config_pkg is called with "packagegui" as the expected rootobj. If any package includes XML files other than packagegui the script calling sync_packages will die as soon as such files get parsed.

I suggest the following approach:

  • Create class XMLUnexpectedRootObject extends Exception
  • Replace die with throw new XMLUnexpectedRootObejct('blabla')
  • Catch and ignore those exceptions in the code in pkg-utils.inc

This approach will not have any side effect on other parts of the system because unhandled exception lead to program termination - just like die.

By the way, there is another problem with sync_package. The sanitation (reinstall pkg) and the execution of the custom hooks must be seperated into two loops (actually reinstallation should not be in the loop at all). Otherwise custom php functions aren't executed for files which triggered a packag reinstallation.

#5 Updated by Lorenz Schori over 9 years ago

Ok, I'm willing and able to put together a patch which solves this issue properly and with minimal changes. I'm unsure though where to inject a call to set_exception_handler in order to propagate unhandled exceptions to either util.inc:log_error or notices.inc:file_notice or both. Perhaps config.inc? If yes where?

#6 Updated by Chris Buechler over 9 years ago

  • Status changed from Feedback to New
  • Target version set to 2.0

#7 Updated by Scott Ullrich about 9 years ago

Lorenz: I would vote for config.lib.inc

#8 Updated by Jim Pingle about 9 years ago

The function cleanup_backupcache() is also adversely affected by the die() calls in xmlparse.inc. It tries to detect a corrupt backup in /conf/backups/ so it can be deleted but the parser die()s before it can return an error, so the corrupt backup is never removed. This can lead to a corrupt backup file bombing the GUI any time a save is attempted.

Perhaps an additional optional argument to parse_xml_config and parse_xml_raw that allows the caller to opt not to die (and instead return an error) would be helpful to circumvent this for non-fatal cases?

#9 Updated by Jim Pingle about 9 years ago

Looks like the cleanup_backupcache() case is handled properly after all. If I manually corrupt a backup, it is detected and removed properly.

#10 Updated by Ermal Luçi about 9 years ago

An exception handler will not do any good here since its a fatal error.
All the package code needs to be patched to learn about the parser failing and handle it 'gracefully'.

So its more a matter of finding all teh calls to the parser in the package code and have a solution, as what to do, in case it fails.

#11 Updated by Lorenz Schori about 9 years ago

Ermal Luçi wrote:

An exception handler will not do any good here since its a fatal error.

Ermal, please read my original proposition. I suggest that we replace die with a custom Exception in order to enable client code to handle this fatal case in different ways. In the current situation the package synchronization and thus the sanitation code at system startup just plain fails if there are xml files of different content in a package.

Exceptions are the proper way to handle this situation.

#12 Updated by Ermal Luçi almost 9 years ago

  • Status changed from New to Feedback

I made it return just an empty array. That should suffice to solve this.

#13 Updated by Chris Buechler almost 9 years ago

  • Status changed from Feedback to Resolved

Also available in: Atom PDF