NormalSHook erroring on non-IsInGame clients

[rtldg]

IEngineSound::EmitSound seems to be called with clients in the filter that might not be IsInGame() which can throw an error when using a NormalSHook and returning Plugin_Changed since all the clients will will be checked if IsInGame().

Using CBroadcastRecipientFilter would add all players than can be grabbed with UTIL_PlayerByIndex and is probably cause of EmitSound having clients that are not IsInGame().

This PR has been changed to remove clients from the players array that are not IsInGame(). Also a check for the callback-provided size variable has been added

previously

Removing the `IsInGame()` check should be fine since `EmitSound` eventually uses `CGameClient::IsActive()` to only send to active clients.

EmitSound -> EmitSoundInternal -> SV_StartSound ->sv.BroadcastSound

CGameClient *pClient = Client( index - 1 );

// client must be fully connect to hear sounds
if ( !pClient->IsActive() )
{
	continue;
}

pClient->SendSound( sound, filter.IsReliable() );

CGameClient -> CBaseClient
virtual bool IsActive( void ) const { return m_nSignonState == SIGNONSTATE_FULL; };

[Fyren]

Removing the IsInGame() check should be fine since EmitSound eventually uses CGameClient::IsActive() to only send to active clients.

Did you check if this is true for all engine versions SM supports? Did you actually test it on any?

[rtldg]

Removing the IsInGame() check should be fine since EmitSound eventually uses CGameClient::IsActive() to only send to active clients.

Did you check if this is true for all engine versions SM supports? Did you actually test it on any?

It looks like it has always checked if the client is active in both older and newer engine versions.
Also it was a CSGO server that was throwing these sourcemod errors about players not being ingame after returning Plugin_Changed in NormalSHook.

[KyleSanderson]

I really don't remember why this was added, I think I hit a filter or something that crashed on a mid-connecting client. That's the reason this one hasn't been merged yet, I know it's painful but the only path to merge this one would be to test every game presently.

[rtldg]

Alternatively, how about removing the client from the players array if not IsInGame()?

[KyleSanderson]

Alternatively, how about removing the client from the players array if not IsInGame()?

I think that's a fine idea

[rtldg]

I force pushed a commit to remove clients from the players array if not IsInGame and also to check if the size variable is valid.

edit:
also instead of

memmove(&players[i], &players[i+1], (size-i-1) * sizeof(int));
--i;
--size;

this would be funny

memmove(&players[i], &players[i+1], (size-- - --i) * sizeof(int));

[asherkin]

Thanks, I think this looks sane to me, but I'd like a 2nd ack for merge.