Closed (fixed)
Project:
Drupal core
Version:
5.0-rc2
Component:
menu system
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
13 Jan 2007 at 15:41 UTC
Updated:
15 Mar 2007 at 16:46 UTC
In _menu_sort(), we use two menu references which are incorrect. This code can cause segfaults in certain situations (in either core-only installations or with contrib modules installed - see here, here, and here). Historically, this appears to be an attempt at reference counting. Chx says "since PHP 4, PHP does not copy values ... just creates a reference and only in case one of the variables changes, it does copy. forcing a reference unless needed is totally pointless". The attached patch removes these pointless references.
| Comment | File | Size | Author |
|---|---|---|---|
| _menu_inc_reference.patch | 593 bytes | morbus iff |
Comments
Comment #1
morbus iffComment #2
chx commentedWhy it is pointless? Because we are not changing either
$anor$menu['items'][$a]. "Historically, this appears to be an attempt at reference counting." -- sorry this is not nonsense, because reference counting is the PHP mechanism I described above. The intention was propably to avoid copying of menu items here, however exactly because of reference counting (what I already described above) such copying will not happen (thanks god we do not deal with objects). If Drupal in its early days ran on PHP 3 then this piece was correct. Not any more.If it can cause segfaults then it is a critical.
Comment #3
Steven commentedIt looks like now, we are making a copy of both menu items... couldn't we just use $menu['items'][$a] directly? Is PHP smart enough to optimize this?
I'd like to see some simple benchmarking at least
Comment #4
Steven commentedOk, chx convinced me on IRC that this is optimal.
Committed to HEAD.
Comment #5
(not verified) commentedComment #6
jsenich commentedIs it safe to use this patch on 4.6.x and 4.7.x?
Comment #7
morbus iffjsenich: yes.