From 560d7e644661fc0f56067035de966bd8d1c33e66 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Sun, 29 Oct 2023 19:14:12 +0000 Subject: [PATCH 1/4] Fix some UB in movie_lib.cc Fixes warnings shown by UBSAN during the title movie playback. Mostly misaligned loads and a couple of bad shifts. --- CMakeLists.txt | 1 + src/endian.hpp | 24 ++++++++++++++++++++ src/movie_lib.cc | 57 ++++++++++++++++++++++-------------------------- 3 files changed, 51 insertions(+), 31 deletions(-) create mode 100644 src/endian.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 45fb995..b6abd46 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -252,6 +252,7 @@ target_sources(${EXECUTABLE_NAME} PUBLIC target_sources(${EXECUTABLE_NAME} PUBLIC "src/audio_engine.cc" "src/audio_engine.h" + "src/endian.hpp" "src/fps_limiter.cc" "src/fps_limiter.h" "src/platform_compat.cc" diff --git a/src/endian.hpp b/src/endian.hpp new file mode 100644 index 0000000..e8422ea --- /dev/null +++ b/src/endian.hpp @@ -0,0 +1,24 @@ +#ifndef ENDIAN_HPP +#define ENDIAN_HPP + +#include + +namespace fallout { + +template +constexpr uint16_t LoadLE16(const T* b) +{ + static_assert(sizeof(T) == 1, "invalid argument"); + return (static_cast(b[1]) << 8) | static_cast(b[0]); +} + +template +constexpr uint32_t LoadLE32(const T* b) +{ + static_assert(sizeof(T) == 1, "invalid argument"); + return (static_cast(b[3]) << 24) | (static_cast(b[2]) << 16) | (static_cast(b[1]) << 8) | static_cast(b[0]); +} + +} // namespace fallout + +#endif /* ENDIAN_HPP */ diff --git a/src/movie_lib.cc b/src/movie_lib.cc index 70123e6..a594edf 100644 --- a/src/movie_lib.cc +++ b/src/movie_lib.cc @@ -5,10 +5,12 @@ #include "movie_lib.h" #include +#include #include #include #include "audio_engine.h" +#include "endian.hpp" #include "platform_compat.h" namespace fallout { @@ -757,7 +759,7 @@ static unsigned char* _ioNextRecord() return NULL; } - _io_next_hdr = *(int*)(buf + (_io_next_hdr & 0xFFFF)); + _io_next_hdr = LoadLE32(buf + (_io_next_hdr & 0xFFFF)); return buf; } @@ -854,7 +856,7 @@ int _MVE_rmStepMovie() } while (1) { - v5 = *(unsigned int*)((unsigned char*)v1 + v0); + v5 = LoadLE32((unsigned char*)v1 + v0); v1 = (unsigned short*)((unsigned char*)v1 + v0 + 4); v0 = v5 & 0xFFFF; @@ -877,7 +879,7 @@ int _MVE_rmStepMovie() } else { v7 = (v1[1] & 0x04) >> 2; } - v8 = *(unsigned int*)((unsigned char*)v1 + 6); + v8 = LoadLE32((unsigned char*)v1 + 6); if ((v5 >> 24) == 0) { v8 &= 0xFFFF; } @@ -1438,7 +1440,7 @@ static int _MVE_sndAdd(unsigned char* dest, unsigned char** src_ptr, int a3, int } if (a5) { - v12 = *(unsigned int*)src; + v12 = LoadLE32(src); src += 4; *(unsigned int*)dest = v12; @@ -1868,8 +1870,6 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in int i; int j; ptrdiff_t v10; - int v11; - int v13; int byte; unsigned int value1; unsigned int value2; @@ -1895,6 +1895,10 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in dest = gMovieDirectDrawSurfaceBuffer1 + dword_6B401B + _mveBW * dword_6B401F; } + constexpr auto getOffset = [](uint16_t v) { + return static_cast(v & 0xFF) + dword_51F018[v >> 8]; + }; + while (a6--) { v49 = a5 >> 1; while (v49--) { @@ -1918,39 +1922,33 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in v10 = gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1; break; case 2: - case 3: + case 3: { byte = *a2++; - v11 = word_51F618[byte]; + uint16_t offset = word_51F618[byte]; if (v7 == 3) { - v11 = ((-(v11 & 0xFF)) & 0xFF) | ((-(v11 >> 8) & 0xFF) << 8); - } else { - v11 = v11; + offset = ((-(offset & 0xFF)) & 0xFF) | ((-(offset >> 8) & 0xFF) << 8); } - v10 = ((v11 << 24) >> 24) + dword_51F018[v11 >> 8]; - break; + v10 = getOffset(offset); + } break; case 4: - case 5: + case 5: { + uint16_t offset; if (v7 == 4) { byte = *a2++; - v13 = word_51F418[byte]; + offset = word_51F418[byte]; } else { - v13 = *(unsigned short*)a2; + offset = LoadLE16(a2); a2 += 2; } - v10 = ((v13 << 24) >> 24) + dword_51F018[v13 >> 8] + (gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1); - break; + v10 = getOffset(offset) + (gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1); + } break; } value2 = _mveBW; - for (i = 0; i < 8; i++) { - src_ptr = (unsigned int*)(dest + v10); - dest_ptr = (unsigned int*)dest; - - dest_ptr[0] = src_ptr[0]; - dest_ptr[1] = src_ptr[1]; - + for (i = 0; i < 8; ++i) { + memcpy(dest, dest + v10, 8); dest += value2; } @@ -2669,11 +2667,8 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in case 11: value2 = _mveBW; - src_ptr = (unsigned int*)a2; - for (i = 0; i < 8; i++) { - dest_ptr = (unsigned int*)dest; - dest_ptr[0] = src_ptr[i * 2]; - dest_ptr[1] = src_ptr[i * 2 + 1]; + for (i = 0; i < 32; i += 4) { + memcpy(dest, &a2[i * 2], 8); dest += value2; } @@ -2763,7 +2758,7 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in value1 = byte | (byte << 8) | (byte << 16) | (byte << 24); value2 = value1; } else { - byte = *(unsigned short*)a2; + byte = LoadLE16(a2); a2 += 2; value1 = byte | (byte << 16); value2 = value1; From 12546a72a3742f8a3a8c7b6157548e4d211f7070 Mon Sep 17 00:00:00 2001 From: Alexander Batalov Date: Tue, 5 Mar 2024 21:31:25 +0300 Subject: [PATCH 2/4] Reorganize --- CMakeLists.txt | 1 - src/endian.hpp | 24 ------------------------ src/movie_lib.cc | 11 ++++++++++- 3 files changed, 10 insertions(+), 26 deletions(-) delete mode 100644 src/endian.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 94ac050..bd50a9e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -264,7 +264,6 @@ target_sources(${EXECUTABLE_NAME} PUBLIC target_sources(${EXECUTABLE_NAME} PUBLIC "src/audio_engine.cc" "src/audio_engine.h" - "src/endian.hpp" "src/fps_limiter.cc" "src/fps_limiter.h" "src/platform_compat.cc" diff --git a/src/endian.hpp b/src/endian.hpp deleted file mode 100644 index e8422ea..0000000 --- a/src/endian.hpp +++ /dev/null @@ -1,24 +0,0 @@ -#ifndef ENDIAN_HPP -#define ENDIAN_HPP - -#include - -namespace fallout { - -template -constexpr uint16_t LoadLE16(const T* b) -{ - static_assert(sizeof(T) == 1, "invalid argument"); - return (static_cast(b[1]) << 8) | static_cast(b[0]); -} - -template -constexpr uint32_t LoadLE32(const T* b) -{ - static_assert(sizeof(T) == 1, "invalid argument"); - return (static_cast(b[3]) << 24) | (static_cast(b[2]) << 16) | (static_cast(b[1]) << 8) | static_cast(b[0]); -} - -} // namespace fallout - -#endif /* ENDIAN_HPP */ diff --git a/src/movie_lib.cc b/src/movie_lib.cc index a594edf..1d9929a 100644 --- a/src/movie_lib.cc +++ b/src/movie_lib.cc @@ -10,7 +10,6 @@ #include #include "audio_engine.h" -#include "endian.hpp" #include "platform_compat.h" namespace fallout { @@ -98,6 +97,16 @@ static int _MVE_sndDecompS16(unsigned short* a1, unsigned char* a2, int a3, int static void _nfPkConfig(); static void _nfPkDecomp(unsigned char* buf, unsigned char* a2, int a3, int a4, int a5, int a6); +constexpr static uint16_t LoadLE16(const uint8_t* b) +{ + return (b[1] << 8) | b[0]; +} + +constexpr static uint32_t LoadLE32(const uint8_t* b) +{ + return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0]; +} + // 0x51EBD8 static int dword_51EBD8 = 0; From 120aecefb8ad716a1ae2561ed4d0e4e0d28f1190 Mon Sep 17 00:00:00 2001 From: Alexander Batalov Date: Tue, 5 Mar 2024 21:49:40 +0300 Subject: [PATCH 3/4] Reorganize --- src/movie_lib.cc | 83 +++++++++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 37 deletions(-) diff --git a/src/movie_lib.cc b/src/movie_lib.cc index 1d9929a..95d43e3 100644 --- a/src/movie_lib.cc +++ b/src/movie_lib.cc @@ -5,7 +5,7 @@ #include "movie_lib.h" #include -#include +#include #include #include @@ -97,15 +97,9 @@ static int _MVE_sndDecompS16(unsigned short* a1, unsigned char* a2, int a3, int static void _nfPkConfig(); static void _nfPkDecomp(unsigned char* buf, unsigned char* a2, int a3, int a4, int a5, int a6); -constexpr static uint16_t LoadLE16(const uint8_t* b) -{ - return (b[1] << 8) | b[0]; -} - -constexpr static uint32_t LoadLE32(const uint8_t* b) -{ - return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0]; -} +static constexpr uint16_t loadUInt16LE(const uint8_t* b); +static constexpr uint32_t loadUInt32LE(const uint8_t* b); +static constexpr uint8_t getOffset(uint16_t v); // 0x51EBD8 static int dword_51EBD8 = 0; @@ -768,7 +762,7 @@ static unsigned char* _ioNextRecord() return NULL; } - _io_next_hdr = LoadLE32(buf + (_io_next_hdr & 0xFFFF)); + _io_next_hdr = loadUInt32LE(buf + (_io_next_hdr & 0xFFFF)); return buf; } @@ -865,7 +859,7 @@ int _MVE_rmStepMovie() } while (1) { - v5 = LoadLE32((unsigned char*)v1 + v0); + v5 = loadUInt32LE((unsigned char*)v1 + v0); v1 = (unsigned short*)((unsigned char*)v1 + v0 + 4); v0 = v5 & 0xFFFF; @@ -888,7 +882,7 @@ int _MVE_rmStepMovie() } else { v7 = (v1[1] & 0x04) >> 2; } - v8 = LoadLE32((unsigned char*)v1 + 6); + v8 = loadUInt32LE((unsigned char*)v1 + 6); if ((v5 >> 24) == 0) { v8 &= 0xFFFF; } @@ -1449,7 +1443,7 @@ static int _MVE_sndAdd(unsigned char* dest, unsigned char** src_ptr, int a3, int } if (a5) { - v12 = LoadLE32(src); + v12 = loadUInt32LE(src); src += 4; *(unsigned int*)dest = v12; @@ -1904,10 +1898,6 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in dest = gMovieDirectDrawSurfaceBuffer1 + dword_6B401B + _mveBW * dword_6B401F; } - constexpr auto getOffset = [](uint16_t v) { - return static_cast(v & 0xFF) + dword_51F018[v >> 8]; - }; - while (a6--) { v49 = a5 >> 1; while (v49--) { @@ -1931,27 +1921,31 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in v10 = gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1; break; case 2: - case 3: { - byte = *a2++; - uint16_t offset = word_51F618[byte]; - if (v7 == 3) { - offset = ((-(offset & 0xFF)) & 0xFF) | ((-(offset >> 8) & 0xFF) << 8); - } - v10 = getOffset(offset); - } break; - case 4: - case 5: { - uint16_t offset; - if (v7 == 4) { + case 3: + if (1) { byte = *a2++; - offset = word_51F418[byte]; - } else { - offset = LoadLE16(a2); - a2 += 2; + uint16_t offset = word_51F618[byte]; + if (v7 == 3) { + offset = ((-(offset & 0xFF)) & 0xFF) | ((-(offset >> 8) & 0xFF) << 8); + } + v10 = getOffset(offset); } + break; + case 4: + case 5: + if (1) { + uint16_t offset; + if (v7 == 4) { + byte = *a2++; + offset = word_51F418[byte]; + } else { + offset = loadUInt16LE(a2); + a2 += 2; + } - v10 = getOffset(offset) + (gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1); - } break; + v10 = getOffset(offset) + (gMovieDirectDrawSurfaceBuffer2 - gMovieDirectDrawSurfaceBuffer1); + } + break; } value2 = _mveBW; @@ -2767,7 +2761,7 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in value1 = byte | (byte << 8) | (byte << 16) | (byte << 24); value2 = value1; } else { - byte = LoadLE16(a2); + byte = loadUInt16LE(a2); a2 += 2; value1 = byte | (byte << 16); value2 = value1; @@ -2798,4 +2792,19 @@ static void _nfPkDecomp(unsigned char* a1, unsigned char* a2, int a3, int a4, in } } +constexpr uint16_t loadUInt16LE(const uint8_t* b) +{ + return (b[1] << 8) | b[0]; +} + +constexpr uint32_t loadUInt32LE(const uint8_t* b) +{ + return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0]; +} + +constexpr uint8_t getOffset(uint16_t v) +{ + return static_cast(v & 0xFF) + dword_51F018[v >> 8]; +} + } // namespace fallout From 08132a2836667eaf517ce218a4c44cd162f4adfa Mon Sep 17 00:00:00 2001 From: Alexander Batalov Date: Tue, 5 Mar 2024 22:05:43 +0300 Subject: [PATCH 4/4] Remove constexpr --- src/movie_lib.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/movie_lib.cc b/src/movie_lib.cc index 95d43e3..fd2f158 100644 --- a/src/movie_lib.cc +++ b/src/movie_lib.cc @@ -99,7 +99,7 @@ static void _nfPkDecomp(unsigned char* buf, unsigned char* a2, int a3, int a4, i static constexpr uint16_t loadUInt16LE(const uint8_t* b); static constexpr uint32_t loadUInt32LE(const uint8_t* b); -static constexpr uint8_t getOffset(uint16_t v); +static uint8_t getOffset(uint16_t v); // 0x51EBD8 static int dword_51EBD8 = 0; @@ -2802,7 +2802,7 @@ constexpr uint32_t loadUInt32LE(const uint8_t* b) return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0]; } -constexpr uint8_t getOffset(uint16_t v) +uint8_t getOffset(uint16_t v) { return static_cast(v & 0xFF) + dword_51F018[v >> 8]; }