Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bounds check in S_StartSound #163

Closed

Conversation

UncannyFish
Copy link
Contributor

I found it while trying to debug mvdevs/mvsdk#14

@aufau
Copy link
Member

aufau commented Jun 4, 2023

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
Copy link
Contributor Author

I didn't check for that.
I see entityNum is used in multiple places as index to array loopSounds.

static loopSound_t loopSounds[MAX_GENTITIES];

If entityNum is MAX_GENTITIES then array will overflow and jk2mv will crash anyway.

@aufau
Copy link
Member

aufau commented Jun 4, 2023

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
Copy link
Contributor Author

I don't have free time for this anymore.

@UncannyFish UncannyFish closed this Jul 7, 2023
@UncannyFish UncannyFish deleted the S_StartSound-bounds-check-fix branch July 11, 2023 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants