From 4b2d60a2c39600d1e2a6c6523577ee26ec00c3bc Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Fri, 27 Jun 2025 15:16:35 +0200 Subject: [PATCH] Ghost: Make special user directory query thread safe Similar to e0ff7731e0. Noticed a data race when working on blender/blender!130543, which calls this function from a thread. Make this thread safe by avoiding returning of static memory, instead use an optional `std::string`. Pull Request: https://projects.blender.org/blender/blender/pulls/141083 --- intern/ghost/GHOST_ISystemPaths.hh | 7 +++-- intern/ghost/GHOST_Path-api.hh | 7 +++-- intern/ghost/intern/GHOST_Path-api.cc | 2 +- intern/ghost/intern/GHOST_SystemPathsCocoa.hh | 7 +++-- intern/ghost/intern/GHOST_SystemPathsCocoa.mm | 15 ++++++++--- intern/ghost/intern/GHOST_SystemPathsUnix.cc | 20 +++++++------- intern/ghost/intern/GHOST_SystemPathsUnix.hh | 7 +++-- intern/ghost/intern/GHOST_SystemPathsWin32.cc | 9 +++++-- intern/ghost/intern/GHOST_SystemPathsWin32.hh | 7 +++-- source/blender/blenkernel/intern/appdir.cc | 27 ++++++++++++------- 10 files changed, 71 insertions(+), 37 deletions(-) diff --git a/intern/ghost/GHOST_ISystemPaths.hh b/intern/ghost/GHOST_ISystemPaths.hh index ca2e3ce83d8..80c2f1acb03 100644 --- a/intern/ghost/GHOST_ISystemPaths.hh +++ b/intern/ghost/GHOST_ISystemPaths.hh @@ -8,6 +8,9 @@ #pragma once +#include +#include + #include "GHOST_Types.h" class GHOST_ISystemPaths { @@ -60,9 +63,9 @@ class GHOST_ISystemPaths { /** * Determine a special ("well known") and easy to reach user directory. - * \return Unsigned char string pointing to user directory (eg `~/Documents/`). + * \return If successfull, a string containing the user directory path (eg `~/Documents/`). */ - virtual const char *getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const = 0; + virtual std::optional getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const = 0; /** * Determine the directory of the current binary diff --git a/intern/ghost/GHOST_Path-api.hh b/intern/ghost/GHOST_Path-api.hh index 76a56719ebc..378bc4dfaad 100644 --- a/intern/ghost/GHOST_Path-api.hh +++ b/intern/ghost/GHOST_Path-api.hh @@ -8,6 +8,9 @@ #pragma once +#include +#include + #include "GHOST_Types.h" GHOST_DECLARE_HANDLE(GHOST_SystemPathsHandle); @@ -46,9 +49,9 @@ extern const char *GHOST_getUserDir(int version, const char *versionstr); /** * Determine a special ("well known") and easy to reach user directory. - * \return Unsigned char string pointing to user directory (eg `~/Documents/`). + * \return If successfull, a string containing the user directory path (eg `~/Documents/`). */ -extern const char *GHOST_getUserSpecialDir(GHOST_TUserSpecialDirTypes type); +extern std::optional GHOST_getUserSpecialDir(GHOST_TUserSpecialDirTypes type); /** * Determine the directory in which the binary file is found. diff --git a/intern/ghost/intern/GHOST_Path-api.cc b/intern/ghost/intern/GHOST_Path-api.cc index 86d86c7b36c..d16a3ce2071 100644 --- a/intern/ghost/intern/GHOST_Path-api.cc +++ b/intern/ghost/intern/GHOST_Path-api.cc @@ -34,7 +34,7 @@ const char *GHOST_getUserDir(int version, const char *versionstr) return systemPaths ? systemPaths->getUserDir(version, versionstr) : nullptr; } -const char *GHOST_getUserSpecialDir(GHOST_TUserSpecialDirTypes type) +std::optional GHOST_getUserSpecialDir(GHOST_TUserSpecialDirTypes type) { const GHOST_ISystemPaths *systemPaths = GHOST_ISystemPaths::get(); /* Shouldn't be `nullptr`. */ diff --git a/intern/ghost/intern/GHOST_SystemPathsCocoa.hh b/intern/ghost/intern/GHOST_SystemPathsCocoa.hh index d8472872388..336939ffe98 100644 --- a/intern/ghost/intern/GHOST_SystemPathsCocoa.hh +++ b/intern/ghost/intern/GHOST_SystemPathsCocoa.hh @@ -12,6 +12,9 @@ # error Apple OSX only! #endif // __APPLE__ +#include +#include + #include "GHOST_SystemPaths.hh" class GHOST_SystemPathsCocoa : public GHOST_SystemPaths { @@ -42,9 +45,9 @@ class GHOST_SystemPathsCocoa : public GHOST_SystemPaths { /** * Determine a special ("well known") and easy to reach user directory. - * \return Unsigned char string pointing to user directory (eg `~/Documents/`). + * \return If successfull, a string containing the user directory path (eg `~/Documents/`). */ - const char *getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; + std::optional getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; /** * Determine the directory of the current binary. diff --git a/intern/ghost/intern/GHOST_SystemPathsCocoa.mm b/intern/ghost/intern/GHOST_SystemPathsCocoa.mm index 6063fc50a24..762e0726b79 100644 --- a/intern/ghost/intern/GHOST_SystemPathsCocoa.mm +++ b/intern/ghost/intern/GHOST_SystemPathsCocoa.mm @@ -5,6 +5,9 @@ #import #import +#include +#include + #include "GHOST_Debug.hh" #include "GHOST_SystemPathsCocoa.hh" @@ -46,9 +49,10 @@ const char *GHOST_SystemPathsCocoa::getUserDir(int /* version */, const char *ve return GetApplicationSupportDir(versionstr, NSUserDomainMask, tempPath, sizeof(tempPath)); } -const char *GHOST_SystemPathsCocoa::getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const +std::optional GHOST_SystemPathsCocoa::getUserSpecialDir( + GHOST_TUserSpecialDirTypes type) const { - static char tempPath[512] = ""; + char tempPath[512] = ""; @autoreleasepool { NSSearchPathDirectory ns_directory; @@ -78,12 +82,12 @@ const char *GHOST_SystemPathsCocoa::getUserSpecialDir(GHOST_TUserSpecialDirTypes GHOST_ASSERT( false, "GHOST_SystemPathsCocoa::getUserSpecialDir(): Invalid enum value for type parameter"); - return nullptr; + return std::nullopt; } NSArray *paths = NSSearchPathForDirectoriesInDomains(ns_directory, NSUserDomainMask, YES); if (paths.count == 0) { - return nullptr; + return std::nullopt; } NSString *basePath = [paths objectAtIndex:0]; @@ -94,6 +98,9 @@ const char *GHOST_SystemPathsCocoa::getUserSpecialDir(GHOST_TUserSpecialDirTypes memcpy(tempPath, basePath_cstr, basePath_len); tempPath[basePath_len] = '\0'; } + if (!tempPath[0]) { + return std::nullopt; + } return tempPath; } diff --git a/intern/ghost/intern/GHOST_SystemPathsUnix.cc b/intern/ghost/intern/GHOST_SystemPathsUnix.cc index a044e1f99d7..d6b07521491 100644 --- a/intern/ghost/intern/GHOST_SystemPathsUnix.cc +++ b/intern/ghost/intern/GHOST_SystemPathsUnix.cc @@ -6,6 +6,7 @@ * \ingroup GHOST */ +#include #include #include "GHOST_SystemPathsUnix.hh" @@ -100,10 +101,10 @@ const char *GHOST_SystemPathsUnix::getUserDir(int version, const char *versionst return user_path.c_str(); } -const char *GHOST_SystemPathsUnix::getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const +std::optional GHOST_SystemPathsUnix::getUserSpecialDir( + GHOST_TUserSpecialDirTypes type) const { const char *type_str; - static string path; switch (type) { case GHOST_kUserSpecialDirDesktop: @@ -133,16 +134,15 @@ const char *GHOST_SystemPathsUnix::getUserSpecialDir(GHOST_TUserSpecialDirTypes /* If `XDG_CACHE_HOME` is not set, then `$HOME/.cache is used`. */ const char *home_dir = home_dir_get(); if (home_dir == nullptr) { - return nullptr; + return std::nullopt; } - path = string(home_dir) + "/.cache"; - return path.c_str(); + return string(home_dir) + "/.cache"; } default: GHOST_ASSERT( false, "GHOST_SystemPathsUnix::getUserSpecialDir(): Invalid enum value for type parameter"); - return nullptr; + return std::nullopt; } /* Pipe `stderr` to `/dev/null` to avoid error prints. We will fail gracefully still. */ @@ -150,7 +150,7 @@ const char *GHOST_SystemPathsUnix::getUserSpecialDir(GHOST_TUserSpecialDirTypes FILE *fstream = popen(command.c_str(), "r"); if (fstream == nullptr) { - return nullptr; + return std::nullopt; } std::stringstream path_stream; while (!feof(fstream)) { @@ -163,11 +163,11 @@ const char *GHOST_SystemPathsUnix::getUserSpecialDir(GHOST_TUserSpecialDirTypes } if (pclose(fstream) == -1) { perror("GHOST_SystemPathsUnix::getUserSpecialDir failed at pclose()"); - return nullptr; + return std::nullopt; } - path = path_stream.str(); - return path[0] ? path.c_str() : nullptr; + std::string path = path_stream.str(); + return path[0] ? std::optional(path) : std::nullopt; } const char *GHOST_SystemPathsUnix::getBinaryDir() const diff --git a/intern/ghost/intern/GHOST_SystemPathsUnix.hh b/intern/ghost/intern/GHOST_SystemPathsUnix.hh index 33a481bef51..31a3f52ccc6 100644 --- a/intern/ghost/intern/GHOST_SystemPathsUnix.hh +++ b/intern/ghost/intern/GHOST_SystemPathsUnix.hh @@ -8,6 +8,9 @@ #pragma once +#include +#include + #include "../GHOST_Types.h" #include "GHOST_SystemPaths.hh" @@ -40,9 +43,9 @@ class GHOST_SystemPathsUnix : public GHOST_SystemPaths { /** * Determine a special ("well known") and easy to reach user directory. - * \return Unsigned char string pointing to user directory (eg `~/Documents/`). + * \return If successfull, a string containing the user directory path (eg `~/Documents/`). */ - const char *getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; + std::optional getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; /** * Determine the directory of the current binary. diff --git a/intern/ghost/intern/GHOST_SystemPathsWin32.cc b/intern/ghost/intern/GHOST_SystemPathsWin32.cc index 489af1d87e6..71d6d45ace5 100644 --- a/intern/ghost/intern/GHOST_SystemPathsWin32.cc +++ b/intern/ghost/intern/GHOST_SystemPathsWin32.cc @@ -62,7 +62,8 @@ const char *GHOST_SystemPathsWin32::getUserDir(int, const char *versionstr) cons return user_dir; } -const char *GHOST_SystemPathsWin32::getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const +std::optional GHOST_SystemPathsWin32::getUserSpecialDir( + GHOST_TUserSpecialDirTypes type) const { const char *special_dir = nullptr; @@ -93,7 +94,7 @@ const char *GHOST_SystemPathsWin32::getUserSpecialDir(GHOST_TUserSpecialDirTypes GHOST_ASSERT( false, "GHOST_SystemPathsWin32::getUserSpecialDir(): Invalid enum value for type parameter"); - return nullptr; + return std::nullopt; } static char knownpath[MAX_PATH * 3] = {0}; @@ -105,6 +106,10 @@ const char *GHOST_SystemPathsWin32::getUserSpecialDir(GHOST_TUserSpecialDirTypes special_dir = knownpath; } + if ((special_dir == nullptr) || (special_dir[0] == '\0')) { + return std::nullopt; + } + CoTaskMemFree(knownpath_16); return special_dir; } diff --git a/intern/ghost/intern/GHOST_SystemPathsWin32.hh b/intern/ghost/intern/GHOST_SystemPathsWin32.hh index 6de8e2a0b77..33a74be0ebd 100644 --- a/intern/ghost/intern/GHOST_SystemPathsWin32.hh +++ b/intern/ghost/intern/GHOST_SystemPathsWin32.hh @@ -8,6 +8,9 @@ #pragma once +#include +#include + #ifndef WIN32 # error WIN32 only! #endif // WIN32 @@ -49,9 +52,9 @@ class GHOST_SystemPathsWin32 : public GHOST_SystemPaths { /** * Determine a special ("well known") and easy to reach user directory. - * \return Unsigned char string pointing to user directory (eg `~/Documents/`). + * \return If successfull, a string containing the user directory path (eg `~/Documents/`). */ - const char *getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; + std::optional getUserSpecialDir(GHOST_TUserSpecialDirTypes type) const override; /** * Determine the directory of the current binary. diff --git a/source/blender/blenkernel/intern/appdir.cc b/source/blender/blenkernel/intern/appdir.cc index 807b73a5993..210f5c8d658 100644 --- a/source/blender/blenkernel/intern/appdir.cc +++ b/source/blender/blenkernel/intern/appdir.cc @@ -168,11 +168,12 @@ bool BKE_appdir_folder_documents(char *dir) { dir[0] = '\0'; - const char *documents_path = GHOST_getUserSpecialDir(GHOST_kUserSpecialDirDocuments); + const std::optional documents_path = GHOST_getUserSpecialDir( + GHOST_kUserSpecialDirDocuments); /* Usual case: Ghost gave us the documents path. We're done here. */ - if (documents_path && BLI_is_dir(documents_path)) { - BLI_strncpy(dir, documents_path, FILE_MAXDIR); + if (documents_path && BLI_is_dir(documents_path->c_str())) { + BLI_strncpy(dir, documents_path->c_str(), FILE_MAXDIR); return true; } @@ -198,21 +199,27 @@ bool BKE_appdir_folder_caches(char *path, const size_t path_maxncpy) { path[0] = '\0'; - const char *caches_root_path = GHOST_getUserSpecialDir(GHOST_kUserSpecialDirCaches); - if (caches_root_path == nullptr || !BLI_is_dir(caches_root_path)) { + std::optional caches_root_path = GHOST_getUserSpecialDir( + GHOST_kUserSpecialDirCaches); + if (!caches_root_path || !BLI_is_dir(caches_root_path->c_str())) { caches_root_path = BKE_tempdir_base(); } - if (caches_root_path == nullptr || !BLI_is_dir(caches_root_path)) { + if (!caches_root_path || !BLI_is_dir(caches_root_path->c_str())) { return false; } #ifdef WIN32 - BLI_path_join( - path, path_maxncpy, caches_root_path, "Blender Foundation", "Blender", "Cache", SEP_STR); + BLI_path_join(path, + path_maxncpy, + caches_root_path->c_str(), + "Blender Foundation", + "Blender", + "Cache", + SEP_STR); #elif defined(__APPLE__) - BLI_path_join(path, path_maxncpy, caches_root_path, "Blender", SEP_STR); + BLI_path_join(path, path_maxncpy, caches_root_path->c_str(), "Blender", SEP_STR); #else /* __linux__ */ - BLI_path_join(path, path_maxncpy, caches_root_path, "blender", SEP_STR); + BLI_path_join(path, path_maxncpy, caches_root_path->c_str(), "blender", SEP_STR); #endif return true;