From 9269def8ec89caed916990b21048d4338f1f9438 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 29 Aug 2017 16:52:06 -0700 Subject: [PATCH] (maint) Fix permissions hacks Applying permissions on Windows and using fs::permissions on Solaris were both hacked around due to a faulty assumption that they were broken on those platforms. The root cause of both issues was a static initialization ordering issue. Fix it by using a macro to alias the variable name, rather than assigning from a global variable in another object. Unwind Windows and Solaris hacks. --- lib/inc/pxp-agent/configuration.hpp | 4 +--- lib/src/configuration.cc | 8 ------- lib/src/external_module.cc | 4 ---- lib/src/modules/task.cc | 36 ++--------------------------- lib/src/results_storage.cc | 6 ----- 5 files changed, 3 insertions(+), 55 deletions(-) diff --git a/lib/inc/pxp-agent/configuration.hpp b/lib/inc/pxp-agent/configuration.hpp index 2ebf9cd6..f300b737 100644 --- a/lib/inc/pxp-agent/configuration.hpp +++ b/lib/inc/pxp-agent/configuration.hpp @@ -24,11 +24,9 @@ namespace PXPAgent { extern const std::string DEFAULT_SPOOL_DIR; // used by unit tests -#ifndef _WIN32 -// Not used on Windows. We instead rely on inherited directory ACLs. +// Note that on Windows we rely on inherited directory ACLs. extern const boost::filesystem::perms NIX_FILE_PERMS; extern const boost::filesystem::perms NIX_DIR_PERMS; -#endif // // Types diff --git a/lib/src/configuration.cc b/lib/src/configuration.cc index a54897bf..c818dde9 100644 --- a/lib/src/configuration.cc +++ b/lib/src/configuration.cc @@ -104,10 +104,8 @@ static const std::string DEFAULT_SPOOL_DIR_PURGE_TTL { "14d" }; static const std::string AGENT_CLIENT_TYPE { "agent" }; -#ifndef _WIN32 const fs::perms NIX_FILE_PERMS { fs::owner_read | fs::owner_write | fs::group_read }; const fs::perms NIX_DIR_PERMS { NIX_FILE_PERMS | fs::owner_exe | fs::group_exe }; -#endif // // Public interface @@ -228,9 +226,7 @@ std::string Configuration::setupLogging() // up logging before calling validateAndNormalizeConfiguration validateLogDirPath(logfile_); logfile_fstream_.open(logfile_.c_str(), std::ios_base::app); -#ifndef _WIN32 fs::permissions(logfile_, NIX_FILE_PERMS); -#endif log_stream = &logfile_fstream_; } else { @@ -243,9 +239,7 @@ std::string Configuration::setupLogging() pcp_access_fstream_ptr_.reset( new boost::nowide::ofstream(pcp_access_logfile_.c_str(), std::ios_base::app)); -#ifndef _WIN32 fs::permissions(pcp_access_logfile_, NIX_FILE_PERMS); -#endif } #ifndef _WIN32 @@ -936,7 +930,6 @@ void Configuration::validateAndNormalizeOtherSettings() fs::path task_cache_dir_path { lth_file::tilde_expand(task_cache_dir) }; check_and_create_dir(task_cache_dir_path, "task-cache-dir", true); -#ifndef _WIN32 try { fs::permissions(task_cache_dir_path, NIX_DIR_PERMS); } catch (const fs::filesystem_error& e) { @@ -944,7 +937,6 @@ void Configuration::validateAndNormalizeOtherSettings() lth_loc::format("Failed to make the task-cache-dir '{1}' user/group readable and executable, and user writable during configuration validation: {2}", task_cache_dir_path.string(), e.what()) }; } -#endif HW::SetFlag("task-cache-dir", task_cache_dir_path.string()); diff --git a/lib/src/external_module.cc b/lib/src/external_module.cc index 3bb6c7a9..74610997 100644 --- a/lib/src/external_module.cc +++ b/lib/src/external_module.cc @@ -362,12 +362,8 @@ ActionResponse ExternalModule::callNonBlockingAction(const ActionRequest& reques std::map(), // environment [results_dir_path](size_t pid) { auto pid_file = (results_dir_path / "pid").string(); -#ifdef _WIN32 - lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file); -#else lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file, NIX_FILE_PERMS, std::ios::binary); -#endif }, // pid callback 0, // timeout { lth_exec::execution_options::thread_safe, diff --git a/lib/src/modules/task.cc b/lib/src/modules/task.cc index 9f7cd619..0288f068 100644 --- a/lib/src/modules/task.cc +++ b/lib/src/modules/task.cc @@ -20,10 +20,6 @@ #include -#ifdef __sun -#include -#endif - namespace PXPAgent { namespace Modules { @@ -164,9 +160,8 @@ static Module::ProcessingError toModuleProcessingError(const fs::filesystem_erro } } -#ifndef _WIN32 -static const fs::perms NIX_TASK_FILE_PERMS = NIX_DIR_PERMS; -#endif +// A define is used here because creating a local variable is subject to static initialization ordering. +#define NIX_TASK_FILE_PERMS NIX_DIR_PERMS // Creates the / directory for a single // task, ensuring that its permissions are readable by @@ -178,9 +173,7 @@ static fs::path createCacheDir(const fs::path& task_cache_dir, const std::string auto cache_dir = task_cache_dir / sha256; fs::create_directory(cache_dir); fs::last_write_time(cache_dir, time(nullptr)); -#ifndef _WIN32 fs::permissions(cache_dir, NIX_DIR_PERMS); -#endif return cache_dir; } @@ -261,19 +254,7 @@ static std::tuple downloadTaskFile(const std::vector& master_uris, auto filepath = cache_dir / filename; if (fs::exists(filepath) && sha256 == calculateSha256(filepath.string())) { -#ifdef __sun - // fs::permissions removes all permissions on Solaris. Use chmod directly as a workaround. - auto err = chmod(filepath.c_str(), S_IRWXU | S_IRGRP | S_IXGRP); - if (err) { - throw fs::filesystem_error("Failed to update mode of + " + filepath.string(), - boost::system::error_code(errno, boost::system::system_category())); - } -#elif !defined(_WIN32) fs::permissions(filepath, NIX_TASK_FILE_PERMS); -#endif return filepath; } @@ -416,12 +388,8 @@ void Task::callNonBlockingAction( environment, [results_dir](size_t pid) { auto pid_file = (results_dir / "pid").string(); -#ifdef _WIN32 - lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file); -#else lth_file::atomic_write_to_file(std::to_string(pid) + "\n", pid_file, NIX_FILE_PERMS, std::ios::binary); -#endif }, // pid callback 0, // timeout leatherman::util::option_set { diff --git a/lib/src/results_storage.cc b/lib/src/results_storage.cc index 61cd7451..c69732f4 100644 --- a/lib/src/results_storage.cc +++ b/lib/src/results_storage.cc @@ -42,11 +42,7 @@ bool ResultsStorage::find(const std::string& transaction_id) static void writeMetadata(const std::string& txt, const std::string& file_path) { try { -#ifdef _WIN32 - lth_file::atomic_write_to_file(txt, file_path); -#else lth_file::atomic_write_to_file(txt, file_path, NIX_FILE_PERMS, std::ios::binary); -#endif } catch (const std::exception& e) { throw ResultsStorage::Error { lth_loc::format("failed to write metadata: {1}", e.what()) }; @@ -63,9 +59,7 @@ void ResultsStorage::initializeMetadataFile(const std::string& transaction_id, transaction_id, results_path.string()); try { fs::create_directories(results_path); -#ifndef _WIN32 fs::permissions(results_path, NIX_DIR_PERMS); -#endif } catch (const fs::filesystem_error& e) { throw ResultsStorage::Error { lth_loc::format("failed to create results directory '{1}'",