From 5a409dbe2cee5f3184e4116179114c52fe8e1932 Mon Sep 17 00:00:00 2001 From: Flyinghead Date: Wed, 4 Jan 2023 12:09:09 +0100 Subject: [PATCH] boxart: [] operator with non-existing key on const json is UB Use json::at() instead. Don't return pointer to GameBoxart to UI since it can be replaced/deleted by the scraper thread. Tentative fix for MINIDUMP-16 --- core/rend/boxart/boxart.cpp | 30 +++--- core/rend/boxart/boxart.h | 2 +- core/rend/boxart/gamesdb.cpp | 196 ++++++++++++++++++++--------------- core/rend/boxart/scraper.h | 8 +- core/rend/gui.cpp | 33 +++--- 5 files changed, 140 insertions(+), 129 deletions(-) diff --git a/core/rend/boxart/boxart.cpp b/core/rend/boxart/boxart.cpp index 52f6727b5..09f9a95f9 100644 --- a/core/rend/boxart/boxart.cpp +++ b/core/rend/boxart/boxart.cpp @@ -24,41 +24,39 @@ static std::string getGameFileName(const std::string& path) { size_t slash = get_last_slash_pos(path); - std::string fileName; if (slash != std::string::npos) return path.substr(slash + 1); else return path; } -const GameBoxart *Boxart::getBoxart(const GameMedia& media) +GameBoxart Boxart::getBoxart(const GameMedia& media) { loadDatabase(); std::string fileName = getGameFileName(media.path); - GameBoxart *boxart = nullptr; + GameBoxart boxart; { std::lock_guard guard(mutex); auto it = games.find(fileName); if (it != games.end()) { - boxart = &it->second; - if (config::FetchBoxart && !boxart->busy && !boxart->scraped) + boxart = it->second; + if (config::FetchBoxart && !boxart.busy && !boxart.scraped) { - boxart->busy = true; - boxart->gamePath = media.path; - toFetch.push_back(*boxart); + boxart.busy = it->second.busy = true; + boxart.gamePath = media.path; + toFetch.push_back(boxart); } } else { - GameBoxart box; - box.fileName = fileName; - box.gamePath = media.path; - box.name = media.name; - box.searchName = media.gameName; // for arcade games - box.busy = true; - games[box.fileName] = box; - toFetch.push_back(box); + boxart.fileName = fileName; + boxart.gamePath = media.path; + boxart.name = media.name; + boxart.searchName = media.gameName; // for arcade games + boxart.busy = true; + games[boxart.fileName] = boxart; + toFetch.push_back(boxart); } } fetchBoxart(); diff --git a/core/rend/boxart/boxart.h b/core/rend/boxart/boxart.h index 0f7d18247..8b9b99101 100644 --- a/core/rend/boxart/boxart.h +++ b/core/rend/boxart/boxart.h @@ -29,7 +29,7 @@ struct GameMedia; class Boxart { public: - const GameBoxart *getBoxart(const GameMedia& media); + GameBoxart getBoxart(const GameMedia& media); void saveDatabase(); private: diff --git a/core/rend/boxart/gamesdb.cpp b/core/rend/boxart/gamesdb.cpp index cf1a56c69..741e48981 100644 --- a/core/rend/boxart/gamesdb.cpp +++ b/core/rend/boxart/gamesdb.cpp @@ -101,7 +101,7 @@ json TheGamesDb::httpGet(const std::string& url) // TODO can this happen? http status should be the same std::string status; try { - status = v["status"]; + status = v.at("status"); } catch (const json::exception& e) { } throw std::runtime_error(std::string("TheGamesDB error ") + std::to_string(code) + ": " + status); @@ -120,15 +120,21 @@ void TheGamesDb::fetchPlatforms() std::string url = makeUrl("Platforms/ByPlatformName") + "&name=" + platform; json v = httpGet(url); - const json& array = v["data"]["platforms"]; + try { + const json& array = v.at("data").at("platforms"); - for (const auto& o : array) - { - std::string name = o["name"]; - if (name == "Sega Dreamcast") - dreamcastPlatformId = o["id"]; - else if (name == "Arcade") - arcadePlatformId = o["id"]; + for (const auto& o : array) + { + try { + std::string name = o.at("name"); + if (name == "Sega Dreamcast") + dreamcastPlatformId = o.at("id"); + else if (name == "Arcade") + arcadePlatformId = o.at("id"); + } catch (const json::exception& e) { + } + } + } catch (const json::exception& e) { } }; getPlatformId("Dreamcast"); @@ -141,89 +147,98 @@ void TheGamesDb::parseBoxart(GameBoxart& item, const json& j, int gameId) { std::string baseUrl; try { - baseUrl = j["base_url"]["thumb"]; + baseUrl = j.at("base_url").at("thumb"); } catch (const json::exception& e) { try { - baseUrl = j["base_url"]["small"]; + baseUrl = j.at("base_url").at("small"); } catch (const json::exception& e) { return; } } - const json& images = j.contains("data") ? j["data"] : j["images"]; - if (images.is_array()) + if (!j.contains("data") && !j.contains("images")) // No boxart return; - json dataArray = images[std::to_string(gameId)]; - std::string imagePath; - for (const auto& o : dataArray) - { - try { - // Look for boxart - if (o["type"] != "boxart") - continue; - } catch (const json::exception& e) { - continue; - } - try { - // We want the front side if specified - if (o["side"] == "back") - continue; - } catch (const json::exception& e) { - // ignore if not found - } - imagePath = o["filename"].get(); - break; - } - if (imagePath.empty()) - { - // Use titlescreen + const json& images = j.contains("data") ? j.at("data") : j.at("images"); + + try { + const json& dataArray = images.at(std::to_string(gameId)); + std::string imagePath; for (const auto& o : dataArray) { try { - if (o["type"] != "titlescreen") + // Look for boxart + if (o.at("type") != "boxart") continue; } catch (const json::exception& e) { continue; } - imagePath = o["filename"].get(); - break; - } - } - if (imagePath.empty()) - { - // Use screenshot - for (const auto& o : dataArray) - { try { - if (o["type"] != "screenshot") + // We want the front side if specified + if (o.at("side") == "back") continue; } catch (const json::exception& e) { - continue; + // ignore if not found + } + try { + imagePath = o.at("filename").get(); + break; + } catch (const json::exception& e) { + // continue if not found } - imagePath = o["filename"].get(); - break; } - } - if (!imagePath.empty()) - { - // Build the full URL and get from cache or download - std::string url = baseUrl + imagePath; - std::string filename = makeUniqueFilename("dummy.jpg"); // thegamesdb returns some images as png, but they are really jpeg - auto cached = boxartCache.find(url); - if (cached != boxartCache.end()) + if (imagePath.empty()) { - copyFile(cached->second, filename); - item.setBoxartPath(filename); - } - else - { - if (downloadImage(url, filename)) + // Use titlescreen + for (const auto& o : dataArray) { - item.setBoxartPath(filename); - boxartCache[url] = filename; + try { + if (o.at("type") != "titlescreen") + continue; + imagePath = o.at("filename").get(); + break; + } catch (const json::exception& e) { + // continue if not found + } } } + if (imagePath.empty()) + { + // Use screenshot + for (const auto& o : dataArray) + { + try { + if (o.at("type") != "screenshot") + continue; + imagePath = o.at("filename").get(); + break; + } catch (const json::exception& e) { + // continue if not found + } + } + } + if (!imagePath.empty()) + { + // Build the full URL and get from cache or download + std::string url = baseUrl + imagePath; + std::string filename = makeUniqueFilename("dummy.jpg"); // thegamesdb returns some images as png, but they are really jpeg + auto cached = boxartCache.find(url); + if (cached != boxartCache.end()) + { + copyFile(cached->second, filename); + item.setBoxartPath(filename); + } + else + { + if (downloadImage(url, filename)) + { + item.setBoxartPath(filename); + boxartCache[url] = filename; + } + } + } + } catch (const json::exception& e) { + // No boxart for this game } } @@ -235,8 +250,8 @@ bool TheGamesDb::parseGameInfo(const json& gameArray, const json& boxartArray, G { bool found = false; try { - for (const auto& uid : game["uids"]) - if (uid["uid"] == diskId) { + for (const auto& uid : game.at("uids")) + if (uid.at("uid") == diskId) { found = true; break; } @@ -246,24 +261,27 @@ bool TheGamesDb::parseGameInfo(const json& gameArray, const json& boxartArray, G continue; } // Name - item.name = game["game_title"]; + try { + item.name = game.at("game_title"); + } catch (const json::exception& e) { + } // Release date try { - item.releaseDate = game["release_date"]; + item.releaseDate = game.at("release_date"); } catch (const json::exception& e) { } // Overview try { - item.overview = game["overview"]; + item.overview = game.at("overview"); } catch (const json::exception& e) { } // GameDB id int id; try { - id = game["id"]; + id = game.at("id"); } catch (const json::exception& e) { return true; } @@ -275,7 +293,10 @@ bool TheGamesDb::parseGameInfo(const json& gameArray, const json& boxartArray, G { std::string imgUrl = makeUrl("Games/Images") + "&games_id=" + std::to_string(id); json images = httpGet(imgUrl); - parseBoxart(item, images["data"], id); + try { + parseBoxart(item, images.at("data"), id); + } catch (const json::exception& e) { + } } return true; } @@ -285,11 +306,13 @@ bool TheGamesDb::parseGameInfo(const json& gameArray, const json& boxartArray, G bool TheGamesDb::fetchGameInfo(GameBoxart& item, const std::string& url, const std::string& diskId) { json v = httpGet(url); - json& array = v["data"]["games"]; - if (array.empty()) + try { + const json& array = v.at("data").at("games"); + const json& boxart = v.at("include").at("boxart"); + return parseGameInfo(array, boxart, item, diskId); + } catch (const json::exception& e) { return false; - - return parseGameInfo(array, v["include"]["boxart"], item, diskId); + } } void TheGamesDb::scrape(GameBoxart& item) @@ -343,14 +366,15 @@ void TheGamesDb::fetchByUids(std::vector& items) std::string url = makeUrl("Games/ByGameUniqueID") + "&fields=overview,uids&include=boxart&filter%5Bplatform%5D=" + std::to_string(dreamcastPlatformId) + "&uid=" + http::urlEncode(uidCriteria); json v = httpGet(url); - json& array = v["data"]["games"]; - if (array.empty()) - return; - json& boxartArray = v["include"]["boxart"]; - for (GameBoxart& item : items) - { - if (!item.scraped && !item.uniqueId.empty() && parseGameInfo(array, boxartArray, item, item.uniqueId)) - item.scraped = true; + try { + const json& array = v.at("data").at("games"); + const json& boxartArray = v.at("include").at("boxart"); + for (GameBoxart& item : items) + { + if (!item.scraped && !item.uniqueId.empty() && parseGameInfo(array, boxartArray, item, item.uniqueId)) + item.scraped = true; + } + } catch (const json::exception& e) { } } diff --git a/core/rend/boxart/scraper.h b/core/rend/boxart/scraper.h index 9c5201ca7..8ba3667df 100644 --- a/core/rend/boxart/scraper.h +++ b/core/rend/boxart/scraper.h @@ -66,14 +66,10 @@ struct GameBoxart static void loadProperty(T& i, const json& j, const std::string& propName) { try { - // asan error if missing contains(). json bug? - if (j.contains(propName)) { - i = j[propName].get(); - return; - } + i = j.at(propName).get(); } catch (const json::exception& e) { + i = T(); } - i = T(); } GameBoxart() = default; diff --git a/core/rend/gui.cpp b/core/rend/gui.cpp index fa042afb5..4bf46c4e2 100644 --- a/core/rend/gui.cpp +++ b/core/rend/gui.cpp @@ -2328,22 +2328,22 @@ static void gameTooltip(const std::string& tip) } } -static bool getGameImage(const GameBoxart *art, ImTextureID& textureId, bool allowLoad) +static bool getGameImage(const GameBoxart& art, ImTextureID& textureId, bool allowLoad) { textureId = ImTextureID{}; - if (art->boxartPath.empty()) + if (art.boxartPath.empty()) return false; // Get the boxart texture. Load it if needed. - textureId = imguiDriver->getTexture(art->boxartPath); + textureId = imguiDriver->getTexture(art.boxartPath); if (textureId == ImTextureID() && allowLoad) { int width, height; - u8 *imgData = loadImage(art->boxartPath, width, height); + u8 *imgData = loadImage(art.boxartPath, width, height); if (imgData != nullptr) { try { - textureId = imguiDriver->updateTextureAndAspectRatio(art->boxartPath, imgData, width, height); + textureId = imguiDriver->updateTextureAndAspectRatio(art.boxartPath, imgData, width, height); } catch (...) { // vulkan can throw during resizing } @@ -2435,12 +2435,9 @@ static void gui_display_content() { ImTextureID textureId{}; GameMedia game; - const GameBoxart *art = boxart.getBoxart(game); - if (art != nullptr) - { - if (getGameImage(art, textureId, loadedImages < 10)) - loadedImages++; - } + GameBoxart art = boxart.getBoxart(game); + if (getGameImage(art, textureId, loadedImages < 10)) + loadedImages++; if (textureId != ImTextureID()) pressed = gameImageButton(textureId, "Dreamcast BIOS", responsiveBoxVec2); else @@ -2468,12 +2465,11 @@ static void gui_display_content() continue; } std::string gameName = game.name; - const GameBoxart *art = nullptr; + GameBoxart art; if (config::BoxartDisplayMode) { art = boxart.getBoxart(game); - if (art != nullptr) - gameName = art->name; + gameName = art.name; } if (filter.PassFilter(gameName.c_str())) { @@ -2485,12 +2481,9 @@ static void gui_display_content() ImGui::SameLine(); counter++; ImTextureID textureId{}; - if (art != nullptr) - { - // Get the boxart texture. Load it if needed (max 10 per frame). - if (getGameImage(art, textureId, loadedImages < 10)) - loadedImages++; - } + // Get the boxart texture. Load it if needed (max 10 per frame). + if (getGameImage(art, textureId, loadedImages < 10)) + loadedImages++; if (textureId != ImTextureID()) pressed = gameImageButton(textureId, game.name, responsiveBoxVec2); else