From bbd69496b4d4d64c7135cdb2b7adba4d3db5b000 Mon Sep 17 00:00:00 2001 From: Tom Lankhorst Date: Sun, 3 Feb 2019 22:59:28 +0100 Subject: [PATCH 1/4] Sanitize screenshot path --- distribution/changelog.txt | 1 + src/openrct2/interface/Screenshot.cpp | 152 ++++++++++++++------------ src/openrct2/platform/Shared.cpp | 19 ++++ src/openrct2/platform/Windows.cpp | 18 +++ src/openrct2/platform/platform.h | 1 + 5 files changed, 119 insertions(+), 72 deletions(-) diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 42da6c34a4..7cbd17abbe 100644 --- a/distribution/changelog.txt +++ b/distribution/changelog.txt @@ -116,6 +116,7 @@ - Fix: [#8588] Guest list scrolling breaks above ~2000 guests. - Fix: [#8591] Game loop does not run at a consistent tick rate of 40 Hz. - Fix: [#8647] Marketing campaigns check for entry fees below £1 (original bug). +- Fix: [#8598] Sanitize screenshot parknames. - Fix: [#8653] Crash when peeps attempt to enter a ride with no vehicles. - Fix: [#8720] Desync due to boats colliding with ghost pieces. - Fix: [#8736] Incomplete warning when all ride slots are full. diff --git a/src/openrct2/interface/Screenshot.cpp b/src/openrct2/interface/Screenshot.cpp index b64b5989bb..18b49c75d1 100644 --- a/src/openrct2/interface/Screenshot.cpp +++ b/src/openrct2/interface/Screenshot.cpp @@ -17,6 +17,7 @@ #include "../audio/audio.h" #include "../core/Console.hpp" #include "../core/Imaging.h" +#include "../core/Optional.hpp" #include "../drawing/Drawing.h" #include "../drawing/X8DrawingEngine.h" #include "../localisation/Localisation.h" @@ -28,10 +29,13 @@ #include "../world/Surface.h" #include "Viewport.h" +#include #include #include #include +#include +using namespace std::literals::string_literals; using namespace OpenRCT2; using namespace OpenRCT2::Drawing; @@ -96,88 +100,93 @@ static void screenshot_get_rendered_palette(rct_palette* palette) } } -static int32_t screenshot_get_next_path(char* path, size_t size) +static std::string screenshot_get_park_name() +{ + char buffer[512]; + format_string(buffer, sizeof(buffer), gParkName, &gParkNameArgs); + return buffer; +} + +static opt::optional screenshot_get_directory() { char screenshotPath[MAX_PATH]; - platform_get_user_directory(screenshotPath, "screenshot", sizeof(screenshotPath)); - if (!platform_ensure_directory_exists(screenshotPath)) + + if (platform_ensure_directory_exists(screenshotPath)) { - log_error("Unable to save screenshots in OpenRCT2 screenshot directory.\n"); - return -1; + return opt::make_optional(screenshotPath); } - char park_name[128]; - format_string(park_name, 128, gParkName, &gParkNameArgs); + log_error("Unable to save screenshots in OpenRCT2 screenshot directory.\n"); + return opt::nullopt; +} - // Retrieve current time - rct2_date currentDate; - platform_get_date_local(¤tDate); - rct2_time currentTime; - platform_get_time_local(¤tTime); +static std::tuple screenshot_get_date_time() +{ + rct2_date date; + platform_get_date_local(&date); -#ifdef _WIN32 - // On NTFS filesystems, a colon (:) in a path - // indicates you want to write a file stream - // (hidden metadata). This will pass the - // file_exists and fopen checks, since it is - // technically valid. We don't want that, so - // replace colons with hyphens in the park name. - char* foundColon = park_name; - while ((foundColon = strchr(foundColon, ':')) != nullptr) - { - *foundColon = '-'; - } -#endif + rct2_time time; + platform_get_time_local(&time); - // Glue together path and filename - safe_strcpy(path, screenshotPath, size); - path_end_with_separator(path, size); - auto fileNameCh = strchr(path, '\0'); - if (fileNameCh == nullptr) - { - log_error("Unable to generate a screenshot filename."); - return -1; - } - const size_t leftBytes = size - strlen(path); + return { date, time }; +} + +static std::string screenshot_get_formatted_date_time() +{ + auto [date, time] = screenshot_get_date_time(); + char formatted[64]; snprintf( - fileNameCh, leftBytes, "%s %d-%02d-%02d %02d-%02d-%02d.png", park_name, currentDate.year, currentDate.month, - currentDate.day, currentTime.hour, currentTime.minute, currentTime.second); + formatted, sizeof(formatted), "%4d-%02d-%02d %02d-%02d-%02d", date.year, date.month, date.day, time.hour, time.minute, + time.second); + return formatted; +} - if (!platform_file_exists(path)) +static opt::optional screenshot_get_next_path() +{ + std::string dir, name, suffix = ".png", path; + + auto screenshotDirectory = screenshot_get_directory(); + + if (screenshotDirectory == opt::nullopt) { - return 0; // path ok + return opt::nullopt; } - // multiple screenshots with same timestamp - // might be possible when switching timezones - // in the unlikely case that this does happen, - // append (%d) to the filename and increment - // this int32_t until it doesn't overwrite any - // other file in the directory. - int32_t i; - for (i = 1; i < 1000; i++) - { - // Glue together path and filename - snprintf( - fileNameCh, leftBytes, "%s %d-%02d-%02d %02d-%02d-%02d (%d).png", park_name, currentDate.year, currentDate.month, - currentDate.day, currentTime.hour, currentTime.minute, currentTime.second, i); + auto parkName = screenshot_get_park_name(); + auto dateTime = screenshot_get_formatted_date_time(); - if (!platform_file_exists(path)) - { - return i; - } + dir = *screenshotDirectory; + name = parkName + " " + dateTime; + + // Generate a path with a `tries` number + auto path_composer = [&dir, &name, &suffix ](int tries) -> auto + { + auto composed_filename = platform_sanitise_filename( + name + ((tries > 0) ? " ("s + std::to_string(tries) + ")" : ""s) + suffix); + return dir + PATH_SEPARATOR + composed_filename; + }; + + for (int tries = 0; tries < 100; tries++) + { + path = path_composer(tries); + if (platform_file_exists(path.c_str())) + continue; + + return path; } log_error("You have too many saved screenshots saved at exactly the same date and time.\n"); - return -1; -} + + return opt::nullopt; +}; std::string screenshot_dump_png(rct_drawpixelinfo* dpi) { // Get a free screenshot path - char path[MAX_PATH] = ""; - if (screenshot_get_next_path(path, MAX_PATH) == -1) + auto path = screenshot_get_next_path(); + + if (path == opt::nullopt) { return ""; } @@ -185,9 +194,9 @@ std::string screenshot_dump_png(rct_drawpixelinfo* dpi) rct_palette renderedPalette; screenshot_get_rendered_palette(&renderedPalette); - if (WriteDpiToFile(path, dpi, renderedPalette)) + if (WriteDpiToFile(path->c_str(), dpi, renderedPalette)) { - return std::string(path); + return *path; } else { @@ -197,9 +206,9 @@ std::string screenshot_dump_png(rct_drawpixelinfo* dpi) std::string screenshot_dump_png_32bpp(int32_t width, int32_t height, const void* pixels) { - // Get a free screenshot path - char path[MAX_PATH] = ""; - if (screenshot_get_next_path(path, MAX_PATH) == -1) + auto path = screenshot_get_next_path(); + + if (path == opt::nullopt) { return ""; } @@ -215,8 +224,8 @@ std::string screenshot_dump_png_32bpp(int32_t width, int32_t height, const void* image.Depth = 32; image.Stride = width * 4; image.Pixels = std::vector(pixels8, pixels8 + pixelsLen); - Imaging::WriteToFile(path, image, IMAGE_FORMAT::PNG_32); - return std::string(path); + Imaging::WriteToFile(path->c_str(), image, IMAGE_FORMAT::PNG_32); + return *path; } catch (const std::exception& e) { @@ -301,9 +310,8 @@ void screenshot_giant() viewport_render(&dpi, &viewport, 0, 0, viewport.width, viewport.height); - // Get a free screenshot path - char path[MAX_PATH]; - if (screenshot_get_next_path(path, MAX_PATH) == -1) + auto path = screenshot_get_next_path(); + if (path == opt::nullopt) { log_error("Giant screenshot failed, unable to find a suitable destination path."); context_show_error(STR_SCREENSHOT_FAILED, STR_NONE); @@ -313,13 +321,13 @@ void screenshot_giant() rct_palette renderedPalette; screenshot_get_rendered_palette(&renderedPalette); - WriteDpiToFile(path, &dpi, renderedPalette); + WriteDpiToFile(path->c_str(), &dpi, renderedPalette); free(dpi.bits); // Show user that screenshot saved successfully set_format_arg(0, rct_string_id, STR_STRING); - set_format_arg(2, char*, path_get_filename(path)); + set_format_arg(2, char*, path_get_filename(path->c_str())); context_show_error(STR_SCREENSHOT_SAVED_AS, STR_NONE); } diff --git a/src/openrct2/platform/Shared.cpp b/src/openrct2/platform/Shared.cpp index 6cc2ec2688..050cb2ac13 100644 --- a/src/openrct2/platform/Shared.cpp +++ b/src/openrct2/platform/Shared.cpp @@ -28,6 +28,7 @@ #include "../world/Climate.h" #include "platform.h" +#include #include #include @@ -198,6 +199,24 @@ uint8_t platform_get_currency_value(const char* currCode) return CURRENCY_POUNDS; } +#ifndef _WIN32 +std::string platform_sanitise_filename(const std::string& path) +{ + auto sanitised = path; + + std::vector prohibited = { '/' }; + + std::replace_if( + sanitised.begin(), sanitised.end(), + [&prohibited](const std::string::value_type& ch) { + return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); + }, + '_'); + + return sanitised; +} +#endif + #ifndef __ANDROID__ float platform_get_default_scale() { diff --git a/src/openrct2/platform/Windows.cpp b/src/openrct2/platform/Windows.cpp index 57c1ca7f64..029014dbf4 100644 --- a/src/openrct2/platform/Windows.cpp +++ b/src/openrct2/platform/Windows.cpp @@ -28,8 +28,10 @@ # include "../util/Util.h" # include "platform.h" +# include # include # include +# include # include # include # include @@ -253,6 +255,22 @@ std::string platform_get_rct2_steam_dir() return "Rollercoaster Tycoon 2"; } +std::string platform_sanitise_filename(const std::string& path) +{ + auto sanitised = path; + + std::vector prohibited = { '<', '>', '*', '\\', ':', '|', '?', '"', '/' }; + + std::replace_if( + sanitised.begin(), sanitised.end(), + [](const std::string::value_type& ch) { + return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); + }, + '_'); + + return sanitised; +} + uint16_t platform_get_locale_language() { CHAR langCode[4]; diff --git a/src/openrct2/platform/platform.h b/src/openrct2/platform/platform.h index 7e2ec421fd..bad0ad63eb 100644 --- a/src/openrct2/platform/platform.h +++ b/src/openrct2/platform/platform.h @@ -126,6 +126,7 @@ bool platform_process_is_elevated(); bool platform_get_steam_path(utf8* outPath, size_t outSize); std::string platform_get_rct1_steam_dir(); std::string platform_get_rct2_steam_dir(); +std::string platform_sanitise_filename(const std::string&); #ifndef NO_TTF bool platform_get_font_path(TTFFontDescriptor* font, utf8* buffer, size_t size); From 744f2225ed0ff203ef918aac56209dfb8eb5735c Mon Sep 17 00:00:00 2001 From: Tom Lankhorst Date: Sat, 2 Mar 2019 14:40:24 +0100 Subject: [PATCH 2/4] Write platform tests --- src/openrct2/platform/Windows.cpp | 3 +++ test/tests/CMakeLists.txt | 7 +++++++ test/tests/Platform.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+) create mode 100644 test/tests/Platform.cpp diff --git a/src/openrct2/platform/Windows.cpp b/src/openrct2/platform/Windows.cpp index 029014dbf4..9ebc13ffc9 100644 --- a/src/openrct2/platform/Windows.cpp +++ b/src/openrct2/platform/Windows.cpp @@ -22,6 +22,7 @@ # include "../OpenRCT2.h" # include "../Version.h" # include "../config/Config.h" +# include "../core/String.h" # include "../localisation/Date.h" # include "../localisation/Language.h" # include "../rct2/RCT2.h" @@ -268,6 +269,8 @@ std::string platform_sanitise_filename(const std::string& path) }, '_'); + sanitised = String::Trim(sanitised); + return sanitised; } diff --git a/test/tests/CMakeLists.txt b/test/tests/CMakeLists.txt index 842b6e7e54..72a485032d 100644 --- a/test/tests/CMakeLists.txt +++ b/test/tests/CMakeLists.txt @@ -144,6 +144,13 @@ target_link_libraries(test_ini ${GTEST_LIBRARIES} test-common ${LDL} z) target_link_platform_libraries(test_ini) add_test(NAME ini COMMAND test_ini) +# Platform +add_executable(test_platform ${CMAKE_CURRENT_LIST_DIR}/Platform.cpp) +SET_CHECK_CXX_FLAGS(test_platform) +target_link_libraries(test_platform ${GTEST_LIBRARIES} test-common ${LDL} z libopenrct2) +target_link_platform_libraries(test_platform) +add_test(NAME platform COMMAND test_platform) + # String test set(STRING_TEST_SOURCES "${CMAKE_CURRENT_LIST_DIR}/StringTest.cpp" diff --git a/test/tests/Platform.cpp b/test/tests/Platform.cpp new file mode 100644 index 0000000000..d8e345c052 --- /dev/null +++ b/test/tests/Platform.cpp @@ -0,0 +1,30 @@ +/***************************************************************************** + * Copyright (c) 2014-2019 OpenRCT2 developers + * + * For a complete list of all authors, please refer to contributors.md + * Interested in contributing? Visit https://github.com/OpenRCT2/OpenRCT2 + * + * OpenRCT2 is licensed under the GNU General Public License version 3. + *****************************************************************************/ + +#include + +#include + +TEST(platform, sanitise_filename) +{ +#ifndef _WIN32 + ASSERT_EQ("normal-filename.png", platform_sanitise_filename("normal-filename.png")); + ASSERT_EQ("utf🎱", platform_sanitise_filename("utf🎱")); + ASSERT_EQ("forbidden_char", platform_sanitise_filename("forbidden/char")); + ASSERT_EQ("forbidden_\\:\"|?*chars", platform_sanitise_filename("forbidden/\\:\"|?*chars")); + ASSERT_EQ(" non trimmed ", platform_sanitise_filename(" non trimmed ")); +#else + ASSERT_EQ("normal-filename.png", platform_sanitise_filename("normal-filename.png")); + ASSERT_EQ("utf🎱", platform_sanitise_filename("utf🎱")); + ASSERT_EQ("forbidden_char", platform_sanitise_filename("forbidden/char")); + ASSERT_EQ("forbidden_______chars", platform_sanitise_filename("forbidden/\\:\"|?*chars")); + ASSERT_EQ("non trimmed", platform_sanitise_filename(" non trimmed ")); +#endif +} + From ed353faccfb805386f9f0bfc81dc36c96a85c3c4 Mon Sep 17 00:00:00 2001 From: Tom Lankhorst Date: Sat, 2 Mar 2019 14:50:15 +0100 Subject: [PATCH 3/4] Implement replace_if condition lambda Fix include typo and CS --- src/openrct2/platform/Shared.cpp | 2 +- src/openrct2/platform/Windows.cpp | 4 ++-- test/tests/Platform.cpp | 4 +--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/openrct2/platform/Shared.cpp b/src/openrct2/platform/Shared.cpp index 050cb2ac13..4c099b39d9 100644 --- a/src/openrct2/platform/Shared.cpp +++ b/src/openrct2/platform/Shared.cpp @@ -208,7 +208,7 @@ std::string platform_sanitise_filename(const std::string& path) std::replace_if( sanitised.begin(), sanitised.end(), - [&prohibited](const std::string::value_type& ch) { + [&prohibited](const std::string::value_type& ch) -> bool { return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); }, '_'); diff --git a/src/openrct2/platform/Windows.cpp b/src/openrct2/platform/Windows.cpp index 9ebc13ffc9..2dda11b53e 100644 --- a/src/openrct2/platform/Windows.cpp +++ b/src/openrct2/platform/Windows.cpp @@ -22,7 +22,7 @@ # include "../OpenRCT2.h" # include "../Version.h" # include "../config/Config.h" -# include "../core/String.h" +# include "../core/String.hpp" # include "../localisation/Date.h" # include "../localisation/Language.h" # include "../rct2/RCT2.h" @@ -264,7 +264,7 @@ std::string platform_sanitise_filename(const std::string& path) std::replace_if( sanitised.begin(), sanitised.end(), - [](const std::string::value_type& ch) { + [&prohibited](const std::string::value_type& ch) -> bool { return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); }, '_'); diff --git a/test/tests/Platform.cpp b/test/tests/Platform.cpp index d8e345c052..dccea3aad0 100644 --- a/test/tests/Platform.cpp +++ b/test/tests/Platform.cpp @@ -7,9 +7,8 @@ * OpenRCT2 is licensed under the GNU General Public License version 3. *****************************************************************************/ -#include - #include +#include TEST(platform, sanitise_filename) { @@ -27,4 +26,3 @@ TEST(platform, sanitise_filename) ASSERT_EQ("non trimmed", platform_sanitise_filename(" non trimmed ")); #endif } - From 7074d6f3ae4c9059297a31e29da74412adf29aeb Mon Sep 17 00:00:00 2001 From: Ted John Date: Wed, 8 May 2019 22:03:17 +0100 Subject: [PATCH 4/4] Apply review suggestions --- distribution/changelog.txt | 4 +-- src/openrct2/interface/Screenshot.cpp | 47 ++++++++++----------------- src/openrct2/platform/Shared.cpp | 7 ++-- src/openrct2/platform/Windows.cpp | 8 ++--- 4 files changed, 23 insertions(+), 43 deletions(-) diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 7cbd17abbe..45167eac71 100644 --- a/distribution/changelog.txt +++ b/distribution/changelog.txt @@ -25,6 +25,7 @@ - Fix: [#8219] Faulty folder recreation in "save" folder. - Fix: [#8507] Incorrect change in vehicle rolling direction. - Fix: [#8537] Imported RCT1 rides/shops are all numbered 1. +- Fix: [#8598] Taking screenshots fails with some park names. - Fix: [#8649] Setting date does not work in multiplayer. - Fix: [#8873] Potential crash when placing footpaths. - Fix: [#8882] Submarine Ride does not count as indoors (original bug). @@ -115,8 +116,7 @@ - Fix: [#8585] Part of track missing on air powered vertical coaster. - Fix: [#8588] Guest list scrolling breaks above ~2000 guests. - Fix: [#8591] Game loop does not run at a consistent tick rate of 40 Hz. -- Fix: [#8647] Marketing campaigns check for entry fees below £1 (original bug). -- Fix: [#8598] Sanitize screenshot parknames. +- Fix: [#8647] Marketing campaigns check for entry fees below £1 (original bug). - Fix: [#8653] Crash when peeps attempt to enter a ride with no vehicles. - Fix: [#8720] Desync due to boats colliding with ghost pieces. - Fix: [#8736] Incomplete warning when all ride slots are full. diff --git a/src/openrct2/interface/Screenshot.cpp b/src/openrct2/interface/Screenshot.cpp index 18b49c75d1..ca46599eee 100644 --- a/src/openrct2/interface/Screenshot.cpp +++ b/src/openrct2/interface/Screenshot.cpp @@ -107,21 +107,14 @@ static std::string screenshot_get_park_name() return buffer; } -static opt::optional screenshot_get_directory() +static std::string screenshot_get_directory() { char screenshotPath[MAX_PATH]; platform_get_user_directory(screenshotPath, "screenshot", sizeof(screenshotPath)); - - if (platform_ensure_directory_exists(screenshotPath)) - { - return opt::make_optional(screenshotPath); - } - - log_error("Unable to save screenshots in OpenRCT2 screenshot directory.\n"); - return opt::nullopt; + return screenshotPath; } -static std::tuple screenshot_get_date_time() +static std::pair screenshot_get_date_time() { rct2_date date; platform_get_date_local(&date); @@ -144,41 +137,35 @@ static std::string screenshot_get_formatted_date_time() static opt::optional screenshot_get_next_path() { - std::string dir, name, suffix = ".png", path; - auto screenshotDirectory = screenshot_get_directory(); - - if (screenshotDirectory == opt::nullopt) + if (!platform_ensure_directory_exists(screenshotDirectory.c_str())) { - return opt::nullopt; + log_error("Unable to save screenshots in OpenRCT2 screenshot directory.\n"); + return {}; } auto parkName = screenshot_get_park_name(); auto dateTime = screenshot_get_formatted_date_time(); - - dir = *screenshotDirectory; - name = parkName + " " + dateTime; + auto name = parkName + " " + dateTime; // Generate a path with a `tries` number - auto path_composer = [&dir, &name, &suffix ](int tries) -> auto - { - auto composed_filename = platform_sanitise_filename( - name + ((tries > 0) ? " ("s + std::to_string(tries) + ")" : ""s) + suffix); - return dir + PATH_SEPARATOR + composed_filename; + auto pathComposer = [&screenshotDirectory, &name](int tries) { + auto composedFilename = platform_sanitise_filename( + name + ((tries > 0) ? " ("s + std::to_string(tries) + ")" : ""s) + ".png"); + return screenshotDirectory + PATH_SEPARATOR + composedFilename; }; for (int tries = 0; tries < 100; tries++) { - path = path_composer(tries); - if (platform_file_exists(path.c_str())) - continue; - - return path; + auto path = pathComposer(tries); + if (!platform_file_exists(path.c_str())) + { + return path; + } } log_error("You have too many saved screenshots saved at exactly the same date and time.\n"); - - return opt::nullopt; + return {}; }; std::string screenshot_dump_png(rct_drawpixelinfo* dpi) diff --git a/src/openrct2/platform/Shared.cpp b/src/openrct2/platform/Shared.cpp index 4c099b39d9..7672bf71d0 100644 --- a/src/openrct2/platform/Shared.cpp +++ b/src/openrct2/platform/Shared.cpp @@ -202,17 +202,14 @@ uint8_t platform_get_currency_value(const char* currCode) #ifndef _WIN32 std::string platform_sanitise_filename(const std::string& path) { + static const std::array prohibited = { '/' }; auto sanitised = path; - - std::vector prohibited = { '/' }; - std::replace_if( sanitised.begin(), sanitised.end(), - [&prohibited](const std::string::value_type& ch) -> bool { + [](const std::string::value_type& ch) -> bool { return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); }, '_'); - return sanitised; } #endif diff --git a/src/openrct2/platform/Windows.cpp b/src/openrct2/platform/Windows.cpp index 2dda11b53e..5167add70b 100644 --- a/src/openrct2/platform/Windows.cpp +++ b/src/openrct2/platform/Windows.cpp @@ -258,19 +258,15 @@ std::string platform_get_rct2_steam_dir() std::string platform_sanitise_filename(const std::string& path) { + static const std::array prohibited = { '<', '>', '*', '\\', ':', '|', '?', '"', '/' }; auto sanitised = path; - - std::vector prohibited = { '<', '>', '*', '\\', ':', '|', '?', '"', '/' }; - std::replace_if( sanitised.begin(), sanitised.end(), - [&prohibited](const std::string::value_type& ch) -> bool { + [](const std::string::value_type& ch) -> bool { return std::find(prohibited.begin(), prohibited.end(), ch) != prohibited.end(); }, '_'); - sanitised = String::Trim(sanitised); - return sanitised; }