From 690912e209412eebf361fa17684ff5af5b8ae4b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:02:40 +0200 Subject: [PATCH 01/10] Fix invalid sound index used when loading external sound fails This should be highly unlikely because external sound mapres were never supported by the editor. --- src/game/editor/mapitems/sound.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/game/editor/mapitems/sound.h b/src/game/editor/mapitems/sound.h index 13a2290c6..360d54546 100644 --- a/src/game/editor/mapitems/sound.h +++ b/src/game/editor/mapitems/sound.h @@ -10,7 +10,7 @@ public: explicit CEditorSound(CEditor *pEditor); ~CEditorSound(); - int m_SoundId = 0; + int m_SoundId = -1; char m_aName[IO_MAX_PATH_LENGTH] = ""; void *m_pData = nullptr; From 3b0d9cd6c92907ec9ac5b3e73a82cf8a89d7f787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:03:05 +0200 Subject: [PATCH 02/10] Fix editor crash with sound preview when sound is not valid This happens if a sound could not be loaded successfully. --- src/game/editor/editor.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/game/editor/editor.cpp b/src/game/editor/editor.cpp index 47091a0c8..226401461 100644 --- a/src/game/editor/editor.cpp +++ b/src/game/editor/editor.cpp @@ -1379,12 +1379,17 @@ void CEditor::DoToolbarSounds(CUIRect ToolBar) if(pSelectedSound->m_SoundId != m_ToolbarPreviewSound && m_ToolbarPreviewSound >= 0 && Sound()->IsPlaying(m_ToolbarPreviewSound)) Sound()->Stop(m_ToolbarPreviewSound); m_ToolbarPreviewSound = pSelectedSound->m_SoundId; + } + else + { + m_ToolbarPreviewSound = -1; + } + if(m_ToolbarPreviewSound >= 0) + { static int s_PlayPauseButton, s_StopButton, s_SeekBar = 0; DoAudioPreview(ToolBarBottom, &s_PlayPauseButton, &s_StopButton, &s_SeekBar, m_ToolbarPreviewSound); } - else - m_ToolbarPreviewSound = -1; } static void Rotate(const CPoint *pCenter, CPoint *pPoint, float Rotation) From 941a302c4d6ec2219f235b85a73f7023de05ce0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:03:19 +0200 Subject: [PATCH 03/10] Ensure editor preview image and sound are unloaded properly The preview image was previously not unloaded and the preview state was not reset. --- src/game/editor/editor.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/game/editor/editor.cpp b/src/game/editor/editor.cpp index 226401461..0a20304c2 100644 --- a/src/game/editor/editor.cpp +++ b/src/game/editor/editor.cpp @@ -8710,11 +8710,10 @@ void CEditor::OnClose() void CEditor::OnDialogClose() { - if(m_FilePreviewSound >= 0) - { - Sound()->UnloadSample(m_FilePreviewSound); - m_FilePreviewSound = -1; - } + Graphics()->UnloadTexture(&m_FilePreviewImage); + Sound()->UnloadSample(m_FilePreviewSound); + m_FilePreviewSound = -1; + m_FilePreviewState = PREVIEW_UNLOADED; } void CEditor::LoadCurrentMap() From 1153507216b787ffe974d6661cd0c58118c96974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:03:38 +0200 Subject: [PATCH 04/10] Fix double-free when reading opus file fails Set the data pointer of the sample only when the sample has been loaded successfully, so the invalid sample data is not freed again when decoding fails. --- src/engine/client/sound.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 2b4f3ce63..4aa8b38ba 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -355,15 +355,15 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c return false; } - Sample.m_pData = (short *)calloc((size_t)NumSamples * NumChannels, sizeof(short)); + short *pSampleData = (short *)calloc((size_t)NumSamples * NumChannels, sizeof(short)); int Pos = 0; while(Pos < NumSamples) { - const int Read = op_read(pOpusFile, Sample.m_pData + Pos * NumChannels, NumSamples * NumChannels, nullptr); + const int Read = op_read(pOpusFile, pSampleData + Pos * NumChannels, NumSamples * NumChannels, nullptr); if(Read < 0) { - free(Sample.m_pData); + free(pSampleData); dbg_msg("sound/opus", "op_read error %d at %d", Read, Pos); return false; } @@ -372,6 +372,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c Pos += Read; } + Sample.m_pData = pSampleData; Sample.m_NumFrames = Pos; Sample.m_Rate = 48000; Sample.m_LoopStart = -1; From 4d37775c17eaf4265d185ed0b0143c5a3ed54b17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:17:15 +0200 Subject: [PATCH 05/10] Only change sample variables when it was decoded successfully Avoid changing the sample member variables until the sample was decoded successfully. --- src/engine/client/sound.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 4aa8b38ba..fd6c2fe62 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -347,9 +347,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c const int NumChannels = op_channel_count(pOpusFile, -1); const int NumSamples = op_pcm_total(pOpusFile, -1); // per channel! - Sample.m_Channels = NumChannels; - - if(Sample.m_Channels > 2) + if(NumChannels > 2) { dbg_msg("sound/opus", "file is not mono or stereo."); return false; @@ -375,6 +373,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c Sample.m_pData = pSampleData; Sample.m_NumFrames = Pos; Sample.m_Rate = 48000; + Sample.m_Channels = NumChannels; Sample.m_LoopStart = -1; Sample.m_LoopEnd = -1; Sample.m_PausedAt = 0; @@ -460,10 +459,7 @@ bool CSound::DecodeWV(CSample &Sample, const void *pData, unsigned DataSize) con const unsigned int SampleRate = WavpackGetSampleRate(pContext); const int NumChannels = WavpackGetNumChannels(pContext); - Sample.m_Channels = NumChannels; - Sample.m_Rate = SampleRate; - - if(Sample.m_Channels > 2) + if(NumChannels > 2) { dbg_msg("sound/wv", "file is not mono or stereo."); s_pWVBuffer = nullptr; @@ -499,6 +495,8 @@ bool CSound::DecodeWV(CSample &Sample, const void *pData, unsigned DataSize) con #endif Sample.m_NumFrames = NumSamples; + Sample.m_Rate = SampleRate; + Sample.m_Channels = NumChannels; Sample.m_LoopStart = -1; Sample.m_LoopEnd = -1; Sample.m_PausedAt = 0; From 51012bcc1b91a4f8c70bb356810b012509e4a6de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:21:04 +0200 Subject: [PATCH 06/10] Fix potential out-of-bounds writes on invalid opus files The third parameter of the `op_read` function specifies the remaining size of the buffer, but we always passed the total size of the buffer without respecting the position at which the data is written into the buffer. --- src/engine/client/sound.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index fd6c2fe62..59714176b 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -358,7 +358,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c int Pos = 0; while(Pos < NumSamples) { - const int Read = op_read(pOpusFile, pSampleData + Pos * NumChannels, NumSamples * NumChannels, nullptr); + const int Read = op_read(pOpusFile, pSampleData + Pos * NumChannels, (NumSamples - Pos) * NumChannels, nullptr); if(Read < 0) { free(pSampleData); From cfb5b15222eadbc86855c6ee38831482df34e010 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:27:49 +0200 Subject: [PATCH 07/10] Log error code if opus file cannot be opened --- src/engine/client/sound.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 59714176b..f88eb7b6a 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -341,7 +341,8 @@ void CSound::RateConvert(CSample &Sample) const bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) const { - OggOpusFile *pOpusFile = op_open_memory((const unsigned char *)pData, DataSize, nullptr); + int OpusError = 0; + OggOpusFile *pOpusFile = op_open_memory((const unsigned char *)pData, DataSize, &OpusError); if(pOpusFile) { const int NumChannels = op_channel_count(pOpusFile, -1); @@ -380,7 +381,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c } else { - dbg_msg("sound/opus", "failed to decode sample"); + dbg_msg("sound/opus", "failed to decode sample, error %d", OpusError); return false; } From d0e27fdcd4dd6cc10eed5825132d681eae924360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:28:07 +0200 Subject: [PATCH 08/10] Fix memory leak of opus file structure We were not calling `op_free` for `OggOpusFile *` after decoding opus files. --- src/engine/client/sound.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index f88eb7b6a..9342ed57a 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -350,6 +350,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c if(NumChannels > 2) { + op_free(pOpusFile); dbg_msg("sound/opus", "file is not mono or stereo."); return false; } @@ -363,6 +364,7 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c if(Read < 0) { free(pSampleData); + op_free(pOpusFile); dbg_msg("sound/opus", "op_read error %d at %d", Read, Pos); return false; } @@ -371,6 +373,8 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c Pos += Read; } + op_free(pOpusFile); + Sample.m_pData = pSampleData; Sample.m_NumFrames = Pos; Sample.m_Rate = 48000; From d06f6d3370d75dc72ee451a25a1bbb7841798498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 20:33:59 +0200 Subject: [PATCH 09/10] Handle failure of `op_pcm_total` function According to the documentation, this function returns `0` on success and a negative number (error code) otherwise, which would cause an allocation of an invalid size. --- src/engine/client/sound.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 9342ed57a..1ef64c204 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -346,8 +346,6 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c if(pOpusFile) { const int NumChannels = op_channel_count(pOpusFile, -1); - const int NumSamples = op_pcm_total(pOpusFile, -1); // per channel! - if(NumChannels > 2) { op_free(pOpusFile); @@ -355,6 +353,14 @@ bool CSound::DecodeOpus(CSample &Sample, const void *pData, unsigned DataSize) c return false; } + const int NumSamples = op_pcm_total(pOpusFile, -1); // per channel! + if(NumSamples < 0) + { + op_free(pOpusFile); + dbg_msg("sound/opus", "failed to get number of samples, error %d", NumSamples); + return false; + } + short *pSampleData = (short *)calloc((size_t)NumSamples * NumChannels, sizeof(short)); int Pos = 0; From 9cf3094934f38faf6046dcc9e0e2b0f584640c2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Thu, 25 Apr 2024 21:42:53 +0200 Subject: [PATCH 10/10] Also check for incorrect sample index with assertion Ensure the sample being allocated is not currently used also by checking its next free sample index. --- src/engine/client/sound.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/engine/client/sound.cpp b/src/engine/client/sound.cpp index 1ef64c204..3bfb8294e 100644 --- a/src/engine/client/sound.cpp +++ b/src/engine/client/sound.cpp @@ -292,14 +292,15 @@ CSample *CSound::AllocSample() return nullptr; CSample *pSample = &m_aSamples[m_FirstFreeSampleIndex]; - m_FirstFreeSampleIndex = pSample->m_NextFreeSampleIndex; - pSample->m_NextFreeSampleIndex = SAMPLE_INDEX_USED; - if(pSample->m_pData != nullptr) + if(pSample->m_pData != nullptr || pSample->m_NextFreeSampleIndex == SAMPLE_INDEX_USED) { - char aError[64]; - str_format(aError, sizeof(aError), "Sample was not unloaded (index=%d, duration=%f)", pSample->m_Index, pSample->TotalTime()); + char aError[128]; + str_format(aError, sizeof(aError), "Sample was not unloaded (index=%d, next=%d, duration=%f, data=%p)", + pSample->m_Index, pSample->m_NextFreeSampleIndex, pSample->TotalTime(), pSample->m_pData); dbg_assert(false, aError); } + m_FirstFreeSampleIndex = pSample->m_NextFreeSampleIndex; + pSample->m_NextFreeSampleIndex = SAMPLE_INDEX_USED; return pSample; }