Skip to content

Commit

Permalink
Fix cg.skulltrails out of bounds access in Team Arena Harvester mode
Browse files Browse the repository at this point in the history 
In Team Arena's Harvester mode, players corrupt your memory from beyond
the grave. Gib the players to stop the corruption!

CG_PlayerTokens is called for player entities, including corpses.
The entity number is used for the index in cg.skulltrails which only has
MAX_CLIENTS elements. This results in incorrect memory being overwritten
for corpse entities (as the entity number is >= MAX_CLIENTS).

So limit skull trails to valid entities (entity number < MAX_CLIENTS).
  • Loading branch information
@zturtleman
zturtleman committed May 2, 2014
1 parent 7beff8b commit b9061c8
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions code/cgame/cg_players.c
View file
Expand Up @@ -1774,6 +1774,9 @@ static void CG_PlayerTokens( centity_t *cent, int renderfx ) {
refEntity_t ent;
vec3_t dir, origin;
skulltrail_t *trail;
if ( cent->currentState.number >= MAX_CLIENTS ) {
return;
}
trail = &cg.skulltrails[cent->currentState.number];
tokens = cent->currentState.generic1;
if ( !tokens ) {
Expand Down

5 comments on commit b9061c8

@robo9k
Copy link

@robo9k robo9k commented on b9061c8 May 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be safer to check;

    if ( cent->currentState.number >= ARRAY_LEN(cg.skulltrails) ) {

The array length is MAX_CLIENTS, but this way you could change it without breaking the check. Furthermore I find it makes it more obvious that we're checking the bounds of skulltrails[] instead of using MAX_CLIENTS out of nowhere.

@zturtleman
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about using ARRAY_LEN but it didn't seem like it fit the existing code style. I think this is how it would of been fixed 14 years ago if it had been found. ARRAY_LEN is generally used for arrays that the compiler determines the size based on initialized elements.

@ensiform
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes more sense to make a define for max skull trails that redefines value of max clients then ?

@robo9k
Copy link

@robo9k robo9k commented on b9061c8 May 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about using ARRAY_LEN but it didn't seem like it fit the existing code style. I think this is how it would of been fixed 14 years ago if it had been found.

I can't really argue against that, but maybe we can improve things a little now?

ARRAY_LEN is generally used for arrays that the compiler determines the size based on initialized elements.

cg_t's skulltrails[MAX_CLIENTS] can be used with ARRAY_LEN if I didn't miss something.

Probably makes more sense to make a define for max skull trails that redefines value of max clients then ?

I don't quite see what a #define MAX_CLIENT_SKULLTRAILS MAX_CLIENTS (MAX_SKULLTRAIL is already defined, adding a plural s sounds too similar imho) would improve?
Sure, you could compare >= MAX_CLIENT_SKULLTRAILS but you'd still need to look up how cg.skulltrails[] is actually defined in order to understand the relation of MAX_CLIENT_SKULLTRAILS ↔ skulltrails[].

I'm sorry for causing so much noise about such a little change 😅

@ensiform
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad robo :}

Please sign in to comment.