Fix NULL pointer dereference in S3StartSound (#284) #285
Conversation
zear
force-pushed
the
fix_pratcam_sfx
branch
from
c27e1d2 to
b7ff021
Compare
src/S3/audio.c
Outdated
| @@ -793,6 +793,18 @@ tS3_sound_tag S3StartSound(tS3_outlet* pOutlet, tS3_sound_id pSound) { | |||
| gS3_last_error = eS3_error_bad_id; | |||
| return 0; | |||
| } | |||
| #if defined(S3_FIX_BUGS) | |||
There was a problem hiding this comment.
I think we could use DETHRACE_FIX_BUGS here - its probably unlikely someone would want to fix bugs in one area but not the other
src/S3/audio.c
Outdated
| @@ -793,6 +793,18 @@ tS3_sound_tag S3StartSound(tS3_outlet* pOutlet, tS3_sound_id pSound) { | |||
| gS3_last_error = eS3_error_bad_id; | |||
| return 0; | |||
| } | |||
| #if defined(S3_FIX_BUGS) | |||
| /* | |||
| * Load the sample early to prevent a possible NULL pointer dereference in | |||
There was a problem hiding this comment.
nice find! I wonder if a smaller fix could be done inside S3CalculateRandomizedFields itself? Something like
#if FIX_BUGS
if (desc->sound_data == NULL) {
chan->rate = desc->min_pitch;
return;
}
#endif
chan->rate = S3IRandomBetweenLog(...)?
There was a problem hiding this comment.
Done. And I can't even hear a difference between chan->rate = desc->min_pitch; and chan->rate = S3IRandomBetweenLog(...); in practice :)
zear
force-pushed
the
fix_pratcam_sfx
branch
2 times, most recently
from
b104036 to
ecb6248
Compare
…-labs#284) If the sound has been muted (!gS3_enabled) during game data loading, it is then possible to unmute it during gameplay and request a sound effect which doesn't have a sample loaded (most prominent for pratcam sfx). While logic in `S3StartSound` accommodates for such a case by loading the missing sample, it first calls `S3CalculateRandomizedFields`, which triggers a NULL pointer dereference on platforms with memory protection. This bug is most likely an overlook from the DOS era. Fix this by checking for NULL pointer before use. Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
zear
force-pushed
the
fix_pratcam_sfx
branch
from
ecb6248 to
38b5de3
Compare
| @@ -18,6 +18,9 @@ else() | |||
| /wd4996 | |||
| ) | |||
| endif() | |||
| if(DETHRACE_FIX_BUGS) | |||
There was a problem hiding this comment.
Not in this bug report, but I think we should combine BRENDER_FIX_BUGS and DETHRACE_FIX_BUGS in one cmake option + compile definition.
|
@zear I've invited you as a collaborator to the project. If you accept, you will be able to merge your own approved PRs when you are ready. And thanks! |
|
Welcome to the 🚗 club @zear ! |
If the sound has been muted (!gS3_enabled) during game data loading, it is then possible to unmute it during gameplay and request a sound effect which doesn't have a sample loaded (most prominent for pratcam sfx). While the code accommodates for such a case, it does it too late, triggering a NULL pointer dereference on platforms which feature memory protection. This bug is most likely an overlook from the DOS era.
Fix this by moving the sample NULL pointer check before its first use.
Signed-off-by: Artur Rojek contact@artur-rojek.eu