From 81f7a21845274fb822b1cbc709d775e6648996fd Mon Sep 17 00:00:00 2001
From: "John C. Allwein" <git@johnnyapol.me>
Date: Sun, 29 Jan 2023 14:58:24 -0700
Subject: [PATCH] openal: fix potential iterator invalidation in
 OpenALAudioManager::update

OpenALAudioManager::update iterates through all currently playing sounds
via a std::set / phash_set object, _sounds_playing.

If a stream queue corruption was detected during OpenALAudioSound::pull_used_buffers,
the logic added in a895890 would call cleanup() on
the sound if we could not successfully locate the target buffer and log an error.

However, the act of calling OpenALAudioSound::cleanup would lead to calling
stop() (since the sound was actively playing). In OpenALAudioSound::stop(),
we would then proceed to call _manager->stopping_sound which would erase
the current sound from _sounds_playing (while we still held an iterator
to it). Per STL standard and real-world observation, std::set::erase
will invalidate the current iterator held in update (https://en.cppreference.com/w/cpp/container/set/erase).
This leads to a segmentation fault when we attempt the next iteration on the loop.

To resolve this, let's ensure we don't hold onto invalid iterators during the updating of playing sounds.

Fixes #1452
---
 panda/src/audiotraits/openalAudioManager.cxx | 33 ++++++++++----------
 panda/src/audiotraits/openalAudioSound.cxx   |  2 +-
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/panda/src/audiotraits/openalAudioManager.cxx b/panda/src/audiotraits/openalAudioManager.cxx
index 67e9bc5379..0e6e06d16a 100644
--- a/panda/src/audiotraits/openalAudioManager.cxx
+++ b/panda/src/audiotraits/openalAudioManager.cxx
@@ -959,32 +959,33 @@ stop_all_sounds() {
  */
 void OpenALAudioManager::
 update() {
-  ReMutexHolder holder(_lock);
+  ReMutexHolder const holder(_lock);
 
-  // See if any of our playing sounds have ended we must first collect a
-  // seperate list of finished sounds and then iterated over those again
-  // calling their finished method.  We can't call finished() within a loop
-  // iterating over _sounds_playing since finished() modifies _sounds_playing
-  SoundsPlaying sounds_finished;
-
-  double rtc = TrueClock::get_global_ptr()->get_short_time();
-  SoundsPlaying::iterator it = _sounds_playing.begin();
-  while (it != _sounds_playing.end()) {
-    PT(OpenALAudioSound) sound = *(it++);
+  // See if any of our playing sounds have ended.
+  double const rtc = TrueClock::get_global_ptr()->get_short_time();
+  SoundsPlaying::iterator i = _sounds_playing.begin();
+  while (i != _sounds_playing.end()) {
+    // The post-increment syntax is *very* important here.
+    // As both OpenALAudioSound::pull_used_buffers and OpenALAudioSound::finished can modify the list of sounds playing
+    // by erasing 'sound' from the list, thus invaliding the iterator.
+    PT(OpenALAudioSound) sound = *(i++);
     sound->pull_used_buffers();
+
+    // If pull_used_buffers() encountered an error, the sound was cleaned up.
+    // No need to do anymore work.
+    if (!sound->is_valid()) {
+      continue;
+    }
+
     sound->push_fresh_buffers();
     sound->restart_stalled_audio();
     sound->cache_time(rtc);
     if (sound->_source == 0 ||
         (sound->_stream_queued.empty() &&
          (sound->_loops_completed >= sound->_playing_loops))) {
-      sounds_finished.insert(std::move(sound));
+      sound->finished();
     }
   }
-
-  for (OpenALAudioSound *sound : sounds_finished) {
-    sound->finished();
-  }
 }
 
 
diff --git a/panda/src/audiotraits/openalAudioSound.cxx b/panda/src/audiotraits/openalAudioSound.cxx
index cba7940f83..13b7ff1305 100644
--- a/panda/src/audiotraits/openalAudioSound.cxx
+++ b/panda/src/audiotraits/openalAudioSound.cxx
@@ -536,7 +536,7 @@ pull_used_buffers() {
           }
         }
         if (!found_culprit) {
-          audio_error("corruption in stream queue");
+          audio_error(get_name() << ": corruption in stream queue");
           cleanup();
           return;
         }