weixin_39583623
weixin_39583623
2020-12-02 19:19

Additional player validation checks

该提问来源于开源项目:alliedmodders/amxmodx

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

15条回答

  • weixin_39668408 weixin_39668408 5月前

    Core has CDetour class to hook functions, but I think it might be a good idea to wait the other PR about gameconfig be merged, so, we could create en entry with this function in a gamedata file. It will be easier if I do it myself later, but let's wait for now. Thanks for finding this issue.

    点赞 评论 复制链接分享
  • weixin_39725756 weixin_39725756 5月前

    Sorry for interrupting you guys, but don't you think that doing things via API is easier,more portable and more sane than searching functions by their signatures?

    点赞 评论 复制链接分享
  • weixin_39675728 weixin_39675728 5月前

    Assuming you're talking about the Metamod API: 1) Easier is debatable. Detouring Steam_NotifyClientDisconnect is very easy once the signature is available. 2) Hardly anything about AMXX is portable, and with an eventual gamedata updater the approach is more viable than ever. 3) This PR is a band-aid fix, Arkshines solution tackles the root cause of ClientDisconnect not being called under certain circumstances (as AMXX incorrectly assumes). This will also serve every written plugin automatically, because I'm very sure a large portion of plugin writers (myself included) have up until now assumed that client_connect and client_disconnect are paired calls.

    点赞 评论 复制链接分享
  • weixin_39668408 weixin_39668408 5月前

    Are you talking about Steam API ? It's true we could use ISteamGameServer directly. I've actually made a module around that, completely forgot. Then yes we would not need signatures. But as said using signatures is fine too, especially with the upcoming updater. That's said if we could do things more sanely, we should do it.

    点赞 评论 复制链接分享
  • weixin_39725756 weixin_39725756 5月前

    I believe ClientDisconnect (and amxx's client_disconnect) was originally designed to not being called if player is not spawned yet. This behaviour has existed for over 10 years, thousands of plugins were written based on that behaviour and, to be honest, I'm pretty surprised that now you are going to break it. Are you sure that client_disconnect code of existing plugins will work correctly with not-yet-spawned players?

    No, I meant g_engfuncs. (but Steam API is also a great idea). My point is: 1) Changes that break backward compatibility should be reviewed more carefully 2) Filtering valid players using GETPLAYERUSERID() looks reasonable

    点赞 评论 复制链接分享
  • weixin_39675728 weixin_39675728 5月前

    I quite honestly believe this to be a bug and not a design. It makes no sense that a post hook on ClientConnect would not be paired with a ClientDisconnect call. I would find it very surprising if just a single plugin relied on the fact that there has to be a client spawn before a client can disconnect, while I would argue that a large majority assumed that client_connect and client_disconnect are paired. As you can clearly see: Even AMXX itself assumed that to be the case.

    You have a point about bcompat, and the change might actually be unsafe, yes. But this PR may then also be unsafe in its current form as it would introduce new error paths. Tricky.

    We clearly need to think more about this. I don't like applying the quick band-aid (especially on something that has apparently been "broken" for a decade) as long as we don't understand all our options.

    点赞 评论 复制链接分享
  • weixin_39725756 weixin_39725756 5月前

    We all forgot that engine API is originally designed to be used by gamedll. Since gamedll contains only logic/rules of the game, it doesn't care about players who are not in the game yet. From that perspective pairing ClientDisconnect with ClientPutInServer makes sense. Also, it's very unlikely that both Quake1 and Half-Life developers didn't discover this flaw in the engine API.

    I think you're right about the majority's assumptions about paired client_connect and client_disconnect. The problem is that developers' assumptions don't make their code safe.

    And yes, this PR may also break something. That's why we should ask to create an issue with detailed description of the problem, examples, etc. It's bad practice to provide a fix to something that others are not aware of.

    点赞 评论 复制链接分享
  • weixin_39675728 weixin_39675728 5月前

    That is my point:

    From that perspective pairing ClientDisconnect with ClientPutInServer makes sense

    Makes sense - yes, but even that is techincally not the case. By the time client_putinserver is called client->spawned is not yet true.

    Guessing from some of the stuff I've seen in reHLDS concerning this I think the engine behavior is itself a result of a band-aid fix that is now legacy. Precisely the reason I don't want to rush this PR through the door, even though it might be working.

    点赞 评论 复制链接分享
  • weixin_39668408 weixin_39668408 5月前

    Please explain your motivations for this.

    点赞 评论 复制链接分享
  • weixin_39583623 weixin_39583623 5月前

    I added (GETPLAYERUSERID(pPlayer->pEdict) > 0) check, because initialized flag doesn't work properly. initialized flag is set on ClientConnect and unset on ClientDisconnect, but if connecting (but not yet connected) player disconnects then ClientDisconnect is not called and initialized flag is not resetted.

    点赞 评论 复制链接分享
  • weixin_39668408 weixin_39668408 5月前

    As far I know, if a player is not disconnected from engine perspective, player is still valid until timeout happens or maybe a new player takes the slot (but disconnection should happen for the previous player before).

    点赞 评论 复制链接分享
  • weixin_39583623 weixin_39583623 5月前

    When SV_DropClient function disconnect connecting player it makes him invalid, but doesn't call ClientDisconnect for gamedll. ClientDisconnect not called for non-spawned players (for players that have not reached ClientPutInServer)

     C++
            if (cl->edict && cl->spawned)
                gEntityInterface.pfnClientDisconnect(cl->edict);
    

    But it still makes them invalid:

     C++
        cl->active = FALSE;
        cl->connected = FALSE;
        cl->hasusrmsgs = FALSE;
        cl->fakeclient = FALSE;
        cl->spawned = FALSE;
        cl->fully_connected = FALSE;
        cl->name[0] = 0;
        cl->connection_started = realtime;
        cl->proxy = FALSE;
        COM_ClearCustomizationList(&cl->customdata, FALSE);
        cl->edict = NULL;
        Q_memset(cl->userinfo, 0, sizeof(cl->userinfo));
        Q_memset(cl->physinfo, 0, sizeof(cl->physinfo));
    

    You can see source code of SV_DropClient function here: https://github.com/dreamstalker/rehlds/blob/fe10787125b02648f6dc9c05a642cc36429a0180/rehlds/engine/host.cpp#L459

    点赞 评论 复制链接分享
  • weixin_39583623 weixin_39583623 5月前

    Hm, but (GETPLAYERUSERID(pPlayer->pEdict) > 0) check can give true result when player is between SV_ConnectClient and gamedll ClientConnect. So I think there need more correct check.

    点赞 评论 复制链接分享
  • weixin_39668408 weixin_39668408 5月前

    I have discussed this with Nextra and we figured out that it would still be useful that client_disconnect to be triggered in such situation, because it makes sense from Amxx API point of view. The easiest and sane way to deal with this Amxx forward, would be likely just to hook directly Steam_NotifyClientDisconnect instead, .

    点赞 评论 复制链接分享
  • weixin_39583623 weixin_39583623 5月前

    And how to hook this function?

    点赞 评论 复制链接分享

相关推荐