diff --git a/init/builtins.cpp b/init/builtins.cpp index 848dfdbf9..27b72f96c 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -150,7 +150,12 @@ static int do_class_restart(const std::vector& args) { } static int do_domainname(const std::vector& args) { - return write_file("/proc/sys/kernel/domainname", args[1]) ? 0 : 1; + std::string err; + if (!WriteFile("/proc/sys/kernel/domainname", args[1], &err)) { + LOG(ERROR) << err; + return -1; + } + return 0; } static int do_enable(const std::vector& args) { @@ -174,7 +179,12 @@ static int do_export(const std::vector& args) { } static int do_hostname(const std::vector& args) { - return write_file("/proc/sys/kernel/hostname", args[1]) ? 0 : 1; + std::string err; + if (!WriteFile("/proc/sys/kernel/hostname", args[1], &err)) { + LOG(ERROR) << err; + return -1; + } + return 0; } static int do_ifup(const std::vector& args) { @@ -651,15 +661,26 @@ static int do_verity_update_state(const std::vector& args) { } static int do_write(const std::vector& args) { - return write_file(args[1], args[2]) ? 0 : 1; + std::string err; + if (!WriteFile(args[1], args[2], &err)) { + LOG(ERROR) << err; + return -1; + } + return 0; } static int do_copy(const std::vector& args) { std::string data; - if (read_file(args[1], &data)) { - return write_file(args[2], data) ? 0 : 1; + std::string err; + if (!ReadFile(args[1], &data, &err)) { + LOG(ERROR) << err; + return -1; } - return 1; + if (!WriteFile(args[2], data, &err)) { + LOG(ERROR) << err; + return -1; + } + return 0; } static int do_chown(const std::vector& args) { diff --git a/init/init.cpp b/init/init.cpp index 52f6b0cec..b5f97e5c8 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -853,7 +853,9 @@ static void selinux_initialize(bool in_kernel_domain) { } } - if (!write_file("/sys/fs/selinux/checkreqprot", "0")) { + std::string err; + if (!WriteFile("/sys/fs/selinux/checkreqprot", "0", &err)) { + LOG(ERROR) << err; security_failure(); } diff --git a/init/init_parser.cpp b/init/init_parser.cpp index 620367a62..1b31cf223 100644 --- a/init/init_parser.cpp +++ b/init/init_parser.cpp @@ -110,7 +110,9 @@ bool Parser::ParseConfigFile(const std::string& path) { LOG(INFO) << "Parsing file " << path << "..."; Timer t; std::string data; - if (!read_file(path, &data)) { + std::string err; + if (!ReadFile(path, &data, &err)) { + LOG(ERROR) << err; return false; } diff --git a/init/init_test.cpp b/init/init_test.cpp index 3da14b5ab..7093ba980 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -152,10 +152,11 @@ TEST(init, EventTriggerOrderMultipleFiles) { "on boot\n" "execute 3"; // clang-format on - // write_file() ensures the right mode is set - ASSERT_TRUE(write_file(std::string(dir.path) + "/a.rc", dir_a_script)); + // WriteFile() ensures the right mode is set + std::string err; + ASSERT_TRUE(WriteFile(std::string(dir.path) + "/a.rc", dir_a_script, &err)); - ASSERT_TRUE(write_file(std::string(dir.path) + "/b.rc", "on boot\nexecute 5")); + ASSERT_TRUE(WriteFile(std::string(dir.path) + "/b.rc", "on boot\nexecute 5", &err)); // clang-format off std::string start_script = "import " + std::string(first_import.path) + "\n" diff --git a/init/property_service.cpp b/init/property_service.cpp index 6f40d1ce0..e01cd0b4b 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -510,8 +510,9 @@ static void load_properties(char *data, const char *filter) static void load_properties_from_file(const char* filename, const char* filter) { Timer t; std::string data; - if (!read_file(filename, &data)) { - PLOG(WARNING) << "Couldn't load properties from " << filename; + std::string err; + if (!ReadFile(filename, &data, &err)) { + PLOG(WARNING) << "Couldn't load property file: " << err; return; } data.push_back('\n'); diff --git a/init/util.cpp b/init/util.cpp index 4df0c25c9..e7f724b8f 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -151,12 +151,14 @@ out_unlink: return -1; } -bool read_file(const std::string& path, std::string* content) { +bool ReadFile(const std::string& path, std::string* content, std::string* err) { content->clear(); + *err = ""; android::base::unique_fd fd( TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC))); if (fd == -1) { + *err = "Unable to open '" + path + "': " + strerror(errno); return false; } @@ -164,29 +166,35 @@ bool read_file(const std::string& path, std::string* content) { // or group-writable files. struct stat sb; if (fstat(fd, &sb) == -1) { - PLOG(ERROR) << "fstat failed for '" << path << "'"; + *err = "fstat failed for '" + path + "': " + strerror(errno); return false; } if ((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) { - LOG(ERROR) << "skipping insecure file '" << path << "'"; + *err = "Skipping insecure file '" + path + "'"; return false; } - return android::base::ReadFdToString(fd, content); + if (!android::base::ReadFdToString(fd, content)) { + *err = "Unable to read '" + path + "': " + strerror(errno); + return false; + } + return true; } -bool write_file(const std::string& path, const std::string& content) { +bool WriteFile(const std::string& path, const std::string& content, std::string* err) { + *err = ""; + android::base::unique_fd fd(TEMP_FAILURE_RETRY( open(path.c_str(), O_WRONLY | O_CREAT | O_NOFOLLOW | O_TRUNC | O_CLOEXEC, 0600))); if (fd == -1) { - PLOG(ERROR) << "write_file: Unable to open '" << path << "'"; + *err = "Unable to open '" + path + "': " + strerror(errno); return false; } - bool success = android::base::WriteStringToFd(content, fd); - if (!success) { - PLOG(ERROR) << "write_file: Unable to write to '" << path << "'"; + if (!android::base::WriteStringToFd(content, fd)) { + *err = "Unable to write to '" + path + "': " + strerror(errno); + return false; } - return success; + return true; } int mkdir_recursive(const std::string& path, mode_t mode, selabel_handle* sehandle) { diff --git a/init/util.h b/init/util.h index 4957f1550..4c33909f0 100644 --- a/init/util.h +++ b/init/util.h @@ -38,8 +38,8 @@ using namespace std::chrono_literals; int create_socket(const char* name, int type, mode_t perm, uid_t uid, gid_t gid, const char* socketcon, selabel_handle* sehandle); -bool read_file(const std::string& path, std::string* content); -bool write_file(const std::string& path, const std::string& content); +bool ReadFile(const std::string& path, std::string* content, std::string* err); +bool WriteFile(const std::string& path, const std::string& content, std::string* err); class Timer { public: diff --git a/init/util_test.cpp b/init/util_test.cpp index a24185b00..4bb8a8378 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -24,53 +24,67 @@ #include #include -TEST(util, read_file_ENOENT) { - std::string s("hello"); - errno = 0; - EXPECT_FALSE(read_file("/proc/does-not-exist", &s)); - EXPECT_EQ(ENOENT, errno); - EXPECT_EQ("", s); // s was cleared. +using namespace std::literals::string_literals; + +TEST(util, ReadFile_ENOENT) { + std::string s("hello"); + std::string err; + errno = 0; + EXPECT_FALSE(ReadFile("/proc/does-not-exist", &s, &err)); + EXPECT_EQ("Unable to open '/proc/does-not-exist': No such file or directory", err); + EXPECT_EQ(ENOENT, errno); + EXPECT_EQ("", s); // s was cleared. } -TEST(util, read_file_group_writeable) { +TEST(util, ReadFileGroupWriteable) { std::string s("hello"); TemporaryFile tf; + std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(write_file(tf.path, s)) << strerror(errno); + EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); + EXPECT_EQ("", err); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0620, AT_SYMLINK_NOFOLLOW)) << strerror(errno); - EXPECT_FALSE(read_file(tf.path, &s)) << strerror(errno); + EXPECT_FALSE(ReadFile(tf.path, &s, &err)) << strerror(errno); + EXPECT_EQ("Skipping insecure file '"s + tf.path + "'", err); EXPECT_EQ("", s); // s was cleared. } -TEST(util, read_file_world_writeable) { +TEST(util, ReadFileWorldWiteable) { std::string s("hello"); TemporaryFile tf; + std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(write_file(tf.path, s.c_str())) << strerror(errno); + EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); + EXPECT_EQ("", err); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0602, AT_SYMLINK_NOFOLLOW)) << strerror(errno); - EXPECT_FALSE(read_file(tf.path, &s)) << strerror(errno); + EXPECT_FALSE(ReadFile(tf.path, &s, &err)) << strerror(errno); + EXPECT_EQ("Skipping insecure file '"s + tf.path + "'", err); EXPECT_EQ("", s); // s was cleared. } -TEST(util, read_file_symbolic_link) { +TEST(util, ReadFileSymbolicLink) { std::string s("hello"); errno = 0; // lrwxrwxrwx 1 root root 13 1970-01-01 00:00 charger -> /sbin/healthd - EXPECT_FALSE(read_file("/charger", &s)); + std::string err; + EXPECT_FALSE(ReadFile("/charger", &s, &err)); + EXPECT_EQ("Unable to open '/charger': Too many symbolic links encountered", err); EXPECT_EQ(ELOOP, errno); EXPECT_EQ("", s); // s was cleared. } -TEST(util, read_file_success) { - std::string s("hello"); - EXPECT_TRUE(read_file("/proc/version", &s)); - EXPECT_GT(s.length(), 6U); - EXPECT_EQ('\n', s[s.length() - 1]); - s[5] = 0; - EXPECT_STREQ("Linux", s.c_str()); +TEST(util, ReadFileSuccess) { + std::string s("hello"); + std::string err; + EXPECT_TRUE(ReadFile("/proc/version", &s, &err)); + EXPECT_EQ("", err); + EXPECT_GT(s.length(), 6U); + EXPECT_EQ('\n', s[s.length() - 1]); + s[5] = 0; + EXPECT_STREQ("Linux", s.c_str()); } -TEST(util, write_file_binary) { +TEST(util, WriteFileBinary) { std::string contents("abcd"); contents.push_back('\0'); contents.push_back('\0'); @@ -78,22 +92,28 @@ TEST(util, write_file_binary) { ASSERT_EQ(10u, contents.size()); TemporaryFile tf; + std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(write_file(tf.path, contents)) << strerror(errno); + EXPECT_TRUE(WriteFile(tf.path, contents, &err)) << strerror(errno); + EXPECT_EQ("", err); std::string read_back_contents; - EXPECT_TRUE(read_file(tf.path, &read_back_contents)) << strerror(errno); + EXPECT_TRUE(ReadFile(tf.path, &read_back_contents, &err)) << strerror(errno); + EXPECT_EQ("", err); EXPECT_EQ(contents, read_back_contents); EXPECT_EQ(10u, read_back_contents.size()); } -TEST(util, write_file_not_exist) { +TEST(util, WriteFileNotExist) { std::string s("hello"); std::string s2("hello"); TemporaryDir test_dir; std::string path = android::base::StringPrintf("%s/does-not-exist", test_dir.path); - EXPECT_TRUE(write_file(path, s)); - EXPECT_TRUE(read_file(path, &s2)); + std::string err; + EXPECT_TRUE(WriteFile(path, s, &err)); + EXPECT_EQ("", err); + EXPECT_TRUE(ReadFile(path, &s2, &err)); + EXPECT_EQ("", err); EXPECT_EQ(s, s2); struct stat sb; int fd = open(path.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC); @@ -103,15 +123,20 @@ TEST(util, write_file_not_exist) { EXPECT_EQ(0, unlink(path.c_str())); } -TEST(util, write_file_exist) { +TEST(util, WriteFileExist) { std::string s2(""); TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(write_file(tf.path, "1hello1")) << strerror(errno); - EXPECT_TRUE(read_file(tf.path, &s2)); + std::string err; + EXPECT_TRUE(WriteFile(tf.path, "1hello1", &err)) << strerror(errno); + EXPECT_EQ("", err); + EXPECT_TRUE(ReadFile(tf.path, &s2, &err)); + EXPECT_EQ("", err); EXPECT_STREQ("1hello1", s2.c_str()); - EXPECT_TRUE(write_file(tf.path, "2ll2")); - EXPECT_TRUE(read_file(tf.path, &s2)); + EXPECT_TRUE(WriteFile(tf.path, "2ll2", &err)); + EXPECT_EQ("", err); + EXPECT_TRUE(ReadFile(tf.path, &s2, &err)); + EXPECT_EQ("", err); EXPECT_STREQ("2ll2", s2.c_str()); }