Module Development and Potential Vulnerability

Hi,
Today I had a very nasty 'shock' whilst developing a module for 6.2 and it took some time to figure out what had happened. To a newbie module developer this could be a catastrophic incident because the cause is not that obvious and the solution even less so.

SYMPTOM:
All menu routing and page requests result in a page not found error after the fault takes place.

CAUSE:
In the menu_router_build() function the DELETE statement is executed *before* the new set of callbacks have been produced, this means that a 'malicious' module could wipe out the entire menu router table and completely disable the operation of a site.

SUGGESTED RECOVERY:
Re-import the menu_router table from an SQL backup or a default installation which should restore the administrative functionality allowing further restorative actions to be undertaken.

HOW TO RE-CREATE:
Put a die statement in your hook_menu() implementation. I did this very innocently to see what was going on as I had some issues with what I was doing at the time.

SUGGESTED FIX / IMPROVEMENT:
The current implementation of menu_router_build() is as follows:

/**
 * Collect, alter and store the menu definitions.
 */
function menu_router_build($reset = FALSE) {
  static $menu;

  if (!isset($menu) || $reset) {
    if (!$reset && ($cache = cache_get('router:', 'cache_menu')) && isset($cache->data)) {
      $menu = $cache->data;
    }
    else {
      db_query('DELETE FROM {menu_router}');
      // We need to manually call each module so that we can know which module
      // a given item came from.
      $callbacks = array();
      foreach (module_implements('menu') as $module) {
        $router_items = call_user_func($module .'_menu');
        if (isset($router_items) && is_array($router_items)) {
          foreach (array_keys($router_items) as $path) {
            $router_items[$path]['module'] = $module;
          }
          $callbacks = array_merge($callbacks, $router_items);
        }
      }
      // Alter the menu as defined in modules, keys are like user/%user.
      drupal_alter('menu', $callbacks);
      $menu = _menu_router_build($callbacks);
      cache_set('router:', $menu, 'cache_menu');
    }
  }
  return $menu;
}

You can see that the DELETE FROM takes place before the new callback structure has been composed which means that anything that causes this loop to break / die / terminate prematurely (i.e. an unstoppable "die" or un-recoverable error) will leave the table totally empty and any future page requests result in a page not found response.

Therefore I suggest that the DELETE FROM be moved after the loop, when the new callback data is available for updating the database.

I have tested this change on my development system and putting a 'die' statement in the hook_menu() function no longer disables the system. I am leaving this change in !

/**
 * Collect, alter and store the menu definitions.
 */
function menu_router_build($reset = FALSE) {
  static $menu;

  if (!isset($menu) || $reset) {
    if (!$reset && ($cache = cache_get('router:', 'cache_menu')) && isset($cache->data)) {
      $menu = $cache->data;
    }
    else {
      // We need to manually call each module so that we can know which module
      // a given item came from.
      $callbacks = array();
      foreach (module_implements('menu') as $module) {
        $router_items = call_user_func($module .'_menu');
        if (isset($router_items) && is_array($router_items)) {
          foreach (array_keys($router_items) as $path) {
            $router_items[$path]['module'] = $module;
          }
          $callbacks = array_merge($callbacks, $router_items);
        }
      }
      // Alter the menu as defined in modules, keys are like user/%user.
      drupal_alter('menu', $callbacks);
      // SUGGESTED CHANGE POINT: =>
      // New data available: delete old routing information now.
      db_query('DELETE FROM {menu_router}');
      // END CHANGE-POINT
      $menu = _menu_router_build($callbacks);
      cache_set('router:', $menu, 'cache_menu');
    }
  }
  return $menu;
}

For completeness I have added the debug_stacktrace output so you can see how it got to where it did from the root controller, index.php:

Array
(
    [0] => Array
        (
            [function] => ppvadmin_menu
            [args] => Array
                (
                )

        )

    [1] => Array
        (
            [file] => /Library/WebServer/Documents/includes/menu.inc
            [line] => 1666
            [function] => call_user_func
            [args] => Array
                (
                    [0] => ppvadmin_menu
                )

        )

    [2] => Array
        (
            [file] => /Library/WebServer/Documents/includes/menu.inc
            [line] => 1641
            [function] => menu_router_build
            [args] => Array
                (
                    [0] => 1
                )

        )

    [3] => Array
        (
            [file] => /Library/WebServer/Documents/modules/system/system.admin.inc
            [line] => 623
            [function] => menu_rebuild
            [args] => Array
                (
                )

        )

    [4] => Array
        (
            [function] => system_modules
            [args] => Array
                (
                    [0] => Array
                        (
                            [storage] => 
                            [submitted] => 
                            [post] => Array
                                (
                                )

                        )

                )

        )

    [5] => Array
        (
            [file] => /Library/WebServer/Documents/includes/form.inc
            [line] => 358
            [function] => call_user_func_array
            [args] => Array
                (
                    [0] => system_modules
                    [1] => Array
                        (
                            [0] => Array
                                (
                                    [storage] => 
                                    [submitted] => 
                                    [post] => Array
                                        (
                                        )

                                )

                        )

                )

        )

    [6] => Array
        (
            [function] => drupal_retrieve_form
            [args] => Array
                (
                    [0] => system_modules
                    [1] => Array
                        (
                            [storage] => 
                            [submitted] => 
                            [post] => Array
                                (
                                )

                        )

                )

        )

    [7] => Array
        (
            [file] => /Library/WebServer/Documents/includes/form.inc
            [line] => 102
            [function] => call_user_func_array
            [args] => Array
                (
                    [0] => drupal_retrieve_form
                    [1] => Array
                        (
                            [0] => system_modules
                            [1] => Array
                                (
                                    [storage] => 
                                    [submitted] => 
                                    [post] => Array
                                        (
                                        )

                                )

                        )

                )

        )

    [8] => Array
        (
            [function] => drupal_get_form
            [args] => Array
                (
                    [0] => system_modules
                )

        )

    [9] => Array
        (
            [file] => /Library/WebServer/Documents/includes/menu.inc
            [line] => 346
            [function] => call_user_func_array
            [args] => Array
                (
                    [0] => drupal_get_form
                    [1] => Array
                        (
                            [0] => system_modules
                        )

                )

        )

    [10] => Array
        (
            [file] => /Library/WebServer/Documents/index.php
            [line] => 18
            [function] => menu_execute_active_handler
            [args] => Array
                (
                )

        )

)