Bug 3330 - Memory write passed the end of allocated array
Status: RESOLVED FIXED
Alias: None
Product: ioquake3
Classification: Unclassified
Component: Misc
Version: unspecified
Hardware: All All
: P3 normal
Assignee: Zachary J. Slater
QA Contact: ioquake3 bugzilla mailing list
URL:
Depends on:
Blocks:
 
Reported: 2007-08-28 03:06 EDT by beast
Modified: 2009-10-06 10:34:50 EDT
1 user (show)

See Also:


Attachments
Diff file with a proposed fix (554 bytes, patch)
2007-08-28 03:07 EDT, beast
Diff file with a proposed fix (5.72 KB, patch)
2007-08-28 14:47 EDT, beast
Diff file with a proposed fix (6.87 KB, patch)
2007-08-29 03:08 EDT, beast

Description beast 2007-08-28 03:06:39 EDT
In code/q3_ui/ui_startserver.c, there are 2 members in the startserver_t struct that are defined as arrays:

char maplist[MAX_SERVERMAPS][MAX_NAMELENGTH] 
int mapGamebits[MAX_SERVERMAPS]

MAX_SERVERMAPS is defined as 64.

The function StartServer_GametypeEvent(), loops through the list of loaded arenas (a maximum of 1024 arenas can be loaded) and matches the selected gametype with the gametype(s) defined for the map. If there is a match, the current map is added to the maplist array and the gamebits are stored in the corresponding mapGamebits array.

The problem is that there is no check to see if the array limit has been reached. The current code just keeps on incrementing the count and writing away. Depending on the number of maps that match the gametype, this could overwrite quite a bit of memory.  I probably don't have to elaborate as to the potential repercussions of this.

Most people don't have that many maps, so this is not a huge problem. But, I stumbled across it because some friends put together a mappack of 100 maps...

I have attached a .diff file with the fix for this problem.
Comment 1 beast 2007-08-28 03:07:14 EDT
Created attachment 1491 [details]
Diff file with a proposed fix
Comment 2 beast 2007-08-28 03:56:05 EDT
I went through the code a little more, and there is a large problem in the code. Please ignore this until I get a chance to post an updated diff file. The short version is that I found that in the StartServer_Cache routine, the array is written for ALL arenas that are found. This means that anyone with more than 64 maps will have memory overwritten. The previously stated problem only occurred if there were more than 64 arenas that matched the gametype. Thanks...
Comment 3 beast 2007-08-28 03:57:40 EDT
I'll take assignment of this defect until I post the updated fix...
Comment 4 beast 2007-08-28 14:47:53 EDT
Created attachment 1492 [details]
Diff file with a proposed fix
Comment 5 beast 2007-08-28 14:49:31 EDT
In StartServer_Cache, the array was being filled with all maps that were loaded. This would cause memory to be overwritten if there were more than 64 maps loaded. I removed the code for storing at that point, because it was superfluous. StartServer_Update was called shortyly after and it resets all of the data.

The maximum arenas is set to 1024, so I changed the startserver_t structure to have an integer index into the master arena list instead of keeping the map name and the gamebits in the startserver_t structure. The map names are now called as they are needed for display on the screen. The maximum is now set to MAX_ARENAS (instead of 64) to match the maximum that will ever be there. This will allow all maps to be displayed that match the criteria. (Well, all up to 1024 :-)

I've attached a new diff file with the proposed fix.
Comment 6 beast 2007-08-29 03:08:12 EDT
Created attachment 1493 [details]
Diff file with a proposed fix

After making the changes, there was still a problem with getting more than about 80 maps to show up. In q3_ui/ui_gameinfo.c, function UI_LoadArenas, there is a local variable allocated to hold all of the files that need to be parsed. This is defined as char dirlist[1024]. I changed this to 2048 to give a little more breathing room. On my test environment, which had 147 maps, I was able to get them all to show up with this change. This is a local stack variable that is only used for this function, so it shouldn't cause any issues that I am aware of. I have attached a new diff file with the proposed changes to both files.
Comment 7 Thilo Schulz 2009-10-06 10:34:50 EDT
Your patch looks reasonably clean, though I didn't do close checking on it. Applied in rev 1646