alliedmodders / sourcemod

SourceMod - Source Engine Scripting and Administration
http://www.sourcemod.net/
985 stars 423 forks source link

SDKHook_Use doesn't respect return Plugin_Handled. #1870

Closed nikita1824 closed 1 year ago

nikita1824 commented 1 year ago

Help us help you

Environment

Description

When trying to prevent players to use entities with sdkhooks, SDKHook_Use doesn't prevent it when function return Plugin_Handled. But if we unload plugin and then load it again, plugin works fine (late load?).

Problematic Code (or Steps to Reproduce)

public void OnEntityCreated(int iEnt, const char[] szClsName) {
    if (strcmp(szClsName, "prop_minigun") == 0 || strcmp(szClsName, "prop_minigun_l4d1") == 0 || strcmp(szClsName, "prop_mounted_machine_gun") == 0)
        SDKHook(iEnt, SDKHook_Use, OnUseMinigun);
}

Action OnUseMinigun(int iEnt, int iActivator, int iCaller, UseType uType, float fValue) {
    if (iCaller <= 0)                return Plugin_Continue;
    if (iCaller > MaxClients)        return Plugin_Continue;
    if (!IsClientInGame(iCaller))    return Plugin_Continue;
    if (GetClientTeam(iCaller) != 2) return Plugin_Continue;
    if (uType != Use_Toggle)         return Plugin_Continue;
    ...
    return Plugin_Handled; << it works but anyway it allows players to use entity
}
psychonic commented 1 year ago

and have provided the absolute smallest test-case possible.

Do you happen to have this? A pared-down version of what's currently included, that is in a compilable state and still reproduces the issue?

Also, have you confirmed via logging that the return Plugin_Handled line is getting called rather than one of your various early outs getting hit?

nikita1824 commented 1 year ago

I'm sorry for not providing enough info. We started to notice the problem when we updated Sourcemod to 1.11 (the first stable release). The code that is provided still reproduces the issue. I'll provide video and code with debug that I used to confirm that the problem is not in the code above. url: https://youtu.be/MDdJO8qLn4A

#pragma newdecls required
#pragma semicolon 1

#include <sourcemod>
#include <sdkhooks>

public void OnEntityCreated(int iEnt, const char[] szClsName) {
    if (strcmp(szClsName, "prop_minigun") == 0 || strcmp(szClsName, "prop_minigun_l4d1") == 0 || strcmp(szClsName, "prop_mounted_machine_gun") == 0) {
        PrintToChatAll("spawned %s (%d)", szClsName, iEnt);
        SDKHook(iEnt, SDKHook_Use, OnUseMinigun);
    }
}

Action OnUseMinigun(int iEnt, int iActivator, int iCaller, UseType uType, float fValue) {
    if (iCaller <= 0) {
        PrintToChatAll("iCaller <= 0 (%d)", iCaller);
        return Plugin_Continue;
    }
    if (iCaller > MaxClients) {
        PrintToChatAll("iCaller > MaxClients (%d)", iCaller);
        return Plugin_Continue;
    }
    if (!IsClientInGame(iCaller)) {
        PrintToChatAll("iCaller (%d) is not in game", iCaller);
        return Plugin_Continue;
    }
    if (GetClientTeam(iCaller) != 2) {
        PrintToChatAll("iCaller (%d) is not in survivors team", iCaller);
        return Plugin_Continue;
    }
    if (uType != Use_Toggle) {
        PrintToChatAll("uType != Use_Toggle (uType == %d)", view_as<int>(uType));
        return Plugin_Continue;
    }
    PrintToChatAll("return Plugin_Handled");
    return Plugin_Handled;
}
nikita1824 commented 1 year ago

I checked more carefully. It seems like it was a conflict with another plugin. One that was loaded later had a priority. Is it intentional if we hook the same entity in different plugins, one of them that was loaded later has a priority? If so, I'm sorry for your time.

psychonic commented 1 year ago

There is no defined order when two plugins hook the same function.

If you return Plugin_Handled on any of them, it should block the game function. If you return Plugin_Stop, on one, any plugin hooks on the same function, in other plugins, will not run if they have not already run.

peace-maker commented 1 year ago

The Use pre-hook doesn't implement that logic though, so this is a valid bug still. https://github.com/alliedmodders/sourcemod/blob/5d391fda073a2431b0314ecb3ce3522847476a3f/extensions/sdkhooks/extension.cpp#L1608-L1612

It returns what the latest callback in the list returned instead of the highest value.

peace-maker commented 1 year ago

If you return Plugin_Stop, on one, any plugin hooks on the same function, in other plugins, will not run if they have not already run.

That behavior of normal forwards isn't implemented in sdkhooks, since it uses it's own callback list of IPluginFunctions instead of private forwards. So Plugin_Stop and Plugin_Handled are the same as far as sdkhooks hook callbacks are concerned. Probably something we should streamline as well?