From 6e0f4ee6655768726fd912c742fb67e5b70b2a32 Mon Sep 17 00:00:00 2001 From: Scott Ehlert Date: Tue, 8 Jul 2008 08:55:50 +0000 Subject: [PATCH] - Fixed amb1802: Crash when client was disconnected as a result of false being returned in OnClientConnect and a function that operated on this client was used. A client's connection state was not reset when this happened. - Removed IForwardFilter due to overall horribleness (should be safe since no one seems to use it). Perhaps it might be back one day? - Added ET_LowEvent forward exec type which is exactly the same as ET_Event, except that it returns the lowest value rather than the highest --HG-- branch : sourcemod-1.0.x extra : convert_revision : svn%3A39bc706e-5318-0410-9160-8a85361fbb7c/branches/sourcemod-1.0.x%402384 --- core/PlayerManager.cpp | 131 ++++++++++++++++++++---------------- core/PlayerManager.h | 1 + core/systems/ForwardSys.cpp | 87 ++++++++++++++---------- core/systems/ForwardSys.h | 4 +- 4 files changed, 126 insertions(+), 97 deletions(-) diff --git a/core/PlayerManager.cpp b/core/PlayerManager.cpp index 5164211e..d6665d1e 100644 --- a/core/PlayerManager.cpp +++ b/core/PlayerManager.cpp @@ -121,7 +121,7 @@ void PlayerManager::OnSourceModAllInitialized() ParamType p1[] = {Param_Cell, Param_String, Param_Cell}; ParamType p2[] = {Param_Cell}; - m_clconnect = g_Forwards.CreateForward("OnClientConnect", ET_Event, 3, p1); + m_clconnect = g_Forwards.CreateForward("OnClientConnect", ET_LowEvent, 3, p1); m_clputinserver = g_Forwards.CreateForward("OnClientPutInServer", ET_Ignore, 1, p2); m_cldisconnect = g_Forwards.CreateForward("OnClientDisconnect", ET_Ignore, 1, p2); m_cldisconnect_post = g_Forwards.CreateForward("OnClientDisconnect_Post", ET_Ignore, 1, p2); @@ -288,7 +288,7 @@ void PlayerManager::RunAuthChecks() unsigned int removed = 0; for (unsigned int i=1; i<=m_AuthQueue[0]; i++) { - pPlayer = GetPlayerByIndex(m_AuthQueue[i]); + pPlayer = &m_Players[m_AuthQueue[i]]; authstr = engine->GetPlayerNetworkIDString(pPlayer->m_pEdict); if (authstr && authstr[0] != '\0' && (strcmp(authstr, "STEAM_ID_PENDING") != 0)) @@ -362,6 +362,7 @@ void PlayerManager::RunAuthChecks() bool PlayerManager::OnClientConnect(edict_t *pEntity, const char *pszName, const char *pszAddress, char *reject, int maxrejectlen) { int client = engine->IndexOfEdict(pEntity); + CPlayer *pPlayer = &m_Players[client]; List::iterator iter; IClientListener *pListener = NULL; @@ -376,26 +377,29 @@ bool PlayerManager::OnClientConnect(edict_t *pEntity, const char *pszName, const cell_t res = 1; - m_Players[client].Initialize(pszName, pszAddress, pEntity); + pPlayer->Initialize(pszName, pszAddress, pEntity); m_clconnect->PushCell(client); m_clconnect->PushStringEx(reject, maxrejectlen, SM_PARAM_STRING_UTF8, SM_PARAM_COPYBACK); m_clconnect->PushCell(maxrejectlen); - m_clconnect->Execute(&res, NULL); + m_clconnect->Execute(&res); if (res) { - if (!m_Players[client].IsAuthorized()) + if (!pPlayer->IsAuthorized()) { m_AuthQueue[++m_AuthQueue[0]] = client; } + + m_UserIdLookUp[engine->GetPlayerUserId(pEntity)] = client; } else { - RETURN_META_VALUE(MRES_SUPERCEDE, false); + if (!pPlayer->IsFakeClient()) + { + RETURN_META_VALUE(MRES_SUPERCEDE, false); + } } - m_UserIdLookUp[engine->GetPlayerUserId(pEntity)] = client; - return true; } @@ -403,7 +407,7 @@ bool PlayerManager::OnClientConnect_Post(edict_t *pEntity, const char *pszName, { int client = engine->IndexOfEdict(pEntity); bool orig_value = META_RESULT_ORIG_RET(bool); - CPlayer *pPlayer = GetPlayerByIndex(client); + CPlayer *pPlayer = &m_Players[client]; if (orig_value) { @@ -418,13 +422,17 @@ bool PlayerManager::OnClientConnect_Post(edict_t *pEntity, const char *pszName, break; } } - } - if (!pPlayer->IsFakeClient() - && m_bIsListenServer - && strncmp(pszAddress, "127.0.0.1", 9) == 0) + if (!pPlayer->IsFakeClient() + && m_bIsListenServer + && strncmp(pszAddress, "127.0.0.1", 9) == 0) + { + m_ListenClient = client; + } + } + else { - m_ListenClient = client; + InvalidatePlayer(pPlayer); } return true; @@ -434,8 +442,8 @@ void PlayerManager::OnClientPutInServer(edict_t *pEntity, const char *playername { cell_t res; int client = engine->IndexOfEdict(pEntity); + CPlayer *pPlayer = &m_Players[client]; - CPlayer *pPlayer = GetPlayerByIndex(client); /* If they're not connected, they're a bot */ if (!pPlayer->IsConnected()) { @@ -494,7 +502,7 @@ void PlayerManager::OnClientPutInServer(edict_t *pEntity, const char *playername } } - m_Players[client].Connect(); + pPlayer->Connect(); m_PlayerCount++; List::iterator iter; @@ -508,9 +516,9 @@ void PlayerManager::OnClientPutInServer(edict_t *pEntity, const char *playername m_clputinserver->PushCell(client); m_clputinserver->Execute(&res, NULL); - if (m_Players[client].IsAuthorized()) + if (pPlayer->IsAuthorized()) { - m_Players[client].DoPostConnectAuthorization(); + pPlayer->DoPostConnectAuthorization(); } } @@ -531,8 +539,9 @@ void PlayerManager::OnClientDisconnect(edict_t *pEntity) { cell_t res; int client = engine->IndexOfEdict(pEntity); + CPlayer *pPlayer = &m_Players[client]; - if (m_Players[client].IsConnected()) + if (pPlayer->IsConnected()) { m_cldisconnect->PushCell(client); m_cldisconnect->Execute(&res, NULL); @@ -543,7 +552,7 @@ void PlayerManager::OnClientDisconnect(edict_t *pEntity) return; } - if (m_Players[client].WasCountedAsInGame()) + if (pPlayer->WasCountedAsInGame()) { m_PlayerCount--; } @@ -556,29 +565,7 @@ void PlayerManager::OnClientDisconnect(edict_t *pEntity) pListener->OnClientDisconnecting(client); } - /** - * Remove client from auth queue if necessary - */ - if (!m_Players[client].IsAuthorized()) - { - for (unsigned int i=1; i<=m_AuthQueue[0]; i++) - { - if (m_AuthQueue[i] == (unsigned)client) - { - /* Move everything ahead of us back by one */ - for (unsigned int j=i+1; j<=m_AuthQueue[0]; j++) - { - m_AuthQueue[j-1] = m_AuthQueue[j]; - } - /* Remove us and break */ - m_AuthQueue[0]--; - break; - } - } - } - - m_Players[client].Disconnect(); - m_UserIdLookUp[engine->GetPlayerUserId(pEntity)] = 0; + InvalidatePlayer(pPlayer); if (m_ListenClient == client) { @@ -613,9 +600,9 @@ void PlayerManager::OnClientCommand(edict_t *pEntity) #endif int client = engine->IndexOfEdict(pEntity); cell_t res = Pl_Continue; + CPlayer *pPlayer = &m_Players[client]; - CPlayer *pPlayer = GetPlayerByIndex(client); - if (!pPlayer || !pPlayer->IsConnected()) + if (!pPlayer->IsConnected()) { return; } @@ -667,8 +654,9 @@ void PlayerManager::OnClientSettingsChanged(edict_t *pEntity) { cell_t res; int client = engine->IndexOfEdict(pEntity); + CPlayer *pPlayer = &m_Players[client]; - if (!m_Players[client].IsConnected()) + if (!pPlayer->IsConnected()) { return; } @@ -676,42 +664,42 @@ void PlayerManager::OnClientSettingsChanged(edict_t *pEntity) m_clinfochanged->PushCell(engine->IndexOfEdict(pEntity)); m_clinfochanged->Execute(&res, NULL); - IPlayerInfo *info = m_Players[client].GetPlayerInfo(); + IPlayerInfo *info = pPlayer->GetPlayerInfo(); const char *new_name = info ? info->GetName() : engine->GetClientConVarValue(client, "name"); - const char *old_name = m_Players[client].m_Name.c_str(); + const char *old_name = pPlayer->m_Name.c_str(); if (strcmp(old_name, new_name) != 0) { AdminId id = g_Admins.FindAdminByIdentity("name", new_name); - if (id != INVALID_ADMIN_ID && m_Players[client].GetAdminId() != id) + if (id != INVALID_ADMIN_ID && pPlayer->GetAdminId() != id) { - if (!CheckSetAdminName(client, &m_Players[client], id)) + if (!CheckSetAdminName(client, pPlayer, id)) { - m_Players[client].Kick("Your name is reserved by SourceMod; set your password to use it."); + pPlayer->Kick("Your name is reserved by SourceMod; set your password to use it."); RETURN_META(MRES_IGNORED); } } else if ((id = g_Admins.FindAdminByIdentity("name", old_name)) != INVALID_ADMIN_ID) { - if (id == m_Players[client].GetAdminId()) + if (id == pPlayer->GetAdminId()) { /* This player is changing their name; force them to drop admin privileges! */ - m_Players[client].SetAdminId(INVALID_ADMIN_ID, false); + pPlayer->SetAdminId(INVALID_ADMIN_ID, false); } } - m_Players[client].SetName(new_name); + pPlayer->SetName(new_name); } if (m_PassInfoVar.size() > 0) { /* Try for a password change */ - const char *old_pass = m_Players[client].m_LastPassword.c_str(); + const char *old_pass = pPlayer->m_LastPassword.c_str(); const char *new_pass = engine->GetClientConVarValue(client, m_PassInfoVar.c_str()); if (strcmp(old_pass, new_pass) != 0) { - m_Players[client].m_LastPassword.assign(new_pass); - if (m_Players[client].IsInGame() && m_Players[client].IsAuthorized()) + pPlayer->m_LastPassword.assign(new_pass); + if (pPlayer->IsInGame() && pPlayer->IsAuthorized()) { /* If there is already an admin id assigned, this will just bail out. */ - m_Players[client].DoBasicAdminChecks(); + pPlayer->DoBasicAdminChecks(); } } } @@ -862,6 +850,33 @@ void PlayerManager::UnregisterCommandTargetProcessor(ICommandTargetProcessor *pH target_processors.remove(pHandler); } +void PlayerManager::InvalidatePlayer(CPlayer *pPlayer) +{ + /** + * Remove client from auth queue if necessary + */ + if (!pPlayer->IsAuthorized()) + { + for (unsigned int i=1; i<=m_AuthQueue[0]; i++) + { + if (m_AuthQueue[i] == (unsigned)pPlayer->m_iIndex) + { + /* Move everything ahead of us back by one */ + for (unsigned int j=i+1; j<=m_AuthQueue[0]; j++) + { + m_AuthQueue[j-1] = m_AuthQueue[j]; + } + /* Remove us and break */ + m_AuthQueue[0]--; + break; + } + } + } + + m_UserIdLookUp[engine->GetPlayerUserId(pPlayer->m_pEdict)] = 0; + pPlayer->Disconnect(); +} + int PlayerManager::InternalFilterCommandTarget(CPlayer *pAdmin, CPlayer *pTarget, int flags) { if ((flags & COMMAND_FILTER_CONNECTED) == COMMAND_FILTER_CONNECTED diff --git a/core/PlayerManager.h b/core/PlayerManager.h index 9d235c49..da67f206 100644 --- a/core/PlayerManager.h +++ b/core/PlayerManager.h @@ -173,6 +173,7 @@ public: unsigned int SetReplyTo(unsigned int reply); private: void OnServerActivate(edict_t *pEdictList, int edictCount, int clientMax); + void InvalidatePlayer(CPlayer *pPlayer); private: List m_hooks; IForward *m_clconnect; diff --git a/core/systems/ForwardSys.cpp b/core/systems/ForwardSys.cpp index cf42fa79..654f158b 100644 --- a/core/systems/ForwardSys.cpp +++ b/core/systems/ForwardSys.cpp @@ -2,7 +2,7 @@ * vim: set ts=4 : * ============================================================================= * SourceMod - * Copyright (C) 2004-2007 AlliedModders LLC. All rights reserved. + * Copyright (C) 2004-2008 AlliedModders LLC. All rights reserved. * ============================================================================= * * This program is free software; you can redistribute it and/or modify it under @@ -283,15 +283,11 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) return err; } - if (filter) - { - filter->OnExecuteBegin(); - } - FuncIter iter = m_functions.begin(); IPluginFunction *func; cell_t cur_result = 0; cell_t high_result = 0; + cell_t low_result = 0; int err; unsigned int failed=0, success=0; unsigned int num_params = m_curparam; @@ -311,12 +307,16 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) { int err = SP_ERROR_PARAM; param = &temp_info[i]; + if (i >= m_numparams || m_types[i] == Param_Any) { type = param->pushedas; - } else { + } + else + { type = m_types[i]; } + if ((i >= m_numparams) || (type & SP_PARAMFLAG_BYREF)) { /* If we're byref or we're vararg, we always push everything by ref. @@ -325,26 +325,30 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) if (type == Param_String) { err = func->PushStringEx((char *)param->byref.orig_addr, param->byref.cells, param->byref.sz_flags, param->byref.flags); - } else if (type == Param_Float || type == Param_Cell) { + } + else if (type == Param_Float || type == Param_Cell) + { err = func->PushCellByRef(¶m->val); - } else { + } + else + { err = func->PushArray(param->byref.orig_addr, param->byref.cells, param->byref.flags); assert(type == Param_Array || type == Param_FloatByRef || type == Param_CellByRef); } - } else { + } + else + { /* If we're not byref or not vararg, our job is a bit easier. */ assert(type == Param_Cell || type == Param_Float); err = func->PushCell(param->val); } + if (err != SP_ERROR_NONE) { - if (!filter || !filter->OnErrorReport(this, func, err)) - { - g_DbgReporter.GenerateError(func->GetParentContext(), - func->GetFunctionID(), - err, - "Failed to push parameter while executing forward"); - } + g_DbgReporter.GenerateError(func->GetParentContext(), + func->GetFunctionID(), + err, + "Failed to push parameter while executing forward"); continue; } } @@ -352,12 +356,10 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) /* Call the function and deal with the return value. */ if ((err=func->Execute(&cur_result)) != SP_ERROR_NONE) { - if (filter) - { - filter->OnErrorReport(this, func, err); - } failed++; - } else { + } + else + { success++; switch (m_ExecType) { @@ -381,14 +383,11 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) } break; } - case ET_Custom: + case ET_LowEvent: { - if (filter) + if (cur_result < low_result) { - if (filter->OnFunctionReturn(this, func, &cur_result) == Pl_Stop) - { - goto done; - } + low_result = cur_result; } break; } @@ -401,25 +400,39 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter) } done: + if (success) { - if (m_ExecType == ET_Event || m_ExecType == ET_Hook) + switch (m_ExecType) { - cur_result = high_result; - } else if (m_ExecType == ET_Ignore) { - cur_result = 0; + case ET_Ignore: + { + cur_result = 0; + break; + } + case ET_Event: + case ET_Hook: + { + cur_result = high_result; + break; + } + case ET_LowEvent: + { + cur_result = low_result; + break; + } + default: + { + break; + } } + if (result) { *result = cur_result; } } - if (filter) - { - filter->OnExecuteEnd(&cur_result, success, failed); - } - return SP_ERROR_NONE; } diff --git a/core/systems/ForwardSys.h b/core/systems/ForwardSys.h index 68b7d9fa..1d6303a4 100644 --- a/core/systems/ForwardSys.h +++ b/core/systems/ForwardSys.h @@ -2,7 +2,7 @@ * vim: set ts=4 : * ============================================================================= * SourceMod - * Copyright (C) 2004-2007 AlliedModders LLC. All rights reserved. + * Copyright (C) 2004-2008 AlliedModders LLC. All rights reserved. * ============================================================================= * * This program is free software; you can redistribute it and/or modify it under @@ -43,7 +43,7 @@ using namespace SourceHook; typedef List::iterator FuncIter; -//:TODO: a global name max define for sourcepawn, should mirror compiler's sNAMEMAX +/* :TODO: a global name max define for sourcepawn, should mirror compiler's sNAMEMAX */ #define FORWARDS_NAME_MAX 64 struct ByrefInfo