From 41f85685416961f42fd156da2703061321995597 Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 05:06:05 +0100 Subject: [PATCH 1/9] Classic Vuln --- src/CMakeLists.txt | 5 + test/CMakeLists.txt | 10 +- test/cpp_doom/CMakeLists.txt | 8 ++ test/{ => cpp_doom}/test_string_functions.cpp | 0 test/cpp_doom/test_z_native.cpp | 99 +++++++++++++++++++ test/{ => cpp_doom}/tests.cpp | 0 test/dehacked/CMakeLists.txt | 8 ++ test/dehacked/test_z_zone.cpp | 38 +++++++ test/dehacked/tests.cpp | 2 + 9 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 test/cpp_doom/CMakeLists.txt rename test/{ => cpp_doom}/test_string_functions.cpp (100%) create mode 100644 test/cpp_doom/test_z_native.cpp rename test/{ => cpp_doom}/tests.cpp (100%) create mode 100644 test/dehacked/CMakeLists.txt create mode 100644 test/dehacked/test_z_zone.cpp create mode 100644 test/dehacked/tests.cpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 87a8d280d..ddcf910b6 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -136,6 +136,11 @@ if(PNG_FOUND) list(APPEND EXTRA_LIBS PNG::PNG) endif() +add_library(lib_common_dehacked ${SOURCE_FILES_WITH_DEH}) +target_compile_definitions(lib_common_dehacked PRIVATE ${DOOM_COMPILE_DEFINITIONS}) +target_include_directories(lib_common_dehacked PRIVATE ${GAME_INCLUDE_DIRS}) +target_link_libraries(lib_common_dehacked ${EXTRA_LIBS} SampleRate::samplerate) + if(WIN32) add_executable("${PROGRAM_PREFIX}doom" WIN32 ${SOURCE_FILES_WITH_DEH} "${CMAKE_CURRENT_BINARY_DIR}/resource.rc") else() diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0a72a095f..85f236e0c 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,10 +1,4 @@ find_package(Catch2 REQUIRED) -file(GLOB_RECURSE sources CONFIGURE_DEPENDS "*.cpp") - -add_executable(test_cpp_doom ${sources}) -target_link_libraries(test_cpp_doom Catch2::Catch2 lib_common_cpp_doom) -target_compile_definitions(test_cpp_doom PUBLIC CATCH_CONFIG_CONSOLE_WIDTH=300) -target_include_directories(test_cpp_doom PRIVATE ${CMAKE_SOURCE_DIR}/src) - -add_test(NAME test_cpp_doom COMMAND test_cpp_doom) +add_subdirectory(cpp_doom) +add_subdirectory(dehacked) \ No newline at end of file diff --git a/test/cpp_doom/CMakeLists.txt b/test/cpp_doom/CMakeLists.txt new file mode 100644 index 000000000..6d8da0d56 --- /dev/null +++ b/test/cpp_doom/CMakeLists.txt @@ -0,0 +1,8 @@ +file(GLOB_RECURSE sources CONFIGURE_DEPENDS "*.cpp") + +add_executable(test_cpp_doom ${sources}) +target_link_libraries(test_cpp_doom Catch2::Catch2 lib_common_cpp_doom) +target_compile_definitions(test_cpp_doom PUBLIC CATCH_CONFIG_CONSOLE_WIDTH=300) +target_include_directories(test_cpp_doom PRIVATE ${CMAKE_SOURCE_DIR}/src) + +add_test(NAME test_cpp_doom COMMAND test_cpp_doom) diff --git a/test/test_string_functions.cpp b/test/cpp_doom/test_string_functions.cpp similarity index 100% rename from test/test_string_functions.cpp rename to test/cpp_doom/test_string_functions.cpp diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp new file mode 100644 index 000000000..28745bb46 --- /dev/null +++ b/test/cpp_doom/test_z_native.cpp @@ -0,0 +1,99 @@ +#include + +#include "z_zone.hpp" + +TEST_CASE("Z_Init", "[z_native]") { + Z_Init(); +// REQUIRE(Z_ZoneSize() == 0x2000000); +} + +#if 0 + +TEST_CASE("Z_Malloc(PU_STATIC)", "[z_native]") { + void * ptr = Z_Malloc(10, PU_STATIC, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL)", "[z_native]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVSPEC)", "[z_native]") { + void * ptr = Z_Malloc(10, PU_LEVSPEC, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free", "[z_native]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + Z_Free(ptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + Z_Free(ptr); + void * ptr2 = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr2 != nullptr); + REQUIRE(ptr == ptr2); +} + +#else + +struct memblock_t { + int id; // = ZONEID + int tag; + int size; + void ** user; + memblock_t * prev; + memblock_t * next; +}; + +long * where; +long what; + +TEST_CASE("Z_ZOverwrite header", "[z_native]") { + + void * guard = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(guard != nullptr); + + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + + void * guard2 = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(guard2 != nullptr); + + // Corrupt header + where = nullptr; + what = 0x42424242; + + auto * byte_ptr = reinterpret_cast(ptr); + auto * header = reinterpret_cast(byte_ptr - sizeof(memblock_t)); + REQUIRE(header->tag == PU_LEVEL); + long * what_ptr = &what; + long ** where_ptr = &where; + + auto distance = reinterpret_cast(&(header->next)) - reinterpret_cast(header); + if constexpr (sizeof(void *) == 8) + REQUIRE(distance == 32); + else + REQUIRE(distance == 20); + + uint8_t * byte_where_ptr = reinterpret_cast(where_ptr); + uint8_t * adjusted_byte_where_ptr = byte_where_ptr - distance; + + header->prev = reinterpret_cast(adjusted_byte_where_ptr); + header->next = reinterpret_cast(what_ptr); + // Corruption done + + // Verify state + REQUIRE(where == nullptr); + + Z_Free(ptr); + + // Check if successful + REQUIRE(where != nullptr); + REQUIRE(*where == 0x42424242); +} +#endif \ No newline at end of file diff --git a/test/tests.cpp b/test/cpp_doom/tests.cpp similarity index 100% rename from test/tests.cpp rename to test/cpp_doom/tests.cpp diff --git a/test/dehacked/CMakeLists.txt b/test/dehacked/CMakeLists.txt new file mode 100644 index 000000000..7719e7460 --- /dev/null +++ b/test/dehacked/CMakeLists.txt @@ -0,0 +1,8 @@ +file(GLOB_RECURSE sources CONFIGURE_DEPENDS "*.cpp") + +add_executable(test_dehacked ${sources}) +target_link_libraries(test_dehacked Catch2::Catch2 lib_common_dehacked) +target_compile_definitions(test_dehacked PUBLIC CATCH_CONFIG_CONSOLE_WIDTH=300) +target_include_directories(test_dehacked PRIVATE ${CMAKE_SOURCE_DIR}/src) + +add_test(NAME test_dehacked COMMAND test_dehacked) diff --git a/test/dehacked/test_z_zone.cpp b/test/dehacked/test_z_zone.cpp new file mode 100644 index 000000000..4ccaad667 --- /dev/null +++ b/test/dehacked/test_z_zone.cpp @@ -0,0 +1,38 @@ +#include + +#include "z_zone.hpp" + +TEST_CASE("Z_Init", "[z_zone]") { + Z_Init(); + REQUIRE(Z_ZoneSize() == 0x2000000); +} + +TEST_CASE("Z_Malloc(PU_STATIC)", "[z_zone]") { + void * ptr = Z_Malloc(10, PU_STATIC, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL)", "[z_zone]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVSPEC)", "[z_zone]") { + void * ptr = Z_Malloc(10, PU_LEVSPEC, nullptr); + REQUIRE(ptr != nullptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free", "[z_zone]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + Z_Free(ptr); +} + +TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_zone]") { + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + Z_Free(ptr); + void * ptr2 = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr2 != nullptr); + REQUIRE(ptr == ptr2); +} diff --git a/test/dehacked/tests.cpp b/test/dehacked/tests.cpp new file mode 100644 index 000000000..4ed06df1f --- /dev/null +++ b/test/dehacked/tests.cpp @@ -0,0 +1,2 @@ +#define CATCH_CONFIG_MAIN +#include From 3c343fe2fec9f66edc157f26af5bd6c50102ca1b Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 05:09:03 +0100 Subject: [PATCH 2/9] Classic Vuln --- test/CMakeLists.txt | 2 +- test/cpp_doom/test_z_native.cpp | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 85f236e0c..fedf0a0fc 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,4 +1,4 @@ find_package(Catch2 REQUIRED) add_subdirectory(cpp_doom) -add_subdirectory(dehacked) \ No newline at end of file +add_subdirectory(dehacked) diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp index 28745bb46..eca52f947 100644 --- a/test/cpp_doom/test_z_native.cpp +++ b/test/cpp_doom/test_z_native.cpp @@ -4,11 +4,8 @@ TEST_CASE("Z_Init", "[z_native]") { Z_Init(); -// REQUIRE(Z_ZoneSize() == 0x2000000); } -#if 0 - TEST_CASE("Z_Malloc(PU_STATIC)", "[z_native]") { void * ptr = Z_Malloc(10, PU_STATIC, nullptr); REQUIRE(ptr != nullptr); @@ -39,8 +36,6 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") REQUIRE(ptr == ptr2); } -#else - struct memblock_t { int id; // = ZONEID int tag; @@ -96,4 +91,3 @@ TEST_CASE("Z_ZOverwrite header", "[z_native]") { REQUIRE(where != nullptr); REQUIRE(*where == 0x42424242); } -#endif \ No newline at end of file From ade213a1fb2e5b76fac2110a824a36e8e0273e6e Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 05:53:55 +0100 Subject: [PATCH 3/9] Classic Vuln --- test/cpp_doom/test_z_native.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp index eca52f947..e9823304d 100644 --- a/test/cpp_doom/test_z_native.cpp +++ b/test/cpp_doom/test_z_native.cpp @@ -33,7 +33,6 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") Z_Free(ptr); void * ptr2 = Z_Malloc(10, PU_LEVEL, nullptr); REQUIRE(ptr2 != nullptr); - REQUIRE(ptr == ptr2); } struct memblock_t { From dcd6ae2e0965ef2d84cbdc676f70eeea0180c635 Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 05:55:46 +0100 Subject: [PATCH 4/9] Classic Vuln --- test/cpp_doom/test_z_native.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp index e9823304d..0b9a70792 100644 --- a/test/cpp_doom/test_z_native.cpp +++ b/test/cpp_doom/test_z_native.cpp @@ -35,6 +35,7 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") REQUIRE(ptr2 != nullptr); } +#ifndef __SANITIZE_ADDRESS__ struct memblock_t { int id; // = ZONEID int tag; @@ -90,3 +91,4 @@ TEST_CASE("Z_ZOverwrite header", "[z_native]") { REQUIRE(where != nullptr); REQUIRE(*where == 0x42424242); } +#endif // !__SANITIZE_ADDRESS__ From 4319157faf96f1b0fb742beb375a0ac7e8c0f859 Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 06:45:44 +0100 Subject: [PATCH 5/9] Classic Vuln --- test/cpp_doom/test_z_native.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp index 0b9a70792..13539a0f8 100644 --- a/test/cpp_doom/test_z_native.cpp +++ b/test/cpp_doom/test_z_native.cpp @@ -35,6 +35,14 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") REQUIRE(ptr2 != nullptr); } +#if defined(__has_feature) +# if __has_feature(address_sanitizer) +# ifndef __SANITIZE_ADDRESS__ +# define __SANITIZE_ADDRESS__ +# endif +# endif +#endif + #ifndef __SANITIZE_ADDRESS__ struct memblock_t { int id; // = ZONEID From 8fba0268e34b1558abdb1f2974ccbf79a3f3dff3 Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Tue, 22 Mar 2022 07:01:22 +0100 Subject: [PATCH 6/9] Classic Vuln --- test/cpp_doom/test_z_native.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/cpp_doom/test_z_native.cpp b/test/cpp_doom/test_z_native.cpp index 13539a0f8..0e8fc2b60 100644 --- a/test/cpp_doom/test_z_native.cpp +++ b/test/cpp_doom/test_z_native.cpp @@ -35,13 +35,14 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_native]") REQUIRE(ptr2 != nullptr); } +// Some compilers don't define __SANITIZE_ADDRESS__ #if defined(__has_feature) # if __has_feature(address_sanitizer) -# ifndef __SANITIZE_ADDRESS__ -# define __SANITIZE_ADDRESS__ -# endif -# endif -#endif +# if !defined(__SANITIZE_ADDRESS__) +# define __SANITIZE_ADDRESS__ // NOLINT +# endif // !defined(__SANITIZE_ADDRESS__) +# endif // __has_feature(address_sanitizer) +#endif // __has_feature #ifndef __SANITIZE_ADDRESS__ struct memblock_t { From d5c4125aaa63029be802d5f519a6d09fa7fda46f Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Sat, 26 Mar 2022 21:51:19 +0100 Subject: [PATCH 7/9] Enable more format string warnings --- src/doom/g_game.cpp | 3 +++ src/i_sdlsound.cpp | 2 +- src/i_system.cpp | 2 +- textscreen/txt_window.cpp | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/doom/g_game.cpp b/src/doom/g_game.cpp index 0325cd56a..b584e8374 100644 --- a/src/doom/g_game.cpp +++ b/src/doom/g_game.cpp @@ -2369,7 +2369,10 @@ void G_DoPlayDemo() { } // [crispy] make non-fatal else { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" fmt::fprintf(stderr, message, demoversion, G_VanillaVersionCode(), DemoVersionDescription(demoversion)); +#pragma GCC diagnostic pop fmt::fprintf(stderr, "\n"); g_doomstat_globals->demoplayback = true; G_CheckDemoStatus(); diff --git a/src/i_sdlsound.cpp b/src/i_sdlsound.cpp index ffaedf5d1..9e7e27186 100644 --- a/src/i_sdlsound.cpp +++ b/src/i_sdlsound.cpp @@ -462,7 +462,7 @@ static bool ExpandSoundData_SRC(sfxinfo_t * sfxinfo, } if (clipped > 0) { - fmt::fprintf(stderr, "Sound '%s': clipped %u samples (%0.2f %%)\n", sfxinfo->name, clipped, 400.0 * clipped / chunk->alen); + fmt::fprintf(stderr, "Sound '%s': clipped %u samples (%0.2f %%)\n", sfxinfo->name.c_str(), clipped, 400.0 * clipped / chunk->alen); } return true; diff --git a/src/i_system.cpp b/src/i_system.cpp index 912c19e95..34cf14d7f 100644 --- a/src/i_system.cpp +++ b/src/i_system.cpp @@ -134,7 +134,7 @@ uint8_t * I_ZoneBase(int * size) { i *= 2; fmt::printf("zone memory: %p, %d MiB allocated for zone\n", - zonemem, + reinterpret_cast(zonemem), *size >> 20); // [crispy] human-understandable zone heap size return zonemem; diff --git a/textscreen/txt_window.cpp b/textscreen/txt_window.cpp index fd97f5dfb..e85cb3ded 100644 --- a/textscreen/txt_window.cpp +++ b/textscreen/txt_window.cpp @@ -490,7 +490,7 @@ txt_window_t * TXT_MessageBox(const char * title, cstring_view message, ...) { va_list args; va_start(args, message); - TXT_vsnprintf(buf, sizeof(buf), message, args); + TXT_vsnprintf(buf, sizeof(buf), message.c_str(), args); va_end(args); txt_window_t * window = TXT_NewWindow(title); From 356bd9e9cb5826c68200ecc4c773e9d0211c4085 Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Sun, 27 Mar 2022 00:29:46 +0100 Subject: [PATCH 8/9] Make a PoC for z_zone too --- test/dehacked/test_z_zone.cpp | 62 +++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/test/dehacked/test_z_zone.cpp b/test/dehacked/test_z_zone.cpp index 4ccaad667..5d52fa947 100644 --- a/test/dehacked/test_z_zone.cpp +++ b/test/dehacked/test_z_zone.cpp @@ -36,3 +36,65 @@ TEST_CASE("Z_Malloc(PU_LEVEL) and Z_Free and Z_Malloc(PU_LEVEL)", "[z_zone]") { REQUIRE(ptr2 != nullptr); REQUIRE(ptr == ptr2); } + +// Some compilers don't define __SANITIZE_ADDRESS__ +#if defined(__has_feature) +# if __has_feature(address_sanitizer) +# if !defined(__SANITIZE_ADDRESS__) +# define __SANITIZE_ADDRESS__ // NOLINT +# endif // !defined(__SANITIZE_ADDRESS__) +# endif // __has_feature(address_sanitizer) +#endif // __has_feature + +#ifndef __SANITIZE_ADDRESS__ +struct memblock_t { + int size; // including the header and possibly tiny fragments + void ** user; + int tag; // PU_FREE if this is free + int id; // should be ZONEID + struct memblock_t * next; + struct memblock_t * prev; +}; + +long what; + +memblock_t fake{ + .size = 0, + .user = nullptr, + .tag = PU_FREE, + .id = 0, + .next = nullptr, // where + .prev = nullptr +}; + +TEST_CASE("Z_ZOverwrite header", "[z_zone]") { + + void * guard = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(guard != nullptr); + + void * ptr = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(ptr != nullptr); + + void * guard2 = Z_Malloc(10, PU_LEVEL, nullptr); + REQUIRE(guard2 != nullptr); + + what = 0x42424242; + + // Corrupt header + auto * byte_ptr = reinterpret_cast(ptr); + auto * header = reinterpret_cast(byte_ptr - sizeof(memblock_t)); + REQUIRE(header->tag == PU_LEVEL); + header->prev = &fake; + header->next = reinterpret_cast(&what); + // Corruption done + + // Verify state + REQUIRE(header->prev->next == nullptr); + + Z_Free(ptr); + + // Check if successful + REQUIRE(header->prev->next != nullptr); + REQUIRE(*(reinterpret_cast(header->prev->next)) == 0x42424242); +} +#endif From 17b9f5c2a8965459988b5ff52347aab18c8a2d6a Mon Sep 17 00:00:00 2001 From: Patricia Aas Date: Mon, 28 Mar 2022 13:00:03 +0200 Subject: [PATCH 9/9] Add Very Verbose to CTest --- .github/workflows/cmake.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 1624bddf7..e097492d6 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -100,7 +100,7 @@ jobs: shell: bash # Execute tests defined by the CMake configuration. # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail - run: ctest -C ${{ matrix.cmake-build-type }} + run: ctest -C ${{ matrix.cmake-build-type }} -VV env: CTEST_OUTPUT_ON_FAILURE: 1