search

mvdevs / jk2mv

JK2MV - improved, modernized JK2 client and server
https://jk2mv.org
GNU General Public License v2.0
108 stars 33 forks source link

Fix bounds check in S_StartSound #163

Closed UncannyFish closed 1 year ago

UncannyFish commented 1 year ago

I found it while trying to debug https://github.com/mvdevs/mvsdk/pull/14

aufau commented 1 year ago

Hi, thanks for your pull request. did you check if S_StartSound doesn't get called with entityNum == MAX_GENTITIES? At least in the engine and retail ui/cgame?

Fixing bugs like this in the engine is never very straight forward, or we could inadvertently make some mods crash with jk2mv.

UncannyFish commented 1 year ago

I didn't check for that. I see entityNum is used in multiple places as index to array loopSounds. https://github.com/mvdevs/jk2mv/blob/5d192510ba7a1454d23230f2e20d13695457d60b/src/client/snd_dma.cpp#L146 If entityNum is MAX_GENTITIES then array will overflow and jk2mv will crash anyway.

aufau commented 1 year ago

Well in C a program doesn't crash when you overflow an array. There was a lot of overflows in original game and it somehow worked. simply making game quit when they happen would make a lot of old mods, maps, models etc. unusable with jk2mv. We usually have to find different solutions when such overflow is detected. Unless it's very unlikely that no mod ever managed to use such bug.

If nobody checked it, it would probably be safer to just raise loopSounds array size by 1 for now. Unless there is a strong argument it's unlikely any mod was able to use it.

UncannyFish commented 1 year ago

I don't have free time for this anymore.