0001510: Server sends useless inconsistent hp/armor values (100/100) to the other clients when a player is damaged

diff -r ef48d0b91baa docs/zandronum-history.txt --- a/docs/zandronum-history.txt Tue Sep 17 18:50:31 2013 +0200 +++ b/docs/zandronum-history.txt Tue Sep 17 22:42:34 2013 +0200 @@ -23,6 +23,7 @@ --- + - Added new dmflag sv_sharekeys. When enabled, any keys picked are shared between players. Also, players joining the game get the keys others have as well. [Dusk] +- - Fixed: when a player is damaged, don't send useless inconsistent values of HP and Armor (100/100) to the clients if they're not allowed to see the real HP/Armor. [Edward-san] 1.2 diff -r ef48d0b91baa src/sv_commands.cpp --- a/src/sv_commands.cpp Tue Sep 17 18:50:31 2013 +0200 +++ b/src/sv_commands.cpp Tue Sep 17 22:42:34 2013 +0200 @@ -632,25 +632,18 @@ if ( SERVER_IsValidClient( ulIdx ) == false ) continue; - SERVER_CheckClientBuffer( ulIdx, 8, true ); + // Send the updated health and armor of the player who's being damaged to this player + // only if this player is allowed to know. + if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer ) == false ) + continue; + + SERVER_CheckClientBuffer( ulIdx, 8, true ); + NETWORK_WriteHeader( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, SVC_DAMAGEPLAYER ); NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulPlayer ); - - // Only send the player who's being damaged to this player if this player is - // allowed to know what his health is. Otherwise, just tell them it's 100/100 - // (WHICH IS A LIE!!!!!!). - if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer )) - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); - } - else - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, -1 ); - } + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); } }

diff -r 2d4c0402fde1 docs/zandronum-history.txt --- a/docs/zandronum-history.txt Sun Jan 12 12:13:41 2014 +0100 +++ b/docs/zandronum-history.txt Sun Apr 13 21:36:17 2014 +0200 @@ -35,6 +35,7 @@ - - Fixed: A malformed packet could make the server kick the clients. Thanks to Konar6 for supplying a proof-of-concept exploit that allowed to debug the issue. [Torr Samaho] - - Fixed: Tampered clients could insert ASCII control characters into chat messages (e.g. line breaks) (ported from Konar6's kpatch). [Torr Samaho] - - Fixed: Strife's crossbow did not appear to use ammo on clients online. [Dusk] +- - Fixed: when a player is damaged, don't send useless inconsistent values of HP and Armor (100/100) to the clients if they're not allowed to see the real HP/Armor. [Edward-san] ! - Changed huffman code from Hexen II's to that provided by Vortex Cortex. [Blzut3, Vortex Cortex] ! - Changed the Offline Skirmish frag slider bar to go in intervals of 1 instead of 5. [Water] diff -r 2d4c0402fde1 src/sv_commands.cpp --- a/src/sv_commands.cpp Sun Jan 12 12:13:41 2014 +0100 +++ b/src/sv_commands.cpp Sun Apr 13 21:36:17 2014 +0200 @@ -632,25 +632,21 @@ if ( SERVER_IsValidClient( ulIdx ) == false ) continue; + // [EP] Send the updated health and armor of the player who's being damaged to this client + // only if this client is allowed to know. + if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer ) == false ) { + // [EP] Pain is not available when there are no updates to health + SERVERCOMMANDS_SetThingState( players[ulPlayer].mo, STATE_PAIN, ulIdx, SVCF_ONLYTHISCLIENT ); + continue; + } + SERVER_CheckClientBuffer( ulIdx, 8, true ); + NETWORK_WriteHeader( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, SVC_DAMAGEPLAYER ); NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulPlayer ); - - // Only send the player who's being damaged to this player if this player is - // allowed to know what his health is. Otherwise, just tell them it's 100/100 - // (WHICH IS A LIE!!!!!!). - if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer )) - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); - } - else - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, -1 ); - } + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); } } @@ -2129,23 +2125,18 @@ //***************************************************************************** // -void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState ) +void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState, ULONG ulPlayerExtra, ULONG ulFlags ) { ULONG ulIdx; if ( !EnsureActorHasNetID (pActor) ) return; - for ( ulIdx = 0; ulIdx < MAXPLAYERS; ulIdx++ ) - { - if ( SERVER_IsValidClient( ulIdx ) == false ) - continue; - - SERVER_CheckClientBuffer( ulIdx, 4, true ); - NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, SVC_SETTHINGSTATE ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, pActor->lNetID ); - NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulState ); - } + NetCommand command( SVC_SETTHINGSTATE ); + command.addShort( pActor->lNetID ); + command.addByte( ulState ); + command.sendCommandToClients( ulPlayerExtra, ulFlags ); + } //***************************************************************************** diff -r 2d4c0402fde1 src/sv_commands.h --- a/src/sv_commands.h Sun Jan 12 12:13:41 2014 +0100 +++ b/src/sv_commands.h Sun Apr 13 21:36:17 2014 +0200 @@ -135,7 +135,7 @@ void SERVERCOMMANDS_MoveThingExact( AActor *pActor, ULONG ulBits, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 ); void SERVERCOMMANDS_DamageThing( AActor *pActor ); void SERVERCOMMANDS_KillThing( AActor *pActor, AActor *pSource, AActor *pInflictor ); -void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState ); +void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 ); void SERVERCOMMANDS_SetThingTarget( AActor *pActor ); void SERVERCOMMANDS_DestroyThing( AActor *pActor ); void SERVERCOMMANDS_SetThingAngle( AActor *pActor, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 );

diff -r 2d4c0402fde1 docs/zandronum-history.txt --- a/docs/zandronum-history.txt Sun Jan 12 12:13:41 2014 +0100 +++ b/docs/zandronum-history.txt Sun Apr 13 23:36:17 2014 +0200 @@ -35,6 +35,7 @@ - - Fixed: A malformed packet could make the server kick the clients. Thanks to Konar6 for supplying a proof-of-concept exploit that allowed to debug the issue. [Torr Samaho] - - Fixed: Tampered clients could insert ASCII control characters into chat messages (e.g. line breaks) (ported from Konar6's kpatch). [Torr Samaho] - - Fixed: Strife's crossbow did not appear to use ammo on clients online. [Dusk] +- - Fixed: when a player is damaged, don't send useless inconsistent values of HP and Armor (100/100) to the clients if they're not allowed to see the real HP/Armor. [Edward-san] ! - Changed huffman code from Hexen II's to that provided by Vortex Cortex. [Blzut3, Vortex Cortex] ! - Changed the Offline Skirmish frag slider bar to go in intervals of 1 instead of 5. [Water] diff -r 2d4c0402fde1 src/sv_commands.cpp --- a/src/sv_commands.cpp Sun Jan 12 12:13:41 2014 +0100 +++ b/src/sv_commands.cpp Sun Apr 13 23:36:17 2014 +0200 @@ -632,25 +632,20 @@ if ( SERVER_IsValidClient( ulIdx ) == false ) continue; + // [EP] Send the updated health and armor of the player who's being damaged to this client + // only if this client is allowed to know (still, don't forget the pain state!). + if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer ) == false ) { + SERVERCOMMANDS_SetThingState( players[ulPlayer].mo, STATE_PAIN, ulIdx, SVCF_ONLYTHISCLIENT ); + continue; + } + SERVER_CheckClientBuffer( ulIdx, 8, true ); + NETWORK_WriteHeader( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, SVC_DAMAGEPLAYER ); NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulPlayer ); - - // Only send the player who's being damaged to this player if this player is - // allowed to know what his health is. Otherwise, just tell them it's 100/100 - // (WHICH IS A LIE!!!!!!). - if ( SERVER_IsPlayerAllowedToKnowHealth( ulIdx, ulPlayer )) - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); - } - else - { - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, 100 ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, -1 ); - } + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].health ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulArmorPoints ); + NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, players[ulPlayer].attacker ? players[ulPlayer].attacker->lNetID : -1 ); } } @@ -2129,23 +2124,18 @@ //***************************************************************************** // -void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState ) +void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState, ULONG ulPlayerExtra, ULONG ulFlags ) { ULONG ulIdx; if ( !EnsureActorHasNetID (pActor) ) return; - for ( ulIdx = 0; ulIdx < MAXPLAYERS; ulIdx++ ) - { - if ( SERVER_IsValidClient( ulIdx ) == false ) - continue; - - SERVER_CheckClientBuffer( ulIdx, 4, true ); - NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, SVC_SETTHINGSTATE ); - NETWORK_WriteShort( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, pActor->lNetID ); - NETWORK_WriteByte( &SERVER_GetClient( ulIdx )->PacketBuffer.ByteStream, ulState ); - } + NetCommand command( SVC_SETTHINGSTATE ); + command.addShort( pActor->lNetID ); + command.addByte( ulState ); + command.sendCommandToClients( ulPlayerExtra, ulFlags ); + } //***************************************************************************** diff -r 2d4c0402fde1 src/sv_commands.h --- a/src/sv_commands.h Sun Jan 12 12:13:41 2014 +0100 +++ b/src/sv_commands.h Sun Apr 13 23:36:17 2014 +0200 @@ -135,7 +135,7 @@ void SERVERCOMMANDS_MoveThingExact( AActor *pActor, ULONG ulBits, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 ); void SERVERCOMMANDS_DamageThing( AActor *pActor ); void SERVERCOMMANDS_KillThing( AActor *pActor, AActor *pSource, AActor *pInflictor ); -void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState ); +void SERVERCOMMANDS_SetThingState( AActor *pActor, ULONG ulState, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 ); void SERVERCOMMANDS_SetThingTarget( AActor *pActor ); void SERVERCOMMANDS_DestroyThing( AActor *pActor ); void SERVERCOMMANDS_SetThingAngle( AActor *pActor, ULONG ulPlayerExtra = MAXPLAYERS, ULONG ulFlags = 0 );

Notes
(0007211) ZzZombo (reporter) 2013-09-18 04:11 edited on: 2013-09-18 04:15	I think either the client should just show ?/? for HP/AP or SpawnHealth value/MaxArmor (zero?) value. Q: Why such system do exist in first place? Many (all known competitive TBH) games such as CS/L4D don't hide opponents' health/armor if they are observed in deathcam or by spectators.

(0007223) Qent (updater) 2013-09-18 17:38 edited on: 2013-09-18 17:39	A couple questions were raised while testing the patch last night: - When a player dies, should other clients be notified of their remaining armor? - If not, should the health be shown at maximum or zero for other clients? My view is that health should drop instantly to zero when spying on a player dying. Armor should remain at whatever the default for that player class was (usually zero, something else in some mods like ZDoomWars).

(0007226) Edward-san (developer) 2013-09-18 21:05	BTW: the test went well, with the help of Qent and 75. Apparently there are no regressions, hence this patch can be appliable imo.

(0007261) AlienOverlord (reporter) 2013-09-21 15:49	Oh, so this explains why spectators observe thick red pain screen after someone gets Soulsphere and immediately receives small damage in competitive modes. Client thinks that player's HP lowers from 200 (local prediction) to 100. This is also a bug btw

(0007940) Edward-san (developer) 2014-01-10 11:23	I just remembered I forgot to set the correct status.

(0007941) ZzZombo (reporter) 2014-01-10 12:03	But don't you find "?/?" being much better solution if we still don't want to stop hiding this information? And I still insist hiding HP and armor shouldn't be so strict (and TBH I never understood why we do it), and at least spectators could see the real state of the game.

(0007942) Konar6 (reporter) 2014-01-10 15:25	I'm wondering as well why spectators are not allowed to see players' health. I can't see how it could be efficiently abused for cheating.

(0007945) Edward-san (developer) 2014-01-10 22:56 edited on: 2014-01-10 22:57	It seems there's a misunderstanding on the bug and the fix: in every gamemode which prohibits the knowledge of the other players informations, if spawnhealth is set differently from default and one player is damaged for the first time, independently on the value of the damage, the spying clients will: 1)see the damage palette as if the player had spawnhealth-default damage 2)if custom SBarInfo HUD is used, health bar drops to 100. This happens because the server sends some bytes to all the clients with the instruction: change the damaged player health/armor to 100/100. If the gamemode doesn't allow players to see each other's status, then the server should respect this and should not send lies at all. The fix is simple: just don't send useless info at all. Nothing changes regarding the general behavior and it's intended because that's the topic of this bug ticket. Imho, if you want something about the rest, just create a new suggestion ticket with your considerations.

(0008562) Torr Samaho (administrator) 2014-04-13 13:27	With your patch damaged players are not put into the pain state anymore.

(0008565) Edward-san (developer) 2014-04-13 16:21	Does this happen also in coop? Because in deathmatch or in other games where the enemy players info is hidden, the pain state hiding is intentional.

(0008566) Torr Samaho (administrator) 2014-04-13 18:13	client_DamagePlayer always calls players[ulPlayer].mo->SetState( pPainState ); no matter which game mode. So you intentionally want to hide the pain state in DM? Can you elaborate why?

(0008567) Edward-san (developer) 2014-04-13 18:26	Ah, sorry, I thought you were speaking of the pain red screen ... I'll take a look at it.

(0008568) Edward-san (developer) 2014-04-13 19:24	This should do it.

(0008570) Edward-san (developer) 2014-04-13 21:43	Updated a new patch which changes some comments. Tested it locally with 2 clients. Pain is heard, can be visible with chasecam, no sudden changes with health and armor.

(0008907) Torr Samaho (administrator) 2014-06-08 10:12 edited on: 2014-06-08 10:23	Looks good. Added! EDIT: This needs to be tested in 1.3 and 2.0 separately since I had to adapt this change to the recent server commands rewrite in 2.0 when merging this change into 2.0.

(0009836) Arco (updater) 2014-07-04 17:36	Works correctly in both v1.3 r140703-1806 and v2.0 r140609-0928.

**Issue Community Support**
This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	ZzZombo
Opponents:	No one explicitly opposes this issue yet.

Issue Community Support

This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.

Supporters:

ZzZombo

Opponents:

No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2013-09-17 21:15	Edward-san	New Issue
2013-09-17 21:15	Edward-san	Status	new => assigned
2013-09-17 21:15	Edward-san	Assigned To	=> Edward-san
2013-09-17 21:15	Edward-san	File Added: inconsistent_hp_armor.txt
2013-09-17 21:17	Edward-san	Relationship added	related to 0000071
2013-09-18 04:11	ZzZombo	Note Added: 0007211
2013-09-18 04:14	ZzZombo	Note Edited: 0007211	View Revisions
2013-09-18 04:15	ZzZombo	Note Edited: 0007211	View Revisions
2013-09-18 17:38	Qent	Note Added: 0007223
2013-09-18 17:39	Qent	Note Edited: 0007223	View Revisions
2013-09-18 21:05	Edward-san	Note Added: 0007226
2013-09-21 15:49	AlienOverlord	Note Added: 0007261
2014-01-10 11:23	Edward-san	Note Added: 0007940
2014-01-10 11:23	Edward-san	Status	assigned => needs review
2014-01-10 12:03	ZzZombo	Note Added: 0007941
2014-01-10 15:25	Konar6	Note Added: 0007942
2014-01-10 22:56	Edward-san	Note Added: 0007945
2014-01-10 22:57	Edward-san	Note Edited: 0007945	View Revisions
2014-04-13 13:27	Torr Samaho	Note Added: 0008562
2014-04-13 16:21	Edward-san	Note Added: 0008565
2014-04-13 18:13	Torr Samaho	Note Added: 0008566
2014-04-13 18:26	Edward-san	Note Added: 0008567
2014-04-13 19:23	Edward-san	File Added: inconsistent_hp_armor_2.txt
2014-04-13 19:24	Edward-san	Note Added: 0008568
2014-04-13 19:27	Edward-san	File Deleted: inconsistent_hp_armor_2.txt
2014-04-13 19:28	Edward-san	File Added: inconsistent_hp_armor_2.txt
2014-04-13 19:36	Edward-san	File Deleted: inconsistent_hp_armor_2.txt
2014-04-13 19:36	Edward-san	File Added: inconsistent_hp_armor_2.txt
2014-04-13 21:37	Edward-san	File Added: inconsistent_hp_armor_3.txt
2014-04-13 21:43	Edward-san	Note Added: 0008570
2014-06-08 00:23	Edward-san	Note Added: 0008891
2014-06-08 00:23	Edward-san	Status	needs review => feedback
2014-06-08 00:24	Edward-san	Note Edited: 0008891	View Revisions
2014-06-08 00:24	Edward-san	Note Deleted: 0008891
2014-06-08 00:24	Edward-san	Status	feedback => needs review
2014-06-08 10:12	Torr Samaho	Note Added: 0008907
2014-06-08 10:23	Torr Samaho	Note Edited: 0008907	View Revisions
2014-06-08 10:23	Torr Samaho	Status	needs review => needs testing
2014-07-04 17:36	Arco	Note Added: 0009836
2014-07-04 17:36	Arco	Status	needs testing => resolved
2014-07-04 17:36	Arco	Resolution	open => fixed
2018-09-30 22:38	Blzut3	Status	resolved => closed