Fix ancient register_menuid bug, using strstr instead of strcmp

[IgnacioFDM]

If you registered a menu whose title was a superset of an already
registered menu, it wouldn't register a new handler but use the
handler of the subset title menu

Test plugin

#include <amxmodx>
#include <amxmisc>

public plugin_init()
{
	register_menu("Test menu", 1, "test_menu");
	register_menu("Test menu advanced", 1, "test_menu_advanced");
	
	register_clcmd("say /testmenu", "show_test_menu");
	register_clcmd("say /testmenuadvanced", "show_test_menu_advanced");
}

public show_test_menu(id)
{
	show_menu(id, 1, "Test menu, press 1", -1, "Test menu");
	client_print(id, print_chat, "Showing Test menu");
}

public show_test_menu_advanced(id)
{
	show_menu(id, 1, "Test menu advanced, press 1", -1, "Test menu advanced");
	client_print(id, print_chat, "Showing Test menu advanced");
}

public test_menu(id, key)
{
	client_print(id, print_chat, "Test menu handler");
	
	return PLUGIN_HANDLED;
}

public test_menu_advanced(id, key)
{
	client_print(id, print_chat, "Test menu advanced handler");
	
	return PLUGIN_HANDLED;
}

(By the way amxmodx won't compile with the latest AMTL)

[Nextra]

Can you give a little explanation on expected vs actual results? This looks like a possible breaking change to me.

[IgnacioFDM]

Check the test plugin

Expected

If /testmenu is typed, it should call show_test_menu(), and after pressing a key (1) it should call test_menu() handler
If /testmenuadvanced is typed, it should call show_test_menu_advanced(), and after pressing a key (1) it should call test_menu_advanced() handler

Actual

If /testmenu is typed, it calls show_test_menu(), and after pressing a key (1) it calls test_menu() handler
If /testmenuadvanced is typed, it calls show_test_menu_advanced(), and after pressing a key (1) it calls test_menu handler

Note that with this bug, the actual result is order dependent too.

If I were to register the menus in this order

	register_menu("Test menu advanced", 1, "test_menu_advanced");
	register_menu("Test menu", 1, "test_menu");

It would work as expected.

This won't be breaking because although the test plugin does change behavior, no actual plugin would be like this since it would be a non-working plugin. For all currently working plugins there is no behavior change. This fix simply allows you to have title names that were not possible before.

[WildCard65]

@Nextra after reading documentation of strstr, the bug makes sense, strstr finds the first instance of substr in str and returns a pointer to where it is in str, NULL if substr isn't found. This forces AMXX plugins to think about regristration order of menus where 1 handler name is part of another where strcmp returns 0 if both strings are equal.

[Arkshine]

This is going to break the hooking of existing game menus if the full string is not used.

[IgnacioFDM]

Yeah but I would guess there isn't even a single plugin out there that doesn't use the full string since this "possibility" isn't documented anywhere (besides the fact that why would you even do that).

[Nextra]

You need to be careful with such statements. I'd say that hooking game menus is exactly the reason why strstr was chosen here in the first place. The current code is not outright buggy, even if the behavior is somewhat unexpected, so we need to consider the implications.

[WPMGPRoSToTeMa]

@Arkshine what is the purpose of using part of the string (but not full) for menu hooking?

[Arkshine]

I don't know. It's difficult to evaluate how abused that behavior is. Changing this is likely to not end well.

Actually I tried to search randomly and I've found right away a script which uses Terrorist_Select instead of #Terrorist_Select. We definitely can't break this.

We could keep strstr for game menu in order to preserve backward compatibility (there are not much menus) and using strcmp for others internal plugin menu. Not sure if it makes sense though. Or we could document it considering there is not really a dramatic bug.

[IgnacioFDM]

We could keep strstr for game menu in order to preserve backward compatibility (there are not much menus) and using strcmp for others internal plugin menu.

I guess this would be a good compromise. For internal menus I've been bitten by this bug many times already throughout the last 5 or 10 years. Never bothered to investigate the actual bug until now because of laziness (I knew that they got routed wrongly, and that changing the name to something different somehow fixed it). I'm surprised this "bug" or "feature" isn't more known considering how likely it is to name two menus in such a way.

[Arkshine]

To be honest this issue is not really a "bug' and is quite minor. I'm not sure it's worth to mess the ugly core code just for that because of legacy breakage. Both solutions are acceptable, but just documenting the behavior is probably the way to go, I think.

[rsKliPPy]

Maybe add a new optional parameter to register_menuid and register_menu to decide whether or not to do a substring or whole string search, with default being the former? Even register_menuid2 and register_menu2 will suffice I guess, with documentations of the old ones pointing to the new ones as a note.

[IgnacioFDM]

Another option would be to add the special case of missing # (as in Arkshine example of Terrorist_Select) since that's probably the only situation where plugins are using a substring of a menu title, and in terms of code it adds basically no complexity at all.

[Nextra]

Ignacio you're moving on very thin ice with these claims. We can't change this behavior based on guesses and anecdotal evidence. The current core code is abused by plugins, and we won't be able to tell to what extent. We should absolutely keep doing what we're doing in this case.

This leaves adding a new native or additional parameters, and by that point we might want to reconsider the magnitude of this issue.