How to reproduce this bug:

1. Edit any existing link within your main menu
2. Take a look at your address bar in your browser and select/copy a part of your URL that looks like this: admin/structure/menu/item/373/edit
3. Paste that text as your new "Path" for the current menu item
4. Save.

It will result with an error message and after a few more refreshes - a WSOD: Fatal error: Allowed memory size of 16777216 bytes exhausted (tried to allocate 2 bytes) in /var/www/drupal7/includes/database/database.inc on line 2040

Forum topic: http://drupal.org/node/794970
D6 bug report: #796984: Self-reference protection

CommentFileSizeAuthor
#24 self_reference_problem-798190-22.patch987 bytespsha
#22 self_reference_problem-798190-22.patch987 bytespsha
#18 self_reference_problem-798190-18.patch921 bytespsha
#15 drupal-798190.patch1.21 KBygerasimov
#12 drupal-798190.patch816 bytesygerasimov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

nice bug! i can confirm this on a clean D7.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

an interesting bug - maybe the problem is in trying to evaluate the access callback? Or in menu_link_load()?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

_menu_link_translate --> _menu_load_objects --> menu_link_load --> _menu_link_translate .... kaboom!

davidwhthomas’s picture

davidwhthomas CreditAttribution: davidwhthomas commented

Confirmed again on D7 dev

The Xdebug backtrace gives:

Fatal error: Maximum function nesting level of '100' reached, aborting! in /var/www/drupal-7.x-dev/includes/database/select.inc on line 688 Call Stack: 0.0001 57984 
1. {main}() /var/www/drupal-7.x-dev/index.php:0 0.0315 1984440 
2. menu_execute_active_handler() /var/www/drupal-7.x-dev/index.php:22 0.0317 1998012 
3. call_user_func_array() /var/www/drupal-7.x-dev/includes/menu.inc:462 0.0317 1998012 
4. drupal_get_form() /var/www/drupal-7.x-dev/includes/menu.inc:0 0.0317 1998012 
5. drupal_build_form() /var/www/drupal-7.x-dev/includes/form.inc:77 0.0318 1998012 
6. drupal_retrieve_form() /var/www/drupal-7.x-dev/includes/form.inc:204 0.0318 1998012 
7. call_user_func_array() /var/www/drupal-7.x-dev/includes/form.inc:567 0.0318 1998056 
8. menu_overview_form() /var/www/drupal-7.x-dev/includes/form.inc:0 0.0327 2008464 
9. menu_tree_check_access() /var/www/drupal-7.x-dev/modules/menu/menu.admin.inc:61 0.0327 2008464 
10. _menu_tree_check_access() /var/www/drupal-7.x-dev/includes/menu.inc:1277 0.0327 2009936 
11. _menu_link_translate() /var/www/drupal-7.x-dev/includes/menu.inc:1288 0.0327 2011256 
12. _menu_load_objects() /var/www/drupal-7.x-dev/includes/menu.inc:792 0.0328 2011984 
13. menu_link_load() /var/www/drupal-7.x-dev/includes/menu.inc:537 0.0337 2026636 
14. _menu_link_translate() /var/www/drupal-7.x-dev/includes/menu.inc:2185 0.0337 2028108 
15. _menu_load_objects() /var/www/drupal-7.x-dev/includes/menu.inc:792 0.0337 2029476
16. menu_link_load() /var/www/drupal-7.x-dev/includes/menu.inc:537 0.0343 2042292

### REPEATS 14, 15 & 16 many times ###

98. db_select() /var/www/drupal-7.x-dev/includes/menu.inc:2179 0.0501 2523968 99. DatabaseConnection->select() /var/www/drupal-7.x-dev/includes/database/database.inc:2175

How could this be patched?

DT

P.S Note: this bug can be reverted at the database level by deleting the menu item from the menu_links table and clearing cache_menu and also occurs in D6

jazzitup’s picture

jazzitup CreditAttribution: jazzitup commented

Maybe it will sound awkward, but let me explain my usecase and how did I get to this bug. It may help you with choosing the right strategy for the future:

There are different roles of users building my site, and one of them (let's call them moderators) just builds the structure and conditional visibility (placeholders, dummy blocks, navigation menus, etc.), some other group (let's call them journalists) builds the content. Most of the time it's much easier for journalists not to think about administering the Drupal site.

Moderators had to prepare the whole main menu (to fill up all the design elements), and since Path is a required field (but they still didn't have the content), somehow it was natural to put their self-reference values to be easier for journalists to fill in their real path values just by clicking on those main menu items later.

Speaking of usability, maybe this could be a useful feature for some cases like mine in the future:

- Path fields for a menu item could be optional.
- All menu items with empty paths could have some kind of a warning visual sign in the menu (core CSS).
- By clicking a menu item (without a path) from the main menu could just bring an AJAX window for a user to enter its existing Path value, with exposing buttons like "Save", "Delete" and "Cancel".

chx’s picture

chx CreditAttribution: chx commented
Priority: Critical » Normal

critical: when a bug renders a module, a core system, or a popular function unusable.

normal: Bugs that just affect one piece of functionality.

jazzitup’s picture

jazzitup CreditAttribution: jazzitup commented

According to this comment, this is not only related to the menu module: http://drupal.org/node/794970#comment-2950302
It could also be applied to node aliases. Maybe we should name all possible occurrences of this issue.

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented

I have tested this bug with Drupal 7.0 alpha 2, 2010-02-21 version and no bug has been detected. I have created menu item to Navigation. Then changed the path to admin/structure/menu/item/325/edit and no WSOD. Can someone check again?

jazzitup’s picture

jazzitup CreditAttribution: jazzitup commented

@ygerasimov: Can you reproduce an error message? Did you try to refresh any page with the error for a couple of times to reproduce WSOD as well?

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented

@madjoe I was not able to reproduce error at all

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented

@madjoe Sorry. I have reproduced error with latest 7.x-dev version. So the bug is still there.

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented
FileSize
drupal-798190.patch816 bytes

I think the problem is in function _menu_link_translate(). If $item has 'load_functions' that is 'menu_link_load', then it will try to load menu item using 'menu_link_load' function. This function will call _menu_link_translate() in its turn. And in this way we come to never ending loop.

I propose to change _menu_link_translate() function and let return TRUE for all items that have load_functions callback menu_link_load.

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented
Status: Active » Needs review
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

patches need to be in unified diff format. (cvs diff -rup)

Also, you've added a check regarding double-unserialize, but I don't see that as any part of the discussion above.

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented
FileSize
drupal-798190.patch1.21 KB

Thank you for the comment. Please review resubmitted patch.

I have added the check as it has been done in function _menu_load_objects() so I decided to keep that code in.

ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-798190.patch, failed testing.

psha’s picture

psha CreditAttribution: psha commented
Status: Needs work » Needs review
FileSize
self_reference_problem-798190-18.patch921 bytes

I think the problem is that when a menu item has its 'mlid' in the link then it is infinitely being loaded, so i propose to go into _menu_load_objects function and check if the current item's mlid is the same as the argument to the load function. For my case it worked.

Status: Needs review » Needs work

The last submitted patch, self_reference_problem-798190-18.patch, failed testing.

psha’s picture

psha CreditAttribution: psha commented
Status: Needs work » Needs review

#18: self_reference_problem-798190-18.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, self_reference_problem-798190-18.patch, failed testing.

psha’s picture

psha CreditAttribution: psha commented
Status: Needs work » Needs review
FileSize
self_reference_problem-798190-22.patch987 bytes
ygerasimov’s picture

ygerasimov CreditAttribution: ygerasimov commented
Status: Needs review » Needs work

Patch has trailing spaces in comments. Also can we add a test to ensure that this will never happen again?

psha’s picture

psha CreditAttribution: psha commented
Status: Needs work » Needs review
FileSize
self_reference_problem-798190-22.patch987 bytes

Here it is better formatted. Haven't thought about a test. Feel free to make one.

johnv’s picture

johnv
Primary language Dutch
CreditAttribution: johnv commented

#24: self_reference_problem-798190-22.patch queued for re-testing.

johnv’s picture

johnv
Primary language Dutch
CreditAttribution: johnv commented
Title: Self-reference protection » WSOD due to self-referencing item in menu tree

Marked #1921412: Menu item with path to itself causes infinite recursion as a duplicate. It has a different patch in the same function. I don't know which is best.

For convenience, this is the patch: 

diff --git a/includes/menu.inc b/includes/menu.inc
index 2be0903..3d03819 100644
--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -561,6 +561,10 @@ function _menu_load_objects(&$item, &$map) {
     foreach ($load_functions as $index => $function) {
       if ($function) {
         $value = isset($path_map[$index]) ? $path_map[$index] : '';
+        // Prevent recursive loading when mlid is self contained in link path.
+        if ($function == 'menu_link_load' && isset($item['mlid']) && $item['mlid'] == $value) {
+          return TRUE;
+        }
         if (is_array($function)) {
           // Set up arguments for the load function. These were pulled from
           // 'load arguments' in the hook_menu() entry, but they need

Also, since _menu_load_objects() is called by _menu_link_translate(), the following issue might prevent loading the same object twice: #753064: _menu_link_translate() might avoid calling _menu_load_objects()