pfSense

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.

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.

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.

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?

Lorenz: I would vote for config.lib.inc

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?

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

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.

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.