From 11a3aeeae3dc887b889d4086d4d26d95c324c08d Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Thu, 3 Aug 2017 12:54:07 -0700 Subject: [PATCH] init: introduce Result for return values and error handling init tries to propagate error information up to build context before logging errors. This is a good thing, however too often init has the overly verbose paradigm for error handling, below: bool CalculateResult(const T& input, U* output, std::string* err) bool CalculateAndUseResult(const T& input, std::string* err) { U output; std::string calculate_result_err; if (!CalculateResult(input, &output, &calculate_result_err)) { *err = "CalculateResult " + input + " failed: " + calculate_result_err; return false; } UseResult(output); return true; } Even more common are functions that return only true/false but also require passing a std::string* err in order to see the error message. This change introduces a Result that is use to either hold a successful return value of type T or to hold an error message as a std::string. If the functional only returns success or a failure with an error message, Result may be used. The classes Error and ErrnoError are used to indicate a failed Result. A successful Result is constructed implicitly from any type that can be implicitly converted to T or from the constructor arguments for T. This allows you to return a type T directly from a function that returns Result. Error and ErrnoError are used to construct a Result has failed. Each of these classes take an ostream as an input and are implicitly cast to a Result containing that failure. ErrnoError() additionally appends ": " + strerror(errno) to the end of the failure string to aid in interacting with C APIs. The end result is that the above code snippet is turned into the much clearer example below: Result CalculateResult(const T& input); Result CalculateAndUseResult(const T& input) { auto output = CalculateResult(input); if (!output) { return Error() << "CalculateResult " << input << " failed: " << output.error(); } UseResult(*output); return Success(); } This change also makes this conversion for some of the util.cpp functions that used the old paradigm. Test: boot bullhead, init unit tests Merged-In: I1e7d3a8820a79362245041251057fbeed2f7979b Change-Id: I1e7d3a8820a79362245041251057fbeed2f7979b --- init/Android.bp | 1 + init/builtins.cpp | 58 +++++----- init/init_test.cpp | 5 +- init/parser.cpp | 11 +- init/property_service.cpp | 12 +-- init/result.h | 142 ++++++++++++++++++++++++ init/result_test.cpp | 222 ++++++++++++++++++++++++++++++++++++++ init/selinux.cpp | 5 +- init/service.cpp | 67 ++++++------ init/service_test.cpp | 28 ++--- init/util.cpp | 60 ++++------- init/util.h | 8 +- init/util_test.cpp | 115 ++++++++------------ 13 files changed, 528 insertions(+), 206 deletions(-) create mode 100644 init/result.h create mode 100644 init/result_test.cpp diff --git a/init/Android.bp b/init/Android.bp index db64f71d4..b1f02793f 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -159,6 +159,7 @@ cc_test { "devices_test.cpp", "init_test.cpp", "property_service_test.cpp", + "result_test.cpp", "service_test.cpp", "ueventd_test.cpp", "util_test.cpp", diff --git a/init/builtins.cpp b/init/builtins.cpp index 944fcee22..28f60e6ac 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -151,9 +151,8 @@ static int do_class_restart(const std::vector& args) { } static int do_domainname(const std::vector& args) { - std::string err; - if (!WriteFile("/proc/sys/kernel/domainname", args[1], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/proc/sys/kernel/domainname", args[1]); !result) { + LOG(ERROR) << "Unable to write to /proc/sys/kernel/domainname: " << result.error(); return -1; } return 0; @@ -199,9 +198,8 @@ static int do_export(const std::vector& args) { } static int do_hostname(const std::vector& args) { - std::string err; - if (!WriteFile("/proc/sys/kernel/hostname", args[1], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/proc/sys/kernel/hostname", args[1]); !result) { + LOG(ERROR) << "Unable to write to /proc/sys/kernel/hostname: " << result.error(); return -1; } return 0; @@ -244,22 +242,22 @@ static int do_mkdir(const std::vector& args) { } if (args.size() >= 4) { - uid_t uid; - std::string decode_uid_err; - if (!DecodeUid(args[3], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[3] << "': " << decode_uid_err; + auto uid = DecodeUid(args[3]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[3] << "': " << uid.error(); return -1; } - gid_t gid = -1; + Result gid = -1; if (args.size() == 5) { - if (!DecodeUid(args[4], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + gid = DecodeUid(args[4]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[3] << "': " << gid.error(); return -1; } } - if (lchown(args[1].c_str(), uid, gid) == -1) { + if (lchown(args[1].c_str(), *uid, *gid) == -1) { return -errno; } @@ -651,9 +649,8 @@ static int do_verity_update_state(const std::vector& args) { } static int do_write(const std::vector& args) { - std::string err; - if (!WriteFile(args[1], args[2], &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile(args[1], args[2]); !result) { + LOG(ERROR) << "Unable to write to file '" << args[1] << "': " << result.error(); return -1; } return 0; @@ -720,39 +717,38 @@ static int do_readahead(const std::vector& args) { } static int do_copy(const std::vector& args) { - std::string data; - std::string err; - if (!ReadFile(args[1], &data, &err)) { - LOG(ERROR) << err; + auto file_contents = ReadFile(args[1]); + if (!file_contents) { + LOG(ERROR) << "Could not read input file '" << args[1] << "': " << file_contents.error(); return -1; } - if (!WriteFile(args[2], data, &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile(args[2], *file_contents); !result) { + LOG(ERROR) << "Could not write to output file '" << args[2] << "': " << result.error(); return -1; } return 0; } static int do_chown(const std::vector& args) { - uid_t uid; - std::string decode_uid_err; - if (!DecodeUid(args[1], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[1] << "': " << decode_uid_err; + auto uid = DecodeUid(args[1]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[1] << "': " << uid.error(); return -1; } // GID is optional and pushes the index of path out by one if specified. const std::string& path = (args.size() == 4) ? args[3] : args[2]; - gid_t gid = -1; + Result gid = -1; if (args.size() == 4) { - if (!DecodeUid(args[2], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[2] << "': " << decode_uid_err; + gid = DecodeUid(args[2]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[2] << "': " << gid.error(); return -1; } } - if (lchown(path.c_str(), uid, gid) == -1) return -errno; + if (lchown(path.c_str(), *uid, *gid) == -1) return -errno; return 0; } diff --git a/init/init_test.cpp b/init/init_test.cpp index 20622901c..58e3d758e 100644 --- a/init/init_test.cpp +++ b/init/init_test.cpp @@ -156,10 +156,9 @@ TEST(init, EventTriggerOrderMultipleFiles) { "execute 3"; // clang-format on // 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(WriteFile(std::string(dir.path) + "/a.rc", dir_a_script)); - ASSERT_TRUE(WriteFile(std::string(dir.path) + "/b.rc", "on boot\nexecute 5", &err)); + ASSERT_TRUE(WriteFile(std::string(dir.path) + "/b.rc", "on boot\nexecute 5")); // clang-format off std::string start_script = "import " + std::string(first_import.path) + "\n" diff --git a/init/parser.cpp b/init/parser.cpp index c6f4f459b..4c34c265b 100644 --- a/init/parser.cpp +++ b/init/parser.cpp @@ -102,15 +102,14 @@ void Parser::ParseData(const std::string& filename, const std::string& data) { bool Parser::ParseConfigFile(const std::string& path) { LOG(INFO) << "Parsing file " << path << "..."; android::base::Timer t; - std::string data; - std::string err; - if (!ReadFile(path, &data, &err)) { - LOG(ERROR) << err; + auto config_contents = ReadFile(path); + if (!config_contents) { + LOG(ERROR) << "Unable to read config file '" << path << "': " << config_contents.error(); return false; } - data.push_back('\n'); // TODO: fix parse_config. - ParseData(path, data); + config_contents->push_back('\n'); // TODO: fix parse_config. + ParseData(path, *config_contents); for (const auto& [section_name, section_parser] : section_parsers_) { section_parser->EndFile(); } diff --git a/init/property_service.cpp b/init/property_service.cpp index 1db8cb7d3..e07550a39 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -588,14 +588,14 @@ static void load_properties(char *data, const char *filter) // "ro.foo.*" is a prefix match, and "ro.foo.bar" is an exact match. static void load_properties_from_file(const char* filename, const char* filter) { Timer t; - std::string data; - std::string err; - if (!ReadFile(filename, &data, &err)) { - PLOG(WARNING) << "Couldn't load property file: " << err; + auto file_contents = ReadFile(filename); + if (!file_contents) { + PLOG(WARNING) << "Couldn't load property file '" << filename + << "': " << file_contents.error(); return; } - data.push_back('\n'); - load_properties(&data[0], filter); + file_contents->push_back('\n'); + load_properties(file_contents->data(), filter); LOG(VERBOSE) << "(Loading properties from " << filename << " took " << t << ".)"; } diff --git a/init/result.h b/init/result.h new file mode 100644 index 000000000..64fa69f26 --- /dev/null +++ b/init/result.h @@ -0,0 +1,142 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// This file contains classes for returning a successful result along with an optional +// arbitrarily typed return value or for returning a failure result along with an optional string +// indicating why the function failed. + +// There are 3 classes that implement this functionality and one additional helper type. +// +// Result either contains a member of type T that can be accessed using similar semantics as +// std::optional or it contains a std::string describing an error, which can be accessed via +// Result::error(). +// +// Success is a typedef that aids in creating Result that do not contain a return value. +// Result is the correct return type for a function that either returns successfully or +// returns an error value. Returning Success() from a function that returns Result is the +// correct way to indicate that a function without a return type has completed successfully. +// +// A successful Result is constructed implicitly from any type that can be implicitly converted +// to T or from the constructor arguments for T. This allows you to return a type T directly from +// a function that returns Result. +// +// Error and ErrnoError are used to construct a Result that has failed. Each of these classes +// take an ostream as an input and are implicitly cast to a Result containing that failure. +// ErrnoError() additionally appends ": " + strerror(errno) to the end of the failure string to aid +// in interacting with C APIs. + +// An example of how to use these is below: +// Result CalculateResult(const T& input) { +// U output; +// if (!SomeOtherCppFunction(input, &output)) { +// return Error() << "SomeOtherCppFunction(" << input << ") failed"; +// } +// if (!c_api_function(output)) { +// return ErrnoError() << "c_api_function(" << output << ") failed"; +// } +// return output; +// } +// +// auto output = CalculateResult(input); +// if (!output) return Error() << "CalculateResult failed: " << output.error(); +// UseOutput(*output); + +#ifndef _INIT_RESULT_H +#define _INIT_RESULT_H + +#include + +#include +#include +#include + +namespace android { +namespace init { + +class Error { + public: + Error() : append_errno_(0) {} + + template + Error&& operator<<(T&& t) { + ss_ << std::forward(t); + return std::move(*this); + } + + const std::string str() const { + if (append_errno_) { + return ss_.str() + ": " + strerror(append_errno_); + } + return ss_.str(); + } + + Error(const Error&) = delete; + Error(Error&&) = delete; + Error& operator=(const Error&) = delete; + Error& operator=(Error&&) = delete; + + protected: + Error(int append_errno) : append_errno_(append_errno) {} + + private: + std::stringstream ss_; + int append_errno_; +}; + +class ErrnoError : public Error { + public: + ErrnoError() : Error(errno) {} +}; + +template +class Result { + public: + template + Result(U&&... result) : contents_(std::in_place_index_t<0>(), std::forward(result)...) {} + + Result(Error&& fb) : contents_(std::in_place_index_t<1>(), fb.str()) {} + + bool has_value() const { return contents_.index() == 0; } + + T& value() & { return std::get<0>(contents_); } + const T& value() const & { return std::get<0>(contents_); } + T&& value() && { return std::get<0>(std::move(contents_)); } + const T&& value() const && { return std::get<0>(std::move(contents_)); } + + const std::string& error() const & { return std::get<1>(contents_); } + std::string&& error() && { return std::get<1>(std::move(contents_)); } + const std::string&& error() const && { return std::get<1>(std::move(contents_)); } + + explicit operator bool() const { return has_value(); } + + T& operator*() & { return value(); } + const T& operator*() const & { return value(); } + T&& operator*() && { return std::move(value()); } + const T&& operator*() const && { return std::move(value()); } + + T* operator->() { return &value(); } + const T* operator->() const { return &value(); } + + private: + std::variant contents_; +}; + +using Success = std::monostate; + +} // namespace init +} // namespace android + +#endif diff --git a/init/result_test.cpp b/init/result_test.cpp new file mode 100644 index 000000000..ca6501369 --- /dev/null +++ b/init/result_test.cpp @@ -0,0 +1,222 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "result.h" + +#include "errno.h" + +#include + +#include + +using namespace std::string_literals; + +namespace android { +namespace init { + +TEST(result, result_accessors) { + Result result = "success"; + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ("success", *result); + EXPECT_EQ("success", result.value()); + + EXPECT_EQ('s', result->data()[0]); +} + +TEST(result, result_accessors_rvalue) { + ASSERT_TRUE(Result("success")); + ASSERT_TRUE(Result("success").has_value()); + + EXPECT_EQ("success", *Result("success")); + EXPECT_EQ("success", Result("success").value()); + + EXPECT_EQ('s', Result("success")->data()[0]); +} + +TEST(result, result_success) { + Result result = Success(); + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ(Success(), *result); + EXPECT_EQ(Success(), result.value()); +} + +TEST(result, result_success_rvalue) { + // Success() doesn't actually create a Result object, but rather an object that can be + // implicitly constructed into a Result object. + + auto MakeRvalueSuccessResult = []() -> Result { return Success(); }; + ASSERT_TRUE(MakeRvalueSuccessResult()); + ASSERT_TRUE(MakeRvalueSuccessResult().has_value()); + + EXPECT_EQ(Success(), *MakeRvalueSuccessResult()); + EXPECT_EQ(Success(), MakeRvalueSuccessResult().value()); +} + +TEST(result, result_error) { + Result result = Error() << "failure" << 1; + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("failure1", result.error()); +} + +TEST(result, result_error_empty) { + Result result = Error(); + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("", result.error()); +} + +TEST(result, result_error_rvalue) { + // Error() and ErrnoError() aren't actually used to create a Result object. + // Under the hood, they are an intermediate class that can be implicitly constructed into a + // Result. This is needed both to create the ostream and because Error() itself, by + // definition will not know what the type, T, of the underlying Result object that it would + // create is. + + auto MakeRvalueErrorResult = []() -> Result { return Error() << "failure" << 1; }; + ASSERT_FALSE(MakeRvalueErrorResult()); + ASSERT_FALSE(MakeRvalueErrorResult().has_value()); + + EXPECT_EQ("failure1", MakeRvalueErrorResult().error()); +} + +TEST(result, result_errno_error) { + constexpr int test_errno = 6; + errno = test_errno; + Result result = ErrnoError() << "failure" << 1; + + ASSERT_FALSE(result); + ASSERT_FALSE(result.has_value()); + + EXPECT_EQ("failure1: "s + strerror(test_errno), result.error()); +} + +TEST(result, constructor_forwarding) { + auto result = Result(5, 'a'); + + ASSERT_TRUE(result); + ASSERT_TRUE(result.has_value()); + + EXPECT_EQ("aaaaa", *result); +} + +struct ConstructorTracker { + static size_t constructor_called; + static size_t copy_constructor_called; + static size_t move_constructor_called; + static size_t copy_assignment_called; + static size_t move_assignment_called; + + template + ConstructorTracker(T&& string) : string(string) { + ++constructor_called; + } + + ConstructorTracker(const ConstructorTracker& ct) { + ++copy_constructor_called; + string = ct.string; + } + ConstructorTracker(ConstructorTracker&& ct) noexcept { + ++move_constructor_called; + string = std::move(ct.string); + } + ConstructorTracker& operator=(const ConstructorTracker& ct) { + ++copy_assignment_called; + string = ct.string; + return *this; + } + ConstructorTracker& operator=(ConstructorTracker&& ct) noexcept { + ++move_assignment_called; + string = std::move(ct.string); + return *this; + } + + std::string string; +}; + +size_t ConstructorTracker::constructor_called = 0; +size_t ConstructorTracker::copy_constructor_called = 0; +size_t ConstructorTracker::move_constructor_called = 0; +size_t ConstructorTracker::copy_assignment_called = 0; +size_t ConstructorTracker::move_assignment_called = 0; + +Result ReturnConstructorTracker(const std::string& in) { + if (in.empty()) { + return "literal string"; + } + if (in == "test2") { + return ConstructorTracker(in + in + "2"); + } + ConstructorTracker result(in + " " + in); + return result; +}; + +TEST(result, no_copy_on_return) { + // If returning parameters that may be used to implicitly construct the type T of Result, + // then those parameters are forwarded to the construction of Result. + + // If returning an prvalue or xvalue, it will be move constructed during the construction of + // Result. + + // This check ensures that that is the case, and particularly that no copy constructors + // are called. + + auto result1 = ReturnConstructorTracker(""); + ASSERT_TRUE(result1); + EXPECT_EQ("literal string", result1->string); + EXPECT_EQ(1U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); + + auto result2 = ReturnConstructorTracker("test2"); + ASSERT_TRUE(result2); + EXPECT_EQ("test2test22", result2->string); + EXPECT_EQ(2U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(1U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); + + auto result3 = ReturnConstructorTracker("test3"); + ASSERT_TRUE(result3); + EXPECT_EQ("test3 test3", result3->string); + EXPECT_EQ(3U, ConstructorTracker::constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_constructor_called); + EXPECT_EQ(2U, ConstructorTracker::move_constructor_called); + EXPECT_EQ(0U, ConstructorTracker::copy_assignment_called); + EXPECT_EQ(0U, ConstructorTracker::move_assignment_called); +} + +TEST(result, die_on_access_failed_result) { + Result result = Error(); + ASSERT_DEATH(*result, ""); +} + +TEST(result, die_on_get_error_succesful_result) { + Result result = "success"; + ASSERT_DEATH(result.error(), ""); +} + +} // namespace init +} // namespace android diff --git a/init/selinux.cpp b/init/selinux.cpp index 3a4a71548..b9305ed47 100644 --- a/init/selinux.cpp +++ b/init/selinux.cpp @@ -339,9 +339,8 @@ void SelinuxInitialize() { } } - std::string err; - if (!WriteFile("/sys/fs/selinux/checkreqprot", "0", &err)) { - LOG(ERROR) << err; + if (auto result = WriteFile("/sys/fs/selinux/checkreqprot", "0"); !result) { + LOG(ERROR) << "Unable to write to /sys/fs/selinux/checkreqprot: " << result.error(); panic(); } diff --git a/init/service.cpp b/init/service.cpp index 6f756fa6b..e37888bf8 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -386,18 +386,20 @@ bool Service::ParseDisabled(const std::vector& args, std::string* e } bool Service::ParseGroup(const std::vector& args, std::string* err) { - std::string decode_uid_err; - if (!DecodeUid(args[1], &gid_, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[1] + "': " + decode_uid_err; + auto gid = DecodeUid(args[1]); + if (!gid) { + *err = "Unable to decode GID for '" + args[1] + "': " + gid.error(); return false; } + gid_ = *gid; + for (std::size_t n = 2; n < args.size(); n++) { - gid_t gid; - if (!DecodeUid(args[n], &gid, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[n] + "': " + decode_uid_err; + gid = DecodeUid(args[n]); + if (!gid) { + *err = "Unable to decode GID for '" + args[n] + "': " + gid.error(); return false; } - supp_gids_.emplace_back(gid); + supp_gids_.emplace_back(*gid); } return true; } @@ -527,26 +529,27 @@ bool Service::ParseShutdown(const std::vector& args, std::string* e template bool Service::AddDescriptor(const std::vector& args, std::string* err) { int perm = args.size() > 3 ? std::strtoul(args[3].c_str(), 0, 8) : -1; - uid_t uid = 0; - gid_t gid = 0; + Result uid = 0; + Result gid = 0; std::string context = args.size() > 6 ? args[6] : ""; - std::string decode_uid_err; if (args.size() > 4) { - if (!DecodeUid(args[4], &uid, &decode_uid_err)) { - *err = "Unable to find UID for '" + args[4] + "': " + decode_uid_err; + uid = DecodeUid(args[4]); + if (!uid) { + *err = "Unable to find UID for '" + args[4] + "': " + uid.error(); return false; } } if (args.size() > 5) { - if (!DecodeUid(args[5], &gid, &decode_uid_err)) { - *err = "Unable to find GID for '" + args[5] + "': " + decode_uid_err; + gid = DecodeUid(args[5]); + if (!gid) { + *err = "Unable to find GID for '" + args[5] + "': " + gid.error(); return false; } } - auto descriptor = std::make_unique(args[1], args[2], uid, gid, perm, context); + auto descriptor = std::make_unique(args[1], args[2], *uid, *gid, perm, context); auto old = std::find_if(descriptors_.begin(), descriptors_.end(), @@ -585,11 +588,12 @@ bool Service::ParseFile(const std::vector& args, std::string* err) } bool Service::ParseUser(const std::vector& args, std::string* err) { - std::string decode_uid_err; - if (!DecodeUid(args[1], &uid_, &decode_uid_err)) { - *err = "Unable to find UID for '" + args[1] + "': " + decode_uid_err; + auto uid = DecodeUid(args[1]); + if (!uid) { + *err = "Unable to find UID for '" + args[1] + "': " + uid.error(); return false; } + uid_ = *uid; return true; } @@ -979,34 +983,35 @@ std::unique_ptr Service::MakeTemporaryOneshotService(const std::vector< if (command_arg > 2 && args[1] != "-") { seclabel = args[1]; } - uid_t uid = 0; + Result uid = 0; if (command_arg > 3) { - std::string decode_uid_err; - if (!DecodeUid(args[2], &uid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[2] << "': " << decode_uid_err; + uid = DecodeUid(args[2]); + if (!uid) { + LOG(ERROR) << "Unable to decode UID for '" << args[2] << "': " << uid.error(); return nullptr; } } - gid_t gid = 0; + Result gid = 0; std::vector supp_gids; if (command_arg > 4) { - std::string decode_uid_err; - if (!DecodeUid(args[3], &gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + gid = DecodeUid(args[3]); + if (!gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[3] << "': " << gid.error(); return nullptr; } std::size_t nr_supp_gids = command_arg - 1 /* -- */ - 4 /* exec SECLABEL UID GID */; for (size_t i = 0; i < nr_supp_gids; ++i) { - gid_t supp_gid; - if (!DecodeUid(args[4 + i], &supp_gid, &decode_uid_err)) { - LOG(ERROR) << "Unable to find UID for '" << args[4 + i] << "': " << decode_uid_err; + auto supp_gid = DecodeUid(args[4 + i]); + if (!supp_gid) { + LOG(ERROR) << "Unable to decode GID for '" << args[4 + i] + << "': " << supp_gid.error(); return nullptr; } - supp_gids.push_back(supp_gid); + supp_gids.push_back(*supp_gid); } } - return std::make_unique(name, flags, uid, gid, supp_gids, no_capabilities, + return std::make_unique(name, flags, *uid, *gid, supp_gids, no_capabilities, namespace_flags, seclabel, str_args); } diff --git a/init/service_test.cpp b/init/service_test.cpp index 62e46f4cd..98d876f51 100644 --- a/init/service_test.cpp +++ b/init/service_test.cpp @@ -132,29 +132,29 @@ static void Test_make_temporary_oneshot_service(bool dash_dash, bool seclabel, b ASSERT_EQ("", svc->seclabel()); } if (uid) { - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("log", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->uid()); + auto decoded_uid = DecodeUid("log"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->uid()); } else { ASSERT_EQ(0U, svc->uid()); } if (gid) { - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("shell", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->gid()); + auto decoded_uid = DecodeUid("shell"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->gid()); } else { ASSERT_EQ(0U, svc->gid()); } if (supplementary_gids) { ASSERT_EQ(2U, svc->supp_gids().size()); - uid_t decoded_uid; - std::string err; - ASSERT_TRUE(DecodeUid("system", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->supp_gids()[0]); - ASSERT_TRUE(DecodeUid("adb", &decoded_uid, &err)); - ASSERT_EQ(decoded_uid, svc->supp_gids()[1]); + + auto decoded_uid = DecodeUid("system"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->supp_gids()[0]); + + decoded_uid = DecodeUid("adb"); + ASSERT_TRUE(decoded_uid); + ASSERT_EQ(*decoded_uid, svc->supp_gids()[1]); } else { ASSERT_EQ(0U, svc->supp_gids().size()); } diff --git a/init/util.cpp b/init/util.cpp index e0379876d..fcf7ca882 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -57,30 +57,20 @@ namespace init { const std::string kDefaultAndroidDtDir("/proc/device-tree/firmware/android/"); // DecodeUid() - decodes and returns the given string, which can be either the -// numeric or name representation, into the integer uid or gid. Returns -// UINT_MAX on error. -bool DecodeUid(const std::string& name, uid_t* uid, std::string* err) { - *uid = UINT_MAX; - *err = ""; - +// numeric or name representation, into the integer uid or gid. +Result DecodeUid(const std::string& name) { if (isalpha(name[0])) { passwd* pwd = getpwnam(name.c_str()); - if (!pwd) { - *err = "getpwnam failed: "s + strerror(errno); - return false; - } - *uid = pwd->pw_uid; - return true; + if (!pwd) return ErrnoError() << "getpwnam failed"; + + return pwd->pw_uid; } errno = 0; uid_t result = static_cast(strtoul(name.c_str(), 0, 0)); - if (errno) { - *err = "strtoul failed: "s + strerror(errno); - return false; - } - *uid = result; - return true; + if (errno) return ErrnoError() << "strtoul failed"; + + return result; } /* @@ -164,50 +154,40 @@ out_unlink: return -1; } -bool ReadFile(const std::string& path, std::string* content, std::string* err) { - content->clear(); - *err = ""; - +Result ReadFile(const std::string& path) { 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; + return ErrnoError() << "open() failed"; } // For security reasons, disallow world-writable // or group-writable files. struct stat sb; if (fstat(fd, &sb) == -1) { - *err = "fstat failed for '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "fstat failed()"; } if ((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) { - *err = "Skipping insecure file '" + path + "'"; - return false; + return Error() << "Skipping insecure file"; } - if (!android::base::ReadFdToString(fd, content)) { - *err = "Unable to read '" + path + "': " + strerror(errno); - return false; + std::string content; + if (!android::base::ReadFdToString(fd, &content)) { + return ErrnoError() << "Unable to read file contents"; } - return true; + return content; } -bool WriteFile(const std::string& path, const std::string& content, std::string* err) { - *err = ""; - +Result WriteFile(const std::string& path, const std::string& content) { 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) { - *err = "Unable to open '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "open() failed"; } if (!android::base::WriteStringToFd(content, fd)) { - *err = "Unable to write to '" + path + "': " + strerror(errno); - return false; + return ErrnoError() << "Unable to write file contents"; } - return true; + return Success(); } bool mkdir_recursive(const std::string& path, mode_t mode) { diff --git a/init/util.h b/init/util.h index a81df478a..298aa1cb2 100644 --- a/init/util.h +++ b/init/util.h @@ -28,6 +28,8 @@ #include #include +#include "result.h" + #define COLDBOOT_DONE "/dev/.coldboot_done" using android::base::boot_clock; @@ -39,10 +41,10 @@ namespace init { int CreateSocket(const char* name, int type, bool passcred, mode_t perm, uid_t uid, gid_t gid, const char* socketcon); -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); +Result ReadFile(const std::string& path); +Result WriteFile(const std::string& path, const std::string& content); -bool DecodeUid(const std::string& name, uid_t* uid, std::string* err); +Result DecodeUid(const std::string& name); bool mkdir_recursive(const std::string& pathname, mode_t mode); int wait_for_file(const char *filename, std::chrono::nanoseconds timeout); diff --git a/init/util_test.cpp b/init/util_test.cpp index 5dd271cd0..007d10ee3 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -30,61 +30,51 @@ namespace android { namespace init { 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); + auto file_contents = ReadFile("/proc/does-not-exist"); EXPECT_EQ(ENOENT, errno); - EXPECT_EQ("", s); // s was cleared. + ASSERT_FALSE(file_contents); + EXPECT_EQ("open() failed: No such file or directory", file_contents.error()); } TEST(util, ReadFileGroupWriteable) { std::string s("hello"); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, s)) << strerror(errno); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0620, AT_SYMLINK_NOFOLLOW)) << 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. + auto file_contents = ReadFile(tf.path); + ASSERT_FALSE(file_contents) << strerror(errno); + EXPECT_EQ("Skipping insecure file", file_contents.error()); } TEST(util, ReadFileWorldWiteable) { std::string s("hello"); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, s, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, s)) << strerror(errno); EXPECT_NE(-1, fchmodat(AT_FDCWD, tf.path, 0602, AT_SYMLINK_NOFOLLOW)) << 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. + auto file_contents = ReadFile(tf.path); + ASSERT_FALSE(file_contents) << strerror(errno); + EXPECT_EQ("Skipping insecure file", file_contents.error()); } TEST(util, ReadFileSymbolicLink) { - std::string s("hello"); errno = 0; // lrwxrwxrwx 1 root root 13 1970-01-01 00:00 charger -> /sbin/healthd - std::string err; - EXPECT_FALSE(ReadFile("/charger", &s, &err)); - EXPECT_EQ("Unable to open '/charger': Too many symbolic links encountered", err); + auto file_contents = ReadFile("/charger"); EXPECT_EQ(ELOOP, errno); - EXPECT_EQ("", s); // s was cleared. + ASSERT_FALSE(file_contents); + EXPECT_EQ("open() failed: Too many symbolic links encountered", file_contents.error()); } 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()); + auto file_contents = ReadFile("/proc/version"); + ASSERT_TRUE(file_contents); + EXPECT_GT(file_contents->length(), 6U); + EXPECT_EQ('\n', file_contents->at(file_contents->length() - 1)); + (*file_contents)[5] = 0; + EXPECT_STREQ("Linux", file_contents->c_str()); } TEST(util, WriteFileBinary) { @@ -95,29 +85,23 @@ TEST(util, WriteFileBinary) { ASSERT_EQ(10u, contents.size()); TemporaryFile tf; - std::string err; ASSERT_TRUE(tf.fd != -1); - EXPECT_TRUE(WriteFile(tf.path, contents, &err)) << strerror(errno); - EXPECT_EQ("", err); + EXPECT_TRUE(WriteFile(tf.path, contents)) << strerror(errno); - std::string read_back_contents; - 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()); + auto read_back_contents = ReadFile(tf.path); + ASSERT_TRUE(read_back_contents) << strerror(errno); + EXPECT_EQ(contents, *read_back_contents); + EXPECT_EQ(10u, read_back_contents->size()); } 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); - 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); + EXPECT_TRUE(WriteFile(path, s)); + auto file_contents = ReadFile(path); + ASSERT_TRUE(file_contents); + EXPECT_EQ(s, *file_contents); struct stat sb; int fd = open(path.c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC); EXPECT_NE(-1, fd); @@ -127,37 +111,30 @@ TEST(util, WriteFileNotExist) { } TEST(util, WriteFileExist) { - std::string s2(""); TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); - 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(WriteFile(tf.path, "2ll2", &err)); - EXPECT_EQ("", err); - EXPECT_TRUE(ReadFile(tf.path, &s2, &err)); - EXPECT_EQ("", err); - EXPECT_STREQ("2ll2", s2.c_str()); + EXPECT_TRUE(WriteFile(tf.path, "1hello1")) << strerror(errno); + auto file_contents = ReadFile(tf.path); + ASSERT_TRUE(file_contents); + EXPECT_EQ("1hello1", *file_contents); + EXPECT_TRUE(WriteFile(tf.path, "2ll2")); + file_contents = ReadFile(tf.path); + ASSERT_TRUE(file_contents); + EXPECT_EQ("2ll2", *file_contents); } TEST(util, DecodeUid) { - uid_t decoded_uid; - std::string err; + auto decoded_uid = DecodeUid("root"); + EXPECT_TRUE(decoded_uid); + EXPECT_EQ(0U, *decoded_uid); - EXPECT_TRUE(DecodeUid("root", &decoded_uid, &err)); - EXPECT_EQ("", err); - EXPECT_EQ(0U, decoded_uid); + decoded_uid = DecodeUid("toot"); + EXPECT_FALSE(decoded_uid); + EXPECT_EQ("getpwnam failed: No such file or directory", decoded_uid.error()); - EXPECT_FALSE(DecodeUid("toot", &decoded_uid, &err)); - EXPECT_EQ("getpwnam failed: No such file or directory", err); - EXPECT_EQ(UINT_MAX, decoded_uid); - - EXPECT_TRUE(DecodeUid("123", &decoded_uid, &err)); - EXPECT_EQ("", err); - EXPECT_EQ(123U, decoded_uid); + decoded_uid = DecodeUid("123"); + EXPECT_TRUE(decoded_uid); + EXPECT_EQ(123U, *decoded_uid); } TEST(util, is_dir) {