Bugzilla – Bug 3243
PLUGIN_HANDLED_MAIN, PLUGIN_HANDLED - wrong return processing(order breached)
Last modified: 2014-02-08 23:52:29 PST
in amxmodx-1.8.1-scr/dlls/engine/forwards.cpp functions: void CmdStart(const edict_t *player, const struct usercmd_s *_cmd, unsigned int random_seed) void pfnTouch(edict_t *pToucher, edict_t *pTouched) void Think(edict_t *pent) fragment <code> retVal=MF_ExecuteForward(Thinks[i]->Forward, (cell)ENTINDEX(pent)); if (retVal & 2/*PLUGIN_HANDLED_MAIN*/) RETURN_META(MRES_SUPERCEDE); else if (retVal) res=MRES_SUPERCEDE;</code> should be changed to <code> retVal=MF_ExecuteForward(Thinks[i] ->Forward, (cell)ENTINDEX(pent)); if (retVal & 2/*PLUGIN_HANDLED_MAIN*/) res=MRES_SUPERCEDE; else if (retVal) RETURN_META(MRES_SUPERCEDE);</code> short details /scripting/include/amxconst.inc <code>#define PLUGIN_CONTINUE 0 //Results returned by public functions #define PLUGIN_HANDLED 1 //stop other plugins #define PLUGIN_HANDLED_MAIN 2 //continue all plugins but stop the command</code>
Created attachment 2482 [details] [review] Proposed patch
For CmdStart (which is only used for client_impulse and register_impulse), function shouldn't be supercede as cmd.impuse is already set to 0 and that it is enough. From what i remember, [g/s]et_usercmd make server crash (wouldn't require to supercede though).
Comment on attachment 2482 [details] [review] Proposed patch Review of attachment 2482 [details] [review]: ----------------------------------------------------------------- Sorry for the late reply - I'm going to pass on this for AMX Mod X 1.8.2. The PLUGIN_HANDLED_MAIN changes look fine, but the ones to PLUGIN_HANDLED could cause two plugins to interfere whereas they wouldn't before. Sadly we muddled the meaning of PLUGIN_HANDLED here. To make things worse the names were poorly chosen and are logically in the wrong order. Open to suggestions but it is so late in the AMX Mod X game that I'd rather not risk busting user code.
After having read more carefully code, this is what i can say : get_usercmd and set_usercmd have never worked and crash server when are used, so, should be fixed and shouldn't be taken in account for backward compatibility. return a value different from PLUGIN_CONTINUE makes the function CmdStart being superceded, that is a bad thing considering it doesn't only handle impulse but lot of other properties. This should be taken in account more seriously than the next question "do we send single forwards to following plugins if PLUGIN_HANDLE or PLUGIN_HANDLE_MAIN is returned ?" client_impulse is sent on each CmdStart function call, where it should only be called on CmdStart + cmd.impulse != 0 What could be done : Send single forwards (from register_impulse()), if return is not PLUGIN_CONTINUE, set cmd.impulse to 0, and then, if return is PLUGIN_HANDLE, don't send next single forwards, else if return is PLUGIN_HANDLE_MAIN, continue to send forwards. Since CmdStart is not superceded anymore, it makes a major change, and interrogations about PLUGIN_HANDLE and PLUGIN_HANDLE_MAIN that were inverted for a long time and that we should or not let like this are not significant anymore. Amxx routine gonna just be consistant and that's a good point. Servers administrators should know that plugins.ini plugins order is important. Send client_impulse forward only when cmd.impulse is different from 0 (original value before single forwards are sent, but it is already the case), and don't allow in there [g/s]et_usercmd usage. Create a new forward sent on each CmdStart, client_CmdStart forward for example where only player index is passed and where [g/s]et_usercmd is allowed. Since [g/s]et_usercmd were bugged and client_impulse was only used in plugins for impulse stuff, i thing the 2 previous changes should be ok. My poor english makes my argumentation approximative, but it is clear in my head lol, let's discuss it now ;)
Forgot to say that [g/s]et_usercmd fix has already been found by arkshine : struct usercmd_s *g_cmd = (struct usercmd_s *)_cmd; should be g_cmd = (struct usercmd_s *)_cmd; New local variable was created in CmdStart fonction, with same name as global variable. [g/s]et_usercmd tries to set or retrieve global variable value that is never handled a cmd pointer, so it was crashing.
Last, i've just realized that this report was only on PLUGIN_HANDLE and PLUGIN_HANDLE_MAIN behavior, and on other places that CmdStart function. For other fonctions (Think, Touch), i think it is ok to let as it is, as argued in comment #3
I would agree with connorr. We are in a special case where fixing return will not change anyway the very wrong behavior of impulse forwards, which block any others "cmd" properties when return is not _CONTINUE. Therefore, considering : - Blocking all "cmd" properties just to block impulse is a critical issue : It leads to nasty bugs and knowing a lot of plugins are hooking CmdStart to alter such properties, is worse than anything else. The behavior must be changed. - The use of register_impulse is rather specific, in the meaning, that's not something heavily used like Think/Touch in different contexts. Most of plugins are to hook spray/flashlight, the possibility user has installed two same kind of plugins is near of 0, which means the concern stated in #3 would be unlikely. And even if it exists for one user, the importance of resolving first point is superior than a possibility of interference. I would conclude connorr suggestions make sense and are acceptable. Unless David is against, I would encourage connorr to create a separate issue with those changes.
Created attachment 3422 [details] [review] Proposed patch This fixes the incorrect behavior of impulse forwards (single and multi). To block an impulse, we do not need to supercede ; setting g_cmd->impulse to 0 is enough. Also, we do not need to return on single forwards since we don't want to break client_impulse being not called.
Pushed changeset: http://hg.alliedmods.net/amxmodx-central/rev/ebba9f21db9a Changelog: Fix incorrect behavior of impulse forwards (bug 3243, r=Arkshine)
Commit pushed to master at https://github.com/alliedmodders/amxmodx https://github.com/alliedmodders/amxmodx/commit/90e7aa65a19316f0bd684303b314ad22b1362f09 Fix incorrect behavior of impulse forwards (bug 3243, r=Arkshine) Former-commit-id: 1aae4c3ec57c61ba3f32579e236b07da54ae14bf