From a8feba029f157a2f009fe712b8cf988cd6571bf7 Mon Sep 17 00:00:00 2001
From: Misa <infoteddy@infoteddy.info>
Date: Thu, 24 Mar 2022 09:35:01 -0700
Subject: [PATCH] Clean up and harden music loading code

During review of #869, I looked at this part of the codebase again. I
have no idea how or why, but during the course of 2.4 this whole area
just became a mess.

The issues I fixed (in no particular order):
- Copy-pasting the code that loads from the binary blobs
- Making sure SDL_RWFromConstMem is used over SDL_RWFromMem wherever
  possible
- Adding checks to make sure the index from the binary blob is valid
  (it's possible it could not exist)
- Adding checks to make sure we gracefully handle
  SDL_RWFromConstMem/PHYSFSRWOPS_openRead returning NULL
- Moving the pointer asterisk to the type instead of the name :)
---
 desktop_version/src/Music.cpp | 46 ++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/desktop_version/src/Music.cpp b/desktop_version/src/Music.cpp
index 289de2c7..34dc2e5a 100644
--- a/desktop_version/src/Music.cpp
+++ b/desktop_version/src/Music.cpp
@@ -244,10 +244,22 @@ void musicclass::init(void)
             int index;
             SDL_RWops* rw;
 
-#define FOREACH_TRACK(blob, track_name) \
+#define TRACK_LOAD_BLOB(blob, track_name) \
     index = blob.getIndex("data/" track_name); \
-    rw = SDL_RWFromMem(blob.getAddress(index), blob.getSize(index)); \
-    musicTracks.push_back(MusicTrack( rw ));
+    if (index >= 0 && index < blob.max_headers) \
+    { \
+        rw = SDL_RWFromConstMem(blob.getAddress(index), blob.getSize(index)); \
+        if (rw == NULL) \
+        { \
+            vlog_error("Unable to read music file header: %s", SDL_GetError()); \
+        } \
+        else \
+        { \
+            musicTracks.push_back(MusicTrack(rw)); \
+        } \
+    }
+
+#define FOREACH_TRACK(blob, track_name) TRACK_LOAD_BLOB(blob, track_name)
 
             TRACK_NAMES(pppppp_blob)
 
@@ -258,9 +270,17 @@ void musicclass::init(void)
             vlog_info("Loading music from loose files...");
 
             SDL_RWops* rw;
+
 #define FOREACH_TRACK(_, track_name) \
     rw = PHYSFSRWOPS_openRead(track_name); \
-    musicTracks.push_back(MusicTrack( rw ));
+    if (rw == NULL) \
+    { \
+        vlog_error("Unable to read loose music file: %s", SDL_GetError()); \
+    } \
+    else \
+    { \
+        musicTracks.push_back(MusicTrack(rw)); \
+    }
 
             TRACK_NAMES(_)
 
@@ -273,22 +293,9 @@ void musicclass::init(void)
 
         mmmmmm = true;
         int index;
-        SDL_RWops *rw;
+        SDL_RWops* rw;
 
-#define FOREACH_TRACK(blob, track_name) \
-    index = blob.getIndex("data/" track_name); \
-    if (index >= 0 && index < blob.max_headers) \
-    { \
-        rw = SDL_RWFromConstMem(blob.getAddress(index), blob.getSize(index)); \
-        if (rw == NULL) \
-        { \
-            vlog_error("Unable to read music file header: %s", SDL_GetError()); \
-        } \
-        else \
-        { \
-            musicTracks.push_back(MusicTrack( rw )); \
-        } \
-    }
+#define FOREACH_TRACK(blob, track_name) TRACK_LOAD_BLOB(blob, track_name)
 
         TRACK_NAMES(mmmmmm_blob)
 
@@ -310,6 +317,7 @@ void musicclass::init(void)
     TRACK_NAMES(pppppp_blob)
 
 #undef FOREACH_TRACK
+#undef TRACK_LOAD_BLOB
     }
 
     num_pppppp_tracks += musicTracks.size() - num_mmmmmm_tracks;