Skip to content

Commit

Permalink
Fix invalid client index in Basebans menu
Browse files Browse the repository at this point in the history 
Fix alliedmodders#1768

The `sm_admin`-triggered Menu flow for banning players is caching client
indices inside the basebans.sp `PlayerInfo` struct, and can then
incorrectly use them in the menu callback without checking for the
related client's UserId validity. This leads to bug alliedmodders#1768 occurring when
the ban target player disconnects from the server before the banning
admin could complete the banmenu UI flow.

Since the related menu callbacks can't rely on the cached client index,
I have removed the basebans.sp `PlayerInfo.banTarget` member entirely,
in favor of the `PlayerInfo.banTarget`, and instead call
`GetClientOfUserId(...)` to get & validate the target client index where
necessary. The `PrepareBan(...)` function has been refactored to take a
ban target UserId (instead of the target client index) accordingly.
  • Loading branch information
@Rainyan
Rainyan committed May 11, 2023
1 parent 97f2fc9 commit bf72128
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 31 deletions.
3 changes: 1 addition & 2 deletions plugins/basebans.sp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public Plugin myinfo =
TopMenu hTopMenu;

enum struct PlayerInfo {
int banTarget;
int banTargetUserId;
int banTime;
int isWaitingForChatReason;
Expand Down Expand Up @@ -387,7 +386,7 @@ public Action OnClientSayCommand(int client, const char[] command, const char[]
if(playerinfo[client].isWaitingForChatReason)
{
playerinfo[client].isWaitingForChatReason = false;
PrepareBan(client, playerinfo[client].banTarget, playerinfo[client].banTime, sArgs);
PrepareBan(client, playerinfo[client].banTargetUserId, playerinfo[client].banTime, sArgs);
return Plugin_Stop;
}

Expand Down
75 changes: 46 additions & 29 deletions plugins/basebans/ban.sp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,18 @@
* Version: $Id$
*/

void PrepareBan(int client, int target, int time, const char[] reason)
void PrepareBan(int adminClient, int banTargetUserId, int time, const char[] reason)
{
int originalTarget = GetClientOfUserId(playerinfo[client].banTargetUserId);

if (originalTarget != target)
if (adminClient == 0)
{
if (client == 0)
{
PrintToServer("[SM] %t", "Player no longer available");
}
else
{
PrintToChat(client, "[SM] %t", "Player no longer available");
}
PrintToServer("[SM] %t", "Player no longer available");
return;
}

int target = GetClientOfUserId(banTargetUserId);
if (target == 0)
{
PrintToChat(adminClient, "[SM] %t", "Player no longer available");
return;
}

Expand All @@ -56,34 +53,34 @@ void PrepareBan(int client, int target, int time, const char[] reason)
{
if (reason[0] == '\0')
{
ShowActivity(client, "%t", "Permabanned player", name);
ShowActivity(adminClient, "%t", "Permabanned player", name);
}
else
{
ShowActivity(client, "%t", "Permabanned player reason", name, reason);
ShowActivity(adminClient, "%t", "Permabanned player reason", name, reason);
}
}
else
{
if (reason[0] == '\0')
{
ShowActivity(client, "%t", "Banned player", name, time);
ShowActivity(adminClient, "%t", "Banned player", name, time);
}
else
{
ShowActivity(client, "%t", "Banned player reason", name, time, reason);
ShowActivity(adminClient, "%t", "Banned player reason", name, time, reason);
}
}

LogAction(client, target, "\"%L\" banned \"%L\" (minutes \"%d\") (reason \"%s\")", client, target, time, reason);
LogAction(adminClient, target, "\"%L\" banned \"%L\" (minutes \"%d\") (reason \"%s\")", adminClient, target, time, reason);

if (reason[0] == '\0')
{
BanClient(target, time, BANFLAG_AUTO, "Banned", "Banned", "sm_ban", client);
BanClient(target, time, BANFLAG_AUTO, "Banned", "Banned", "sm_ban", adminClient);
}
else
{
BanClient(target, time, BANFLAG_AUTO, reason, reason, "sm_ban", client);
BanClient(target, time, BANFLAG_AUTO, reason, reason, "sm_ban", adminClient);
}
}

Expand All @@ -103,10 +100,17 @@ void DisplayBanTargetMenu(int client)

void DisplayBanTimeMenu(int client)
{
int target = GetClientOfUserId(playerinfo[client].banTargetUserId);
if (target == 0)
{
PrintToChat(client, "[SM] %t", "Player no longer available");
return;
}

Menu menu = new Menu(MenuHandler_BanTimeList);

char title[100];
Format(title, sizeof(title), "%T: %N", "Ban player", client, playerinfo[client].banTarget);
Format(title, sizeof(title), "%T: %N", "Ban player", client, target);
menu.SetTitle(title);
menu.ExitBackButton = true;

Expand All @@ -123,10 +127,17 @@ void DisplayBanTimeMenu(int client)

void DisplayBanReasonMenu(int client)
{
int target = GetClientOfUserId(playerinfo[client].banTargetUserId);
if (target == 0)
{
PrintToChat(client, "[SM] %t", "Player no longer available");
return;
}

Menu menu = new Menu(MenuHandler_BanReasonList);

char title[100];
Format(title, sizeof(title), "%T: %N", "Ban reason", client, playerinfo[client].banTarget);
Format(title, sizeof(title), "%T: %N", "Ban reason", client, target);
menu.SetTitle(title);
menu.ExitBackButton = true;

Expand Down Expand Up @@ -201,8 +212,8 @@ public int MenuHandler_BanReasonList(Menu menu, MenuAction action, int param1, i
char info[64];

menu.GetItem(param2, info, sizeof(info));
PrepareBan(param1, playerinfo[param1].banTarget, playerinfo[param1].banTime, info);

PrepareBan(param1, playerinfo[param1].banTargetUserId, playerinfo[param1].banTime, info);
}
}

Expand Down Expand Up @@ -240,7 +251,6 @@ public int MenuHandler_BanPlayerList(Menu menu, MenuAction action, int param1, i
}
else
{
playerinfo[param1].banTarget = target;
playerinfo[param1].banTargetUserId = userid;
DisplayBanTimeMenu(param1);
}
Expand All @@ -264,12 +274,19 @@ public int MenuHandler_BanTimeList(Menu menu, MenuAction action, int param1, int
}
else if (action == MenuAction_Select)
{
char info[32];
if (GetClientOfUserId(playerinfo[param1].banTargetUserId) == 0)
{
PrintToChat(param1, "[SM] %t", "Player no longer available");
}
else
{
char info[32];

menu.GetItem(param2, info, sizeof(info));
playerinfo[param1].banTime = StringToInt(info);
menu.GetItem(param2, info, sizeof(info));
playerinfo[param1].banTime = StringToInt(info);

DisplayBanReasonMenu(param1);
DisplayBanReasonMenu(param1);
}
}

return 0;
Expand Down Expand Up @@ -318,7 +335,7 @@ public Action Command_Ban(int client, int args)
playerinfo[client].banTargetUserId = GetClientUserId(target);

int time = StringToInt(s_time);
PrepareBan(client, target, time, Arguments[len]);
PrepareBan(client, playerinfo[client].banTargetUserId, time, Arguments[len]);

return Plugin_Handled;
}

0 comments on commit bf72128

Please sign in to comment.