From a28c7ba5407a4b02a1cc9fc35ad2297bf6b4a138 Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Fri, 25 Jun 2021 15:06:10 +0200 Subject: [PATCH] Improve parsing of umask values --- cmake/config.h.in | 7 ++++++ doc/MANUAL.adoc | 6 +++--- src/Config.cpp | 33 ++++++++++++----------------- src/Config.hpp | 6 +++--- src/Context.cpp | 4 ++-- src/UmaskScope.hpp | 6 +++--- src/ccache.cpp | 2 +- src/system.hpp | 5 ----- src/util/string_utils.cpp | 11 ++++++++++ src/util/string_utils.hpp | 4 ++++ unittest/test_Config.cpp | 6 +++--- unittest/test_util_string_utils.cpp | 15 +++++++++++++ 12 files changed, 65 insertions(+), 40 deletions(-) diff --git a/cmake/config.h.in b/cmake/config.h.in index fe99e16d8..6014ca611 100644 --- a/cmake/config.h.in +++ b/cmake/config.h.in @@ -179,3 +179,10 @@ # undef HAVE_STRUCT_STAT_ST_CTIM # undef HAVE_STRUCT_STAT_ST_MTIM #endif + +#ifdef _WIN32 +# ifdef _MSC_VER +typedef unsigned __int32 mode_t; +typedef int pid_t; +# endif +#endif // _WIN32 diff --git a/doc/MANUAL.adoc b/doc/MANUAL.adoc index 2856d68dd..4948dfd04 100644 --- a/doc/MANUAL.adoc +++ b/doc/MANUAL.adoc @@ -868,9 +868,9 @@ filesystem as the *CCACHE_DIR* path, but this requirement has been relaxed.) [[config_umask]] *umask* (*CCACHE_UMASK*):: - This option specifies the umask for files and directories in the cache - directory. This is mostly useful when you wish to share your cache with - other users. + This option (an octal integer) specifies the umask for files and directories + in the cache directory. This is mostly useful when you wish to share your + cache with other users. == Cache size management diff --git a/src/Config.cpp b/src/Config.cpp index def12f232..4bd947dfb 100644 --- a/src/Config.cpp +++ b/src/Config.cpp @@ -27,6 +27,8 @@ #include "exceptions.hpp" #include "fmtmacros.hpp" +#include + #include "third_party/fmt/core.h" // System headers @@ -333,28 +335,13 @@ format_sloppiness(uint32_t sloppiness) return result; } -uint32_t -parse_umask(const std::string& value) -{ - if (value.empty()) { - return std::numeric_limits::max(); - } - - size_t end; - uint32_t result = std::stoul(value, &end, 8); - if (end != value.size()) { - throw Error("not an octal integer: \"{}\"", value); - } - return result; -} - std::string -format_umask(uint32_t umask) +format_umask(nonstd::optional umask) { - if (umask == std::numeric_limits::max()) { - return {}; + if (umask) { + return FMT("{:03o}", *umask); } else { - return FMT("{:03o}", umask); + return {}; } } @@ -982,7 +969,13 @@ Config::set_item(const std::string& key, break; case ConfigItem::umask: - m_umask = parse_umask(value); + if (!value.empty()) { + const auto umask = util::parse_umask(value); + if (!umask) { + throw Error(umask.error()); + } + m_umask = *umask; + } break; } diff --git a/src/Config.hpp b/src/Config.hpp index e64afff42..a4ea78913 100644 --- a/src/Config.hpp +++ b/src/Config.hpp @@ -81,7 +81,7 @@ public: bool stats() const; const std::string& stats_log() const; const std::string& temporary_dir() const; - uint32_t umask() const; + nonstd::optional umask() const; void set_base_dir(const std::string& value); void set_cache_dir(const std::string& value); @@ -174,7 +174,7 @@ private: bool m_stats = true; std::string m_stats_log; std::string m_temporary_dir; - uint32_t m_umask = std::numeric_limits::max(); // Don't set umask + nonstd::optional m_umask; bool m_temporary_dir_configured_explicitly = false; @@ -417,7 +417,7 @@ Config::temporary_dir() const return m_temporary_dir; } -inline uint32_t +inline nonstd::optional Config::umask() const { return m_umask; diff --git a/src/Context.cpp b/src/Context.cpp index b5a24e3ec..27b404813 100644 --- a/src/Context.cpp +++ b/src/Context.cpp @@ -52,8 +52,8 @@ Context::Context() // initial configuration file. The intention is that all files and directories // in the cache directory should be affected by the configured umask and that // no other files and directories should. - if (config.umask() != std::numeric_limits::max()) { - original_umask = umask(config.umask()); + if (config.umask()) { + original_umask = umask(*config.umask()); } } diff --git a/src/UmaskScope.hpp b/src/UmaskScope.hpp index d96f448e2..a641c1f35 100644 --- a/src/UmaskScope.hpp +++ b/src/UmaskScope.hpp @@ -1,4 +1,4 @@ -// Copyright (C) 2020 Joel Rosdahl and other contributors +// Copyright (C) 2020-2021 Joel Rosdahl and other contributors // // See doc/AUTHORS.adoc for a complete list of contributors. // @@ -34,7 +34,7 @@ private: nonstd::optional m_saved_umask; }; -UmaskScope::UmaskScope(nonstd::optional new_umask) +inline UmaskScope::UmaskScope(nonstd::optional new_umask) { #ifndef _WIN32 if (new_umask) { @@ -45,7 +45,7 @@ UmaskScope::UmaskScope(nonstd::optional new_umask) #endif } -UmaskScope::~UmaskScope() +inline UmaskScope::~UmaskScope() { #ifndef _WIN32 if (m_saved_umask) { diff --git a/src/ccache.cpp b/src/ccache.cpp index 7528a2503..7c30e5642 100644 --- a/src/ccache.cpp +++ b/src/ccache.cpp @@ -1952,7 +1952,7 @@ cache_compilation(int argc, const char* const* argv) bool fall_back_to_original_compiler = false; Args saved_orig_args; - nonstd::optional original_umask; + nonstd::optional original_umask; std::string saved_temp_dir; { diff --git a/src/system.hpp b/src/system.hpp index b58119e35..be8234308 100644 --- a/src/system.hpp +++ b/src/system.hpp @@ -91,11 +91,6 @@ const size_t READ_BUFFER_SIZE = 65536; # error _WIN32_WINNT is undefined # endif -# ifdef _MSC_VER -typedef int mode_t; -typedef int pid_t; -# endif - # ifndef __MINGW32__ typedef int64_t ssize_t; # endif diff --git a/src/util/string_utils.cpp b/src/util/string_utils.cpp index e6ff892a4..cd15ddcc0 100644 --- a/src/util/string_utils.cpp +++ b/src/util/string_utils.cpp @@ -19,6 +19,7 @@ #include "string_utils.hpp" #include +#include #include // System headers @@ -27,6 +28,16 @@ namespace util { +nonstd::expected +parse_umask(const std::string& value) +{ + try { + return Util::parse_unsigned(value, 0, 0777, "umask", 8); + } catch (const Error& e) { + return nonstd::make_unexpected(e.what()); + } +} + nonstd::expected percent_decode(nonstd::string_view string) { diff --git a/src/util/string_utils.hpp b/src/util/string_utils.hpp index 7f23b0fe4..41685798e 100644 --- a/src/util/string_utils.hpp +++ b/src/util/string_utils.hpp @@ -23,11 +23,15 @@ #include // System headers +#include #include // End of system headers namespace util { +// Parse `value` (an octal integer). +nonstd::expected parse_umask(const std::string& value); + // Percent-decode[1] `string`. // // [1]: https://en.wikipedia.org/wiki/Percent-encoding diff --git a/unittest/test_Config.cpp b/unittest/test_Config.cpp index 669f56381..8530032db 100644 --- a/unittest/test_Config.cpp +++ b/unittest/test_Config.cpp @@ -76,7 +76,7 @@ TEST_CASE("Config: default values") CHECK(config.sloppiness() == 0); CHECK(config.stats()); CHECK(config.temporary_dir().empty()); // Set later - CHECK(config.umask() == std::numeric_limits::max()); + CHECK(config.umask() == nonstd::nullopt); } TEST_CASE("Config::update_from_file") @@ -175,7 +175,7 @@ TEST_CASE("Config::update_from_file") | SLOPPY_IVFSOVERLAY)); CHECK_FALSE(config.stats()); CHECK(config.temporary_dir() == FMT("{}_foo", user)); - CHECK(config.umask() == 0777); + CHECK(config.umask() == 0777u); } TEST_CASE("Config::update_from_file, error handling") @@ -221,7 +221,7 @@ TEST_CASE("Config::update_from_file, error handling") { Util::write_file("ccache.conf", "umask = "); CHECK(config.update_from_file("ccache.conf")); - CHECK(config.umask() == std::numeric_limits::max()); + CHECK(config.umask() == nonstd::nullopt); } SUBCASE("invalid size") diff --git a/unittest/test_util_string_utils.cpp b/unittest/test_util_string_utils.cpp index 5df0159b9..c70659867 100644 --- a/unittest/test_util_string_utils.cpp +++ b/unittest/test_util_string_utils.cpp @@ -28,6 +28,21 @@ operator==( return left.first == right.first && left.second == right.second; } +TEST_CASE("util::parse_umask") +{ + CHECK(util::parse_umask("1") == 01u); + CHECK(util::parse_umask("002") == 2u); + CHECK(util::parse_umask("777") == 0777u); + CHECK(util::parse_umask("0777") == 0777u); + + CHECK(util::parse_umask("").error() + == "invalid unsigned octal integer: \"\""); + CHECK(util::parse_umask(" ").error() + == "invalid unsigned octal integer: \"\""); + CHECK(util::parse_umask("088").error() + == "invalid unsigned octal integer: \"088\""); +} + TEST_CASE("util::percent_decode") { CHECK(util::percent_decode("") == ""); -- 2.47.3