Improve parsing of umask values

diff --git a/cmake/config.h.in b/cmake/config.h.in
index fe99e16d827318fe38449b7b6d38d8c3d0372475..6014ca6118c5988d5c291c674e69dcc2da57f77a 100644 (file)
--- 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 2856d68dd0d0495b6afb59f376fc8413432fbb0a..4948dfd0496a4abc1a44572f2e0ed87e103891c9 100644 (file)
--- 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 def12f232aaabf77be6f4615793e5ef930db43b5..4bd947dfb1179fe691742f82852f93453acdab34 100644 (file)
--- a/src/Config.cpp
+++ b/src/Config.cpp
@@ -27,6 +27,8 @@
 #include "exceptions.hpp"
 #include "fmtmacros.hpp"
 
+#include <util/string_utils.hpp>
+
 #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<uint32_t>::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<mode_t> umask)
 {
-  if (umask == std::numeric_limits<uint32_t>::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 e64afff42c5dd316ed021df36f1da37f12db9f88..a4ea78913731ddd34e7e84963c9da2731067659e 100644 (file)
--- 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<mode_t> 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<uint32_t>::max(); // Don't set umask
+  nonstd::optional<mode_t> 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<mode_t>
 Config::umask() const
 {
   return m_umask;

diff --git a/src/Context.cpp b/src/Context.cpp
index b5a24e3ecbbdbf318667fc102e21c45b1b75c678..27b4048139bd6ecd7ae29751a7b5f0c06923bdd8 100644 (file)
--- 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<uint32_t>::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 d96f448e23fce2862da78523bf447d846430888b..a641c1f35b377f5c0b6f9052662d17684b0c00ef 100644 (file)
--- 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<mode_t> m_saved_umask;
 };
 
-UmaskScope::UmaskScope(nonstd::optional<mode_t> new_umask)
+inline UmaskScope::UmaskScope(nonstd::optional<mode_t> new_umask)
 {
 #ifndef _WIN32
   if (new_umask) {
@@ -45,7 +45,7 @@ UmaskScope::UmaskScope(nonstd::optional<mode_t> 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 7528a250374aa839c42b775e5a3dfa0887a7d230..7c30e5642fe5dc2c0fbb879d1dcc713aa5490076 100644 (file)
--- 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<mode_t> original_umask;
+  nonstd::optional<uint32_t> original_umask;
   std::string saved_temp_dir;
 
   {

diff --git a/src/system.hpp b/src/system.hpp
index b58119e35834fe8ede35183b070adbfb0c11c683..be82343081ad4301cdbbcbff52e69eff26aa8e52 100644 (file)
--- 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 e6ff892a4a9e8b7204cd934c14e7240dd95e428f..cd15ddcc0dc40e3c99e6ecef64b5a026a4e65d53 100644 (file)
--- a/src/util/string_utils.cpp
+++ b/src/util/string_utils.cpp
@@ -19,6 +19,7 @@
 #include "string_utils.hpp"
 
 #include <FormatNonstdStringView.hpp>
+#include <Util.hpp>
 #include <fmtmacros.hpp>
 
 // System headers
@@ -27,6 +28,16 @@
 
 namespace util {
 
+nonstd::expected<mode_t, std::string>
+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<std::string, std::string>
 percent_decode(nonstd::string_view string)
 {

diff --git a/src/util/string_utils.hpp b/src/util/string_utils.hpp
index 7f23b0fe4bed6959e80569c73b28ffc1e9c957ae..41685798e53f76efc3f151bdb9d5642c7cc54bd1 100644 (file)
--- a/src/util/string_utils.hpp
+++ b/src/util/string_utils.hpp
@@ -23,11 +23,15 @@
 #include <third_party/nonstd/string_view.hpp>
 
 // System headers
+#include <string>
 #include <utility>
 // End of system headers
 
 namespace util {
 
+// Parse `value` (an octal integer).
+nonstd::expected<mode_t, std::string> 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 669f563819289a325b8d859a27c7275bc77401ae..8530032dbbfc44c1aa7a138a04ea5a74d0247a60 100644 (file)
--- 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<uint32_t>::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<uint32_t>::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 5df0159b908dc84447fb579bfa6df72c68c2b0a3..c70659867a4070229f080493be47b521ed984609 100644 (file)
--- 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("") == "");