Plugin called menu_display when item=MENU_EXIT

[ivani6651]

Since I put the latest version on amxmodx, I get these bugs on every plugin with reasons "Plugin called menu_display when item=MENU_EXIT".

L 10/20/2018 - 01:16:29: [mapmanager.amxx] StartVote: timeleft 111 L 10/20/2018 - 01:16:34: Plugin called menu_display when item=MENU_EXIT L 10/20/2018 - 01:16:34: [AMXX] Displaying debug trace (plugin "mapmanager.amxx", version "2.5.61") L 10/20/2018 - 01:16:34: [AMXX] Run time error 10: native error (native "show_menu") L 10/20/2018 - 01:16:34: [AMXX] [0] mapmanager.sma::VoteMenu (line 1664) L 10/20/2018 - 01:16:34: [AMXX] [1] mapmanager.sma::ShowVoteMenu (line 1579) L 10/20/2018 - 01:16:34: [AMXX] [2] mapmanager.sma::ShowTimer (line 1544) L 10/20/2018 - 01:16:49: Plugin called menu_display when item=MENU_EXIT L 10/20/2018 - 01:16:49: [AMXX] Displaying debug trace (plugin "mapmanager.amxx", version "2.5.61") L 10/20/2018 - 01:16:49: [AMXX] Run time error 10: native error (native "show_menu") L 10/20/2018 - 01:16:49: [AMXX] [0] mapmanager.sma::Task_Timer (line 1590)

[Arkshine]

Please provide a link to the plugin or attach it here.

[WPMGPRoSToTeMa]

@Arkshine can this happen when a plugin wants to open another menu (e.g. previous menu) on exit from the current menu?

[ivani6651]

Please provide a link to the plugin or attach it here.

https://dev-cs.ru/resources/105/

[Arkshine]

@WPMGPRoSToTeMa I think so. It sounds like a plugin trying to force-show its menu until you choose something. Then when this mapchooser shows a menu, the previous menu can't be closed because displayed again on MENU_EXIT. Basically, it's a recursion, AMXX detects it and it will throw an error after a few times. (it doesn't seem the case for this plugin)

It seems to be related to https://github.com/alliedmodders/amxmodx/pull/335, but...

1. It starts with #21. Fixed a bug where calling show_menu would not fire the newmenu callback if the client was already viewing a newmenu. [...] This could result in menu handle leaks and is theoretically even client exploitable

2. Then #140, which is essentially a side-effect fix of #21. A buffer issue which could lead to a recursion.

3. Finally, #335, which is essentially a side-effect fix of #140. Now, it tries to close up to 10 newmenus before throwing an error. Typically, situation like, you are in a submenu, and when closed, the main menu is displayed.

Nore sure if there is something to fix. A plugin should not redraw a menu on MENU_EXIT (at least not the root menu), we would need some kind of system to deal with priority or whatever. Don't know.

@ivani6651 Can you attach or send me by PM (dev-cs.ru or alliedmods) all your plugins you're using?

[stambeto2006]

@WPMGPRoSToTeMa Yes. I have the same problem only with another plugin. L 10/15/2018 - 21:23:10: Plugin called menu_display when item=MENU_EXIT L 10/15/2018 - 21:23:10: [AMXX] Displaying debug trace (plugin "admin_spec_esp.amxx", version "1.3") L 10/15/2018 - 21:23:10: [AMXX] Run time error 10: native error (native "show_menu") L 10/15/2018 - 21:23:10: [AMXX] [0] admin_spec_esp.sma::show_esp_menu (line 132) L 10/15/2018 - 21:23:10: [AMXX] [1] admin_spec_esp.sma::client_PreThink (line 288)

[Arkshine]

@stambeto2006 What other plugin exactly? Just curious to see the code.

[stambeto2006]

@Arkshine https://www.mediafire.com/file/4gx5sioxyc1z23c/admin_spec_esp.sma/

[Arkshine]

I meant the other plugin. I understand admin_spec_esp triggers this error because you have another plugin displaying a menu. I want to see this plugin.

[stambeto2006]

@Arkshine When I'm a spectator and I can not close this menu csdm_equip too.

[OciXCrom]

@Arkshine He meant he has the same problem with a plugin different than the one mentioned here. He's talking about the admin_spec_esp one.

[WPMGPRoSToTeMa]

A plugin should not redraw a menu on MENU_EXIT (at least not the root menu)

~I think we can't ignore that, because looks like #21 partially breaks backward compatibility.~ The problem is that there is no way to distinguish between normal exit and forced exit. I can't imagine a situation when you need to open an another menu on forced exit, but on normal exit it is ok. So, I think we can add MENU_NORMAL_EXIT and MENU_FORCED_EXIT (or something like MENU_CANCEL?) that are called before/after MENU_EXIT.

[Arkshine]

@WPMGPRoSToTeMa I guess it would be welcomed to avoid any behavior change.

What about keeping track of newmenu forced closed so to be freed at mapchange if available, and then as you said notifying the plugin with MENU_CANCEL? We keep old behavior, we still avoid memory leak and we give a way to know when a menu is forced closed.

[stambeto2006]

Any solution to the problem?

[WPMGPRoSToTeMa]

@WPMGPRoSToTeMa I guess it would be welcomed to avoid any behavior change.

We can use a separate handler function for this to avoid it.

What about keeping track of newmenu forced closed so to be freed at mapchange if available, and then as you said notifying the plugin with MENU_CANCEL?

Does it change the old behavior? I think we can change it only for show_menu, because menu closing in it was introduced in 1.8.3 (1.9).

By the way, looks like there was no infinite loops in the reported cases with the old amxmodx. Is it a regression by #335 or by another PR?

[Arkshine]

I get it, #335 is fine but it's a regression due to the behavior change (if showmenu overwrites a newmenu; and that newmenu displays again on MENU_EXIT, show_menu will return a recursion error instead of letting the newmenu).

To fix this behavior change we need to do: "if show_menu overwrites a newmenu and can't take over, don't display the show_menu. "

Actually, there is #471 which was on the right track: https://github.com/alliedmodders/amxmodx/commit/387dc6a1882b3362f51cd2c8f9b71aa8596b674f#diff-487c6cf9264c2a33d9839f854ad014ec. If a recursion was detected it would return 0 basically. There are two typos in this PR though : 1) it should not return an error if a newmenu can't be closed 2) the second closeMenu() should be == 2 as well. By fixing those 2 typos, it should restore the old behavior.