Less open file permissions (#21922)

* Less open file permissions

* add test back

* remove params test for permissions

* remove umask

* bump cereal

Co-authored-by: Adeeb Shihadeh <adeebshihadeh@gmail.com>

cereal

@@ -1 +1 @@
-Subproject commit 9327e4fbec1a1b4e0658a18db4a6f332c1f53688
+Subproject commit e137c5731eb0264497c1f3b151fa030af5bb940a

common/file_helpers.py

@@ -39,28 +39,6 @@ def get_tmpdir_on_same_filesystem(path):
   return "/tmp"
 
 
-class AutoMoveTempdir():
-  def __init__(self, target_path, temp_dir=None):
-    self._target_path = target_path
-    self._path = tempfile.mkdtemp(dir=temp_dir)
-
-  @property
-  def name(self):
-    return self._path
-
-  def close(self):
-    os.rename(self._path, self._target_path)
-
-  def __enter__(self):
-    return self
-
-  def __exit__(self, exc_type, exc_value, traceback):
-    if exc_type is None:
-      self.close()
-    else:
-      shutil.rmtree(self._path)
-
-
 class NamedTemporaryDir():
   def __init__(self, temp_dir=None):
     self._path = tempfile.mkdtemp(dir=temp_dir)
@@ -100,9 +78,7 @@ class CallbackReader:
 
 def _get_fileobject_func(writer, temp_dir):
   def _get_fileobject():
-    file_obj = writer.get_fileobject(dir=temp_dir)
-    os.chmod(file_obj.name, 0o644)
-    return file_obj
+    return writer.get_fileobject(dir=temp_dir)
   return _get_fileobject
 
 
@@ -122,20 +98,3 @@ def atomic_write_in_dir(path, **kwargs):
   """
   writer = AtomicWriter(path, **kwargs)
   return writer._open(_get_fileobject_func(writer, os.path.dirname(path)))
-
-
-def atomic_write_in_dir_neos(path, contents, mode=None):
-  """
-  Atomically writes contents to path using a temporary file in the same directory
-  as path. Useful on NEOS, where `os.link` (required by atomic_write_in_dir) is missing.
-  """
-
-  f = tempfile.NamedTemporaryFile(delete=False, prefix=".tmp", dir=os.path.dirname(path))
-  f.write(contents)
-  f.flush()
-  if mode is not None:
-    os.fchmod(f.fileno(), mode)
-  os.fsync(f.fileno())
-  f.close()
-
-  os.rename(f.name, path)

common/tests/test_file_helpers.py

@@ -14,7 +14,6 @@ class TestFileHelpers(unittest.TestCase):
 
     with open(path) as f:
       self.assertEqual(f.read(), "test")
-    self.assertEqual(os.stat(path).st_mode & 0o777, 0o644)
     os.remove(path)
 
   def test_atomic_write_on_fs_tmp(self):

common/tests/test_params.py

@@ -1,9 +1,7 @@
-import os
 import threading
 import time
 import tempfile
 import shutil
-import stat
 import unittest
 
 from common.params import Params, ParamKeyType, UnknownKeyName, put_nonblocking
@@ -69,13 +67,6 @@ class TestParams(unittest.TestCase):
     with self.assertRaises(UnknownKeyName):
       self.params.put_bool("swag", True)
 
-  def test_params_permissions(self):
-    permissions = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IWGRP | stat.S_IROTH | stat.S_IWOTH
-
-    self.params.put("DongleId", "cb38263377b873ee")
-    st_mode = os.stat(f"{self.tmpdir}/d/DongleId").st_mode
-    assert (st_mode & permissions) == permissions
-
   def test_delete_not_there(self):
     assert self.params.get("CarParams") is None
     self.params.delete("CarParams")

selfdrive/common/params.cc

@@ -52,25 +52,21 @@ int fsync_dir(const char* path) {
   return result;
 }
 
-// TODO: replace by std::filesystem::create_directories
 int mkdir_p(std::string path) {
   char * _path = (char *)path.c_str();
 
-  mode_t prev_mask = umask(0);
   for (char *p = _path + 1; *p; p++) {
     if (*p == '/') {
       *p = '\0'; // Temporarily truncate
-      if (mkdir(_path, 0777) != 0) {
+      if (mkdir(_path, 0775) != 0) {
         if (errno != EEXIST) return -1;
       }
       *p = '/';
     }
   }
-  if (mkdir(_path, 0777) != 0) {
+  if (mkdir(_path, 0775) != 0) {
     if (errno != EEXIST) return -1;
   }
-  chmod(_path, 0777);
-  umask(prev_mask);
   return 0;
 }
 
@@ -94,10 +90,6 @@ bool create_params_path(const std::string &param_path, const std::string &key_pa
       return false;
     }
 
-    if (chmod(tmp_dir, 0777) != 0) {
-      return false;
-    }
-
     std::string link_path = std::string(tmp_dir) + ".link";
     if (symlink(tmp_dir, link_path.c_str()) != 0) {
       return false;
@@ -109,8 +101,7 @@ bool create_params_path(const std::string &param_path, const std::string &key_pa
     }
   }
 
-  // Ensure permissions are correct in case we didn't create the symlink
-  return chmod(key_path.c_str(), 0777) == 0;
+  return true;
 }
 
 void ensure_params_path(const std::string &params_path) {
@@ -265,8 +256,6 @@ int Params::put(const char* key, const char* value, size_t value_size) {
       break;
     }
 
-    // change permissions to 0666 for apks
-    if ((result = fchmod(tmp_fd, 0666)) < 0) break;
     // fsync to force persist the changes.
     if ((result = fsync(tmp_fd)) < 0) break;
 

selfdrive/common/util.h

@@ -61,7 +61,7 @@ std::string dir_name(std::string const& path);
 // **** file fhelpers *****
 std::string read_file(const std::string& fn);
 std::map<std::string, std::string> read_files_in_dir(const std::string& path);
-int write_file(const char* path, const void* data, size_t size, int flags = O_WRONLY, mode_t mode = 0777);
+int write_file(const char* path, const void* data, size_t size, int flags = O_WRONLY, mode_t mode = 0664);
 std::string readlink(const std::string& path);
 bool file_exists(const std::string& fn);
 

selfdrive/loggerd/logger.cc

@@ -36,7 +36,7 @@ int logger_mkpath(char* file_path) {
   char* p;
   for (p=strchr(file_path+1, '/'); p; p=strchr(p+1, '/')) {
     *p = '\0';
-    if (mkdir(file_path, 0777)==-1) {
+    if (mkdir(file_path, 0775)==-1) {
       if (errno != EEXIST) {
         *p = '/';
         return -1;
@@ -102,7 +102,7 @@ kj::Array<capnp::word> logger_build_init_data() {
   init.setGitRemote(params_map["GitRemote"]);
   init.setPassive(params.getBool("Passive"));
   init.setDongleId(params_map["DongleId"]);
-  
+
   auto lparams = init.initParams().initEntries(params_map.size());
   int i = 0;
   for (auto& [key, value] : params_map) {
@@ -145,8 +145,6 @@ static void log_sentinel(LoggerState *s, cereal::Sentinel::SentinelType type, in
 // ***** logging functions *****
 
 void logger_init(LoggerState *s, const char* log_name, bool has_qlog) {
-  umask(0);
-
   pthread_mutex_init(&s->lock, NULL);
 
   s->part = -1;

selfdrive/loggerd/omx_encoder.cc

@@ -514,7 +514,7 @@ void OmxEncoder::encoder_open(const char* path) {
 
   // create camera lock file
   snprintf(this->lock_path, sizeof(this->lock_path), "%s/%s.lock", path, this->filename);
-  int lock_fd = open(this->lock_path, O_RDWR | O_CREAT, 0777);
+  int lock_fd = open(this->lock_path, O_RDWR | O_CREAT, 0664);
   assert(lock_fd >= 0);
   close(lock_fd);
 
@@ -583,8 +583,8 @@ OmxEncoder::~OmxEncoder() {
   OMX_CHECK(OMX_FreeHandle(this->handle));
 
   OMX_BUFFERHEADERTYPE *out_buf;
-  while (this->free_in.try_pop(out_buf)); 
-  while (this->done_out.try_pop(out_buf)); 
+  while (this->free_in.try_pop(out_buf));
+  while (this->done_out.try_pop(out_buf));
 
   if (this->codec_config) {
     free(this->codec_config);

selfdrive/loggerd/raw_logger.cc

@@ -70,7 +70,7 @@ void RawLogger::encoder_open(const char* path) {
 
   LOG("open %s\n", lock_path.c_str());
 
-  int lock_fd = open(lock_path.c_str(), O_RDWR | O_CREAT, 0777);
+  int lock_fd = open(lock_path.c_str(), O_RDWR | O_CREAT, 0664);
   assert(lock_fd >= 0);
   close(lock_fd);
 

selfdrive/manager/manager.py

@@ -55,8 +55,6 @@ def manager_init():
   if params.get("Passive") is None:
     raise Exception("Passive must be set to continue")
 
-  os.umask(0)  # Make sure we can create files with 777 permissions
-
   # Create folders needed for msgq
   try:
     os.mkdir("/dev/shm")

tools/lib/cache.py

@@ -1,6 +1,6 @@
 import os
 import urllib.parse
-from tools.lib.file_helpers import mkdirs_exists_ok
+from common.file_helpers import mkdirs_exists_ok
 
 DEFAULT_CACHE_DIR = os.path.expanduser("~/.commacache")
 

tools/lib/file_helpers.py

@@ -1,26 +0,0 @@
-import os
-from atomicwrites import AtomicWriter
-
-
-def atomic_write_in_dir(path, **kwargs):
-  """Creates an atomic writer using a temporary file in the same directory
-     as the destination file.
-  """
-  writer = AtomicWriter(path, **kwargs)
-  return writer._open(_get_fileobject_func(writer, os.path.dirname(path)))
-
-
-def _get_fileobject_func(writer, temp_dir):
-  def _get_fileobject():
-    file_obj = writer.get_fileobject(dir=temp_dir)
-    os.chmod(file_obj.name, 0o644)
-    return file_obj
-  return _get_fileobject
-
-
-def mkdirs_exists_ok(path):
-  try:
-    os.makedirs(path)
-  except OSError:
-    if not os.path.isdir(path):
-      raise

tools/lib/framereader.py

@@ -15,7 +15,7 @@ from lru import LRU
 import _io
 from tools.lib.cache import cache_path_for_file_path
 from tools.lib.exceptions import DataUnreadableError
-from tools.lib.file_helpers import atomic_write_in_dir
+from common.file_helpers import atomic_write_in_dir
 
 try:
   from xx.chffr.lib.filereader import FileReader

tools/lib/url_file.py

@@ -9,7 +9,7 @@ import pycurl
 from hashlib import sha256
 from io import BytesIO
 from tenacity import retry, wait_random_exponential, stop_after_attempt
-from tools.lib.file_helpers import mkdirs_exists_ok, atomic_write_in_dir
+from common.file_helpers import mkdirs_exists_ok, atomic_write_in_dir
 #  Cache chunk size
 K = 1000
 CHUNK_SIZE = 1000 * K