Prevent subprocesses from accessing FDs opened in other threads.
Unlike installd, artd is multi-threaded. When we fork & exec, the
subprocess gets FDs opened in all threads. This is unexpected. For
example, profman gets FDs opened for dex2oat. This CL prevents this by
closing other FDs before we exec.
Bug: 262230400
Test: m test-art-host-gtest-art_artd_tests
Test: m test-art-host-gtest-art_libarttools_tests
Test: atest ArtGtestsTargetChroot:ArtExecTest
Test: -
1. adb shell setprop pm.dexopt.bg-dexopt.concurrency 4
2. adb shell pm art optimize-packages bg-dexopt
3. No longer see SELinux complaining that profman is trying to read
some files opened for dex2oat.
Ignore-AOSP-First: ART Services.
Change-Id: Ia8068d804294debe0de6c947f3878da4dbf8c8ca
diff --git a/artd/artd.cc b/artd/artd.cc
index c41e9d5..9b005ad 100644
--- a/artd/artd.cc
+++ b/artd/artd.cc
@@ -327,6 +327,14 @@
void Add(const NewFile& file) { fd_mapping_.emplace_back(file.Fd(), file.TempPath()); }
void Add(const File& file) { fd_mapping_.emplace_back(file.Fd(), file.GetPath()); }
+ std::string GetFds() {
+ std::vector<int> fds;
+ for (const auto& [fd, path] : fd_mapping_) {
+ fds.push_back(fd);
+ }
+ return Join(fds, ':');
+ }
+
private:
std::vector<std::pair<int, std::string>> fd_mapping_;
@@ -415,12 +423,13 @@
std::string profile_path = OR_RETURN_FATAL(BuildProfileOrDmPath(in_profile));
OR_RETURN_FATAL(ValidateDexPath(in_dexFile));
- CmdlineBuilder args;
FdLogger fd_logger;
- args.Add(OR_RETURN_FATAL(GetArtExec()))
- .Add("--drop-capabilities")
- .Add("--")
- .Add(OR_RETURN_FATAL(GetProfman()));
+
+ CmdlineBuilder art_exec_args;
+ art_exec_args.Add(OR_RETURN_FATAL(GetArtExec())).Add("--drop-capabilities");
+
+ CmdlineBuilder args;
+ args.Add(OR_RETURN_FATAL(GetProfman()));
Result<std::unique_ptr<File>> profile = OpenFileForReading(profile_path);
if (!profile.ok()) {
@@ -438,10 +447,12 @@
args.Add("--apk-fd=%d", dex_file->Fd());
fd_logger.Add(*dex_file);
- LOG(INFO) << "Running profman: " << Join(args.Get(), /*separator=*/" ")
+ art_exec_args.Add("--keep-fds=%s", fd_logger.GetFds()).Add("--").Concat(std::move(args));
+
+ LOG(INFO) << "Running profman: " << Join(art_exec_args.Get(), /*separator=*/" ")
<< "\nOpened FDs: " << fd_logger;
- Result<int> result = ExecAndReturnCode(args.Get(), kShortTimeoutSec);
+ Result<int> result = ExecAndReturnCode(art_exec_args.Get(), kShortTimeoutSec);
if (!result.ok()) {
return NonFatal("Failed to run profman: " + result.error().message());
}
@@ -465,13 +476,13 @@
std::string dst_path = OR_RETURN_FATAL(BuildFinalProfilePath(in_dst->profilePath));
OR_RETURN_FATAL(ValidateDexPath(in_dexFile));
- CmdlineBuilder args;
FdLogger fd_logger;
- args.Add(OR_RETURN_FATAL(GetArtExec()))
- .Add("--drop-capabilities")
- .Add("--")
- .Add(OR_RETURN_FATAL(GetProfman()))
- .Add("--copy-and-update-profile-key");
+
+ CmdlineBuilder art_exec_args;
+ art_exec_args.Add(OR_RETURN_FATAL(GetArtExec())).Add("--drop-capabilities");
+
+ CmdlineBuilder args;
+ args.Add(OR_RETURN_FATAL(GetProfman())).Add("--copy-and-update-profile-key");
Result<std::unique_ptr<File>> src = OpenFileForReading(src_path);
if (!src.ok()) {
@@ -493,10 +504,12 @@
args.Add("--reference-profile-file-fd=%d", dst->Fd());
fd_logger.Add(*dst);
- LOG(INFO) << "Running profman: " << Join(args.Get(), /*separator=*/" ")
+ art_exec_args.Add("--keep-fds=%s", fd_logger.GetFds()).Add("--").Concat(std::move(args));
+
+ LOG(INFO) << "Running profman: " << Join(art_exec_args.Get(), /*separator=*/" ")
<< "\nOpened FDs: " << fd_logger;
- Result<int> result = ExecAndReturnCode(args.Get(), kShortTimeoutSec);
+ Result<int> result = ExecAndReturnCode(art_exec_args.Get(), kShortTimeoutSec);
if (!result.ok()) {
return NonFatal("Failed to run profman: " + result.error().message());
}
@@ -595,12 +608,13 @@
return Fatal("Only one of 'forceMerge', 'dumpOnly', and 'dumpClassesAndMethods' can be set");
}
- CmdlineBuilder args;
FdLogger fd_logger;
- args.Add(OR_RETURN_FATAL(GetArtExec()))
- .Add("--drop-capabilities")
- .Add("--")
- .Add(OR_RETURN_FATAL(GetProfman()));
+
+ CmdlineBuilder art_exec_args;
+ art_exec_args.Add(OR_RETURN_FATAL(GetArtExec())).Add("--drop-capabilities");
+
+ CmdlineBuilder args;
+ args.Add(OR_RETURN_FATAL(GetProfman()));
std::vector<std::unique_ptr<File>> profile_files;
for (const std::string& profile_path : profile_paths) {
@@ -668,10 +682,12 @@
.AddIf(in_options.forBootImage, "--boot-image-merge");
}
- LOG(INFO) << "Running profman: " << Join(args.Get(), /*separator=*/" ")
+ art_exec_args.Add("--keep-fds=%s", fd_logger.GetFds()).Add("--").Concat(std::move(args));
+
+ LOG(INFO) << "Running profman: " << Join(art_exec_args.Get(), /*separator=*/" ")
<< "\nOpened FDs: " << fd_logger;
- Result<int> result = ExecAndReturnCode(args.Get(), kShortTimeoutSec);
+ Result<int> result = ExecAndReturnCode(art_exec_args.Get(), kShortTimeoutSec);
if (!result.ok()) {
return NonFatal("Failed to run profman: " + result.error().message());
}
@@ -772,19 +788,14 @@
std::string oat_dir_path;
OR_RETURN_NON_FATAL(PrepareArtifactsDirs(in_outputArtifacts, &oat_dir_path));
- CmdlineBuilder args;
- args.Add(OR_RETURN_FATAL(GetArtExec())).Add("--drop-capabilities");
-
- if (in_priorityClass < PriorityClass::BOOT) {
- args.Add(in_priorityClass <= PriorityClass::BACKGROUND ?
- "--set-task-profile=Dex2OatBackground" :
- "--set-task-profile=Dex2OatBootComplete")
- .Add("--set-priority=background");
- }
-
- args.Add("--").Add(OR_RETURN_FATAL(GetDex2Oat()));
FdLogger fd_logger;
+ CmdlineBuilder art_exec_args;
+ art_exec_args.Add(OR_RETURN_FATAL(GetArtExec())).Add("--drop-capabilities");
+
+ CmdlineBuilder args;
+ args.Add(OR_RETURN_FATAL(GetDex2Oat()));
+
const FsPermission& fs_permission = in_outputArtifacts.permissionSettings.fileFsPermission;
std::unique_ptr<File> dex_file = OR_RETURN_NON_FATAL(OpenFileForReading(in_dexFile));
@@ -900,9 +911,11 @@
AddBootImageFlags(args);
AddCompilerConfigFlags(
in_instructionSet, in_compilerFilter, in_priorityClass, in_dexoptOptions, args);
- AddPerfConfigFlags(in_priorityClass, args);
+ AddPerfConfigFlags(in_priorityClass, art_exec_args, args);
- LOG(INFO) << "Running dex2oat: " << Join(args.Get(), /*separator=*/" ")
+ art_exec_args.Add("--keep-fds=%s", fd_logger.GetFds()).Add("--").Concat(std::move(args));
+
+ LOG(INFO) << "Running dex2oat: " << Join(art_exec_args.Get(), /*separator=*/" ")
<< "\nOpened FDs: " << fd_logger;
ExecCallbacks callbacks{
@@ -925,7 +938,7 @@
};
ProcessStat stat;
- Result<int> result = ExecAndReturnCode(args.Get(), kLongTimeoutSec, callbacks, &stat);
+ Result<int> result = ExecAndReturnCode(art_exec_args.Get(), kLongTimeoutSec, callbacks, &stat);
_aidl_return->wallTimeMs = stat.wall_time_ms;
_aidl_return->cpuTimeMs = stat.cpu_time_ms;
if (!result.ok()) {
@@ -1161,7 +1174,9 @@
.AddRuntimeIf(dexopt_options.hiddenApiPolicyEnabled, "-Xhidden-api-policy:enabled");
}
-void Artd::AddPerfConfigFlags(PriorityClass priority_class, /*out*/ CmdlineBuilder& args) {
+void Artd::AddPerfConfigFlags(PriorityClass priority_class,
+ /*out*/ CmdlineBuilder& art_exec_args,
+ /*out*/ CmdlineBuilder& dex2oat_args) {
// CPU set and number of threads.
std::string default_cpu_set_prop = "dalvik.vm.dex2oat-cpu-set";
std::string default_threads_prop = "dalvik.vm.dex2oat-threads";
@@ -1180,15 +1195,22 @@
cpu_set = props_->GetOrEmpty(default_cpu_set_prop);
threads = props_->GetOrEmpty(default_threads_prop);
}
- args.AddIfNonEmpty("--cpu-set=%s", cpu_set).AddIfNonEmpty("-j%s", threads);
+ dex2oat_args.AddIfNonEmpty("--cpu-set=%s", cpu_set).AddIfNonEmpty("-j%s", threads);
- args.AddRuntimeIfNonEmpty("-Xms%s", props_->GetOrEmpty("dalvik.vm.dex2oat-Xms"))
+ if (priority_class < PriorityClass::BOOT) {
+ art_exec_args
+ .Add(priority_class <= PriorityClass::BACKGROUND ? "--set-task-profile=Dex2OatBackground" :
+ "--set-task-profile=Dex2OatBootComplete")
+ .Add("--set-priority=background");
+ }
+
+ dex2oat_args.AddRuntimeIfNonEmpty("-Xms%s", props_->GetOrEmpty("dalvik.vm.dex2oat-Xms"))
.AddRuntimeIfNonEmpty("-Xmx%s", props_->GetOrEmpty("dalvik.vm.dex2oat-Xmx"));
// Enable compiling dex files in isolation on low ram devices.
// It takes longer but reduces the memory footprint.
- args.AddIf(props_->GetBool("ro.config.low_ram", /*default_value=*/false),
- "--compile-individually");
+ dex2oat_args.AddIf(props_->GetBool("ro.config.low_ram", /*default_value=*/false),
+ "--compile-individually");
}
Result<int> Artd::ExecAndReturnCode(const std::vector<std::string>& args,
diff --git a/artd/artd.h b/artd/artd.h
index 484c281..f0288da 100644
--- a/artd/artd.h
+++ b/artd/artd.h
@@ -202,6 +202,7 @@
/*out*/ art::tools::CmdlineBuilder& args);
void AddPerfConfigFlags(aidl::com::android::server::art::PriorityClass priority_class,
+ /*out*/ art::tools::CmdlineBuilder& art_exec_args,
/*out*/ art::tools::CmdlineBuilder& args);
android::base::Result<struct stat> Fstat(const art::File& file) const;
diff --git a/artd/artd_test.cc b/artd/artd_test.cc
index 8eab8ad..ca1a136 100644
--- a/artd/artd_test.cc
+++ b/artd/artd_test.cc
@@ -33,9 +33,12 @@
#include <string>
#include <thread>
#include <type_traits>
+#include <unordered_set>
+#include <utility>
#include <vector>
#include "aidl/com/android/server/art/BnArtd.h"
+#include "android-base/collections.h"
#include "android-base/errors.h"
#include "android-base/file.h"
#include "android-base/logging.h"
@@ -45,6 +48,7 @@
#include "android-base/strings.h"
#include "android/binder_auto_utils.h"
#include "android/binder_status.h"
+#include "base/array_ref.h"
#include "base/common_art_test.h"
#include "exec_utils.h"
#include "fmt/format.h"
@@ -52,6 +56,7 @@
#include "gtest/gtest.h"
#include "path_utils.h"
#include "profman/profman_result.h"
+#include "testing.h"
#include "tools/system_properties.h"
namespace art {
@@ -70,6 +75,7 @@
using ::aidl::com::android::server::art::PriorityClass;
using ::aidl::com::android::server::art::ProfilePath;
using ::aidl::com::android::server::art::VdexPath;
+using ::android::base::Append;
using ::android::base::Error;
using ::android::base::make_scope_guard;
using ::android::base::ParseInt;
@@ -98,6 +104,7 @@
using ::testing::ResultOf;
using ::testing::Return;
using ::testing::SetArgPointee;
+using ::testing::UnorderedElementsAreArray;
using ::testing::WithArg;
using PrimaryCurProfilePath = ProfilePath::PrimaryCurProfilePath;
@@ -125,26 +132,44 @@
expected_value);
}
-void WriteToFdFlagImpl(const std::vector<std::string>& args,
- const std::string& flag,
- const std::string& content,
- bool assume_empty) {
+Result<std::vector<std::string>> GetFlagValues(ArrayRef<const std::string> args,
+ std::string_view flag) {
+ std::vector<std::string> values;
for (const std::string& arg : args) {
std::string_view value(arg);
if (android::base::ConsumePrefix(&value, flag)) {
- int fd;
- ASSERT_TRUE(ParseInt(std::string(value), &fd));
- if (assume_empty) {
- ASSERT_EQ(lseek(fd, /*offset=*/0, SEEK_CUR), 0);
- } else {
- ASSERT_EQ(ftruncate(fd, /*length=*/0), 0);
- ASSERT_EQ(lseek(fd, /*offset=*/0, SEEK_SET), 0);
- }
- ASSERT_TRUE(WriteStringToFd(content, fd));
- return;
+ values.emplace_back(value);
}
}
- FAIL() << "Flag '{}' not found"_format(flag);
+ if (values.empty()) {
+ return Errorf("Flag '{}' not found", flag);
+ }
+ return values;
+}
+
+Result<std::string> GetFlagValue(ArrayRef<const std::string> args, std::string_view flag) {
+ std::vector<std::string> flag_values = OR_RETURN(GetFlagValues(args, flag));
+ if (flag_values.size() > 1) {
+ return Errorf("Duplicate flag '{}'", flag);
+ }
+ return flag_values[0];
+}
+
+void WriteToFdFlagImpl(const std::vector<std::string>& args,
+ std::string_view flag,
+ std::string_view content,
+ bool assume_empty) {
+ std::string value = OR_FAIL(GetFlagValue(ArrayRef<const std::string>(args), flag));
+ ASSERT_NE(value, "");
+ int fd;
+ ASSERT_TRUE(ParseInt(value, &fd));
+ if (assume_empty) {
+ ASSERT_EQ(lseek(fd, /*offset=*/0, SEEK_CUR), 0);
+ } else {
+ ASSERT_EQ(ftruncate(fd, /*length=*/0), 0);
+ ASSERT_EQ(lseek(fd, /*offset=*/0, SEEK_SET), 0);
+ }
+ ASSERT_TRUE(WriteStringToFd(content, fd));
}
// Writes `content` to the FD specified by the `flag`.
@@ -199,17 +224,52 @@
return ExplainMatchResult(matcher, actual_content, result_listener);
}
+template <typename T, typename U>
+Result<std::pair<ArrayRef<const T>, ArrayRef<const T>>> SplitBy(const std::vector<T>& list,
+ const U& separator) {
+ auto it = std::find(list.begin(), list.end(), separator);
+ if (it == list.end()) {
+ return Errorf("'{}' not found", separator);
+ }
+ size_t pos = it - list.begin();
+ return std::make_pair(ArrayRef<const T>(list).SubArray(0, pos),
+ ArrayRef<const T>(list).SubArray(pos + 1));
+}
+
// Matches a container that, when split by `separator`, the first part matches `head_matcher`, and
// the second part matches `tail_matcher`.
MATCHER_P3(WhenSplitBy, separator, head_matcher, tail_matcher, "") {
- using Value = const typename std::remove_reference<decltype(arg)>::type::value_type;
- auto it = std::find(arg.begin(), arg.end(), separator);
- if (it == arg.end()) {
- return false;
+ auto [head, tail] = OR_MISMATCH(SplitBy(arg, separator));
+ return ExplainMatchResult(head_matcher, head, result_listener) &&
+ ExplainMatchResult(tail_matcher, tail, result_listener);
+}
+
+MATCHER_P(HasKeepFdsForImpl, fd_flags, "") {
+ auto [head, tail] = OR_MISMATCH(SplitBy(arg, "--"));
+ std::string keep_fds_value = OR_MISMATCH(GetFlagValue(head, "--keep-fds="));
+ std::vector<std::string> keep_fds = Split(keep_fds_value, ":");
+ std::vector<std::string> fd_flag_values;
+ for (std::string_view fd_flag : fd_flags) {
+ for (const std::string& fd_flag_value : OR_MISMATCH(GetFlagValues(tail, fd_flag))) {
+ for (std::string& fd : Split(fd_flag_value, ":")) {
+ fd_flag_values.push_back(std::move(fd));
+ }
+ }
}
- size_t pos = it - arg.begin();
- return ExplainMatchResult(head_matcher, ArrayRef<Value>(arg).SubArray(0, pos), result_listener) &&
- ExplainMatchResult(tail_matcher, ArrayRef<Value>(arg).SubArray(pos + 1), result_listener);
+ return ExplainMatchResult(UnorderedElementsAreArray(fd_flag_values), keep_fds, result_listener);
+}
+
+// Matches an argument list that has the "--keep-fds=" flag before "--", whose value is a
+// semicolon-separated list that contains exactly the values of the given flags after "--".
+//
+// E.g., if the flags after "--" are "--foo=1", "--bar=2:3", "--baz=4", "--baz=5", and the matcher
+// is `HasKeepFdsFor("--foo=", "--bar=", "--baz=")`, then it requires the "--keep-fds=" flag before
+// "--" to contain exactly 1, 2, 3, 4, and 5.
+template <typename... Args>
+auto HasKeepFdsFor(Args&&... args) {
+ std::vector<std::string_view> fd_flags;
+ Append(fd_flags, std::forward<Args>(args)...);
+ return HasKeepFdsForImpl(fd_flags);
}
class MockSystemProperties : public tools::SystemProperties {
@@ -520,42 +580,57 @@
}
TEST_F(ArtdTest, dexopt) {
+ dexopt_options_.generateAppImage = true;
+
EXPECT_CALL(
*mock_exec_utils_,
DoExecAndReturnCode(
- WhenSplitBy(
- "--",
- AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
- AllOf(
- Contains(art_root_ + "/bin/dex2oat32"),
- Contains(Flag("--zip-fd=", FdOf(dex_file_))),
- Contains(Flag("--zip-location=", dex_file_)),
- Contains(Flag("--oat-location=", scratch_path_ + "/a/oat/arm64/b.odex")),
- Contains(Flag("--instruction-set=", "arm64")),
- Contains(Flag("--compiler-filter=", "speed")),
- Contains(Flag("--profile-file-fd=",
- FdOf(android_data_ +
- "/misc/profiles/ref/com.android.foo/primary.prof.12345.tmp"))),
- Contains(Flag("--input-vdex-fd=", FdOf(scratch_path_ + "/a/oat/arm64/b.vdex"))),
- Contains(Flag("--dm-fd=", FdOf(scratch_path_ + "/a/b.dm"))))),
+ AllOf(WhenSplitBy(
+ "--",
+ AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
+ AllOf(Contains(art_root_ + "/bin/dex2oat32"),
+ Contains(Flag("--zip-fd=", FdOf(dex_file_))),
+ Contains(Flag("--zip-location=", dex_file_)),
+ Contains(Flag("--oat-location=", scratch_path_ + "/a/oat/arm64/b.odex")),
+ Contains(Flag("--instruction-set=", "arm64")),
+ Contains(Flag("--compiler-filter=", "speed")),
+ Contains(Flag(
+ "--profile-file-fd=",
+ FdOf(android_data_ +
+ "/misc/profiles/ref/com.android.foo/primary.prof.12345.tmp"))),
+ Contains(Flag("--input-vdex-fd=",
+ FdOf(scratch_path_ + "/a/oat/arm64/b.vdex"))),
+ Contains(Flag("--dm-fd=", FdOf(scratch_path_ + "/a/b.dm"))))),
+ HasKeepFdsFor("--zip-fd=",
+ "--profile-file-fd=",
+ "--input-vdex-fd=",
+ "--dm-fd=",
+ "--oat-fd=",
+ "--output-vdex-fd=",
+ "--app-image-fd=",
+ "--class-loader-context-fds=",
+ "--swap-fd=")),
_,
_))
.WillOnce(DoAll(WithArg<0>(WriteToFdFlag("--oat-fd=", "oat")),
WithArg<0>(WriteToFdFlag("--output-vdex-fd=", "vdex")),
+ WithArg<0>(WriteToFdFlag("--app-image-fd=", "art")),
SetArgPointee<2>(ProcessStat{.wall_time_ms = 100, .cpu_time_ms = 400}),
Return(0)));
RunDexopt(EX_NONE,
AllOf(Field(&DexoptResult::cancelled, false),
Field(&DexoptResult::wallTimeMs, 100),
Field(&DexoptResult::cpuTimeMs, 400),
- Field(&DexoptResult::sizeBytes, strlen("oat") + strlen("vdex")),
+ Field(&DexoptResult::sizeBytes, strlen("art") + strlen("oat") + strlen("vdex")),
Field(&DexoptResult::sizeBeforeBytes,
strlen("old_art") + strlen("old_oat") + strlen("old_vdex"))));
CheckContent(scratch_path_ + "/a/oat/arm64/b.odex", "oat");
CheckContent(scratch_path_ + "/a/oat/arm64/b.vdex", "vdex");
+ CheckContent(scratch_path_ + "/a/oat/arm64/b.art", "art");
CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.odex", true);
CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.vdex", true);
+ CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.art", true);
}
TEST_F(ArtdTest, dexoptClassLoaderContext) {
@@ -683,7 +758,12 @@
_,
_))
.WillOnce(Return(0));
- RunDexopt();
+
+ // `sizeBeforeBytes` should include the size of the old ART file even if no new ART file is
+ // generated.
+ RunDexopt(EX_NONE,
+ Field(&DexoptResult::sizeBeforeBytes,
+ strlen("old_art") + strlen("old_oat") + strlen("old_vdex")));
}
TEST_F(ArtdTest, dexoptDexoptOptions2) {
@@ -702,13 +782,13 @@
AllOf(Contains(Flag("--compilation-reason=", "bg-dexopt")),
Contains(Flag("-Xtarget-sdk-version:", "456")),
Contains("--debuggable"),
+ Contains(Flag("--app-image-fd=", _)),
Contains(Flag("-Xhidden-api-policy:", "enabled")))),
_,
_))
- .WillOnce(DoAll(WithArg<0>(WriteToFdFlag("--app-image-fd=", "art")), Return(0)));
- RunDexopt();
+ .WillOnce(Return(0));
- CheckContent(scratch_path_ + "/a/oat/arm64/b.art", "art");
+ RunDexopt();
}
TEST_F(ArtdTest, dexoptDefaultFlagsWhenNoSystemProps) {
@@ -1155,11 +1235,13 @@
EXPECT_CALL(
*mock_exec_utils_,
DoExecAndReturnCode(
- WhenSplitBy("--",
- AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
- AllOf(Contains(art_root_ + "/bin/profman"),
- Contains(Flag("--reference-profile-file-fd=", FdOf(profile_file))),
- Contains(Flag("--apk-fd=", FdOf(dex_file_))))),
+ AllOf(WhenSplitBy(
+ "--",
+ AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
+ AllOf(Contains(art_root_ + "/bin/profman"),
+ Contains(Flag("--reference-profile-file-fd=", FdOf(profile_file))),
+ Contains(Flag("--apk-fd=", FdOf(dex_file_))))),
+ HasKeepFdsFor("--reference-profile-file-fd=", "--apk-fd=")),
_,
_))
.WillOnce(Return(ProfmanResult::kSkipCompilationSmallDelta));
@@ -1218,12 +1300,14 @@
EXPECT_CALL(
*mock_exec_utils_,
DoExecAndReturnCode(
- WhenSplitBy("--",
- AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
- AllOf(Contains(art_root_ + "/bin/profman"),
- Contains("--copy-and-update-profile-key"),
- Contains(Flag("--profile-file-fd=", FdOf(src_file))),
- Contains(Flag("--apk-fd=", FdOf(dex_file_))))),
+ AllOf(WhenSplitBy(
+ "--",
+ AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
+ AllOf(Contains(art_root_ + "/bin/profman"),
+ Contains("--copy-and-update-profile-key"),
+ Contains(Flag("--profile-file-fd=", FdOf(src_file))),
+ Contains(Flag("--apk-fd=", FdOf(dex_file_))))),
+ HasKeepFdsFor("--profile-file-fd=", "--reference-profile-file-fd=", "--apk-fd=")),
_,
_))
.WillOnce(DoAll(WithArg<0>(WriteToFdFlag("--reference-profile-file-fd=", "def")),
@@ -1504,16 +1588,18 @@
EXPECT_CALL(
*mock_exec_utils_,
DoExecAndReturnCode(
- WhenSplitBy("--",
- AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
- AllOf(Contains(art_root_ + "/bin/profman"),
- Not(Contains(Flag("--profile-file-fd=", FdOf(profile_0_file)))),
- Contains(Flag("--profile-file-fd=", FdOf(profile_1_file))),
- Contains(Flag("--reference-profile-file-fd=", FdHasContent("abc"))),
- Contains(Flag("--apk-fd=", FdOf(dex_file_1))),
- Contains(Flag("--apk-fd=", FdOf(dex_file_2))),
- Not(Contains("--force-merge")),
- Not(Contains("--boot-image-merge")))),
+ AllOf(WhenSplitBy(
+ "--",
+ AllOf(Contains(art_root_ + "/bin/art_exec"), Contains("--drop-capabilities")),
+ AllOf(Contains(art_root_ + "/bin/profman"),
+ Not(Contains(Flag("--profile-file-fd=", FdOf(profile_0_file)))),
+ Contains(Flag("--profile-file-fd=", FdOf(profile_1_file))),
+ Contains(Flag("--reference-profile-file-fd=", FdHasContent("abc"))),
+ Contains(Flag("--apk-fd=", FdOf(dex_file_1))),
+ Contains(Flag("--apk-fd=", FdOf(dex_file_2))),
+ Not(Contains("--force-merge")),
+ Not(Contains("--boot-image-merge")))),
+ HasKeepFdsFor("--profile-file-fd=", "--reference-profile-file-fd=", "--apk-fd=")),
_,
_))
.WillOnce(DoAll(WithArg<0>(ClearAndWriteToFdFlag("--reference-profile-file-fd=", "merged")),
@@ -1664,10 +1750,11 @@
EXPECT_CALL(*mock_exec_utils_,
DoExecAndReturnCode(
- WhenSplitBy("--",
- _,
- AllOf(Contains("--dump-only"),
- Not(Contains(Flag("--reference-profile-file-fd=", _))))),
+ AllOf(WhenSplitBy("--",
+ _,
+ AllOf(Contains("--dump-only"),
+ Not(Contains(Flag("--reference-profile-file-fd=", _))))),
+ HasKeepFdsFor("--profile-file-fd=", "--apk-fd=", "--dump-output-to-fd=")),
_,
_))
.WillOnce(DoAll(WithArg<0>(WriteToFdFlag("--dump-output-to-fd=", "dump")),
diff --git a/artd/testing.h b/artd/testing.h
new file mode 100644
index 0000000..df01a9a
--- /dev/null
+++ b/artd/testing.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2022 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.
+ */
+
+#ifndef ART_ARTD_TESTING_H_
+#define ART_ARTD_TESTING_H_
+
+// Returns the value of the given `android::base::Result`, or reports the error as a gMock matcher
+// mismatch. This is only to be used in a gMock matcher.
+#define OR_MISMATCH(expr) \
+ ({ \
+ decltype(expr)&& tmp__ = (expr); \
+ if (!tmp__.ok()) { \
+ *result_listener << tmp__.error().message(); \
+ return false; \
+ } \
+ std::move(tmp__).value(); \
+ })
+
+// Returns the value of the given `android::base::Result`, or fails the GoogleTest.
+#define OR_FAIL(expr) \
+ ({ \
+ decltype(expr)&& tmp__ = (expr); \
+ ASSERT_TRUE(tmp__.ok()) << tmp__.error().message(); \
+ std::move(tmp__).value(); \
+ })
+
+#endif // ART_ARTD_TESTING_H_
diff --git a/libarttools/Android.bp b/libarttools/Android.bp
index f927011..fe8916c 100644
--- a/libarttools/Android.bp
+++ b/libarttools/Android.bp
@@ -115,6 +115,7 @@
"libbase",
],
static_libs: [
+ "libc++fs",
"libcap",
],
apex_available: [
diff --git a/libarttools/tools/art_exec.cc b/libarttools/tools/art_exec.cc
index 48dadb5..886ea27 100644
--- a/libarttools/tools/art_exec.cc
+++ b/libarttools/tools/art_exec.cc
@@ -18,18 +18,23 @@
#include <sys/resource.h>
#include <unistd.h>
+#include <filesystem>
#include <iostream>
#include <iterator>
#include <optional>
#include <string>
#include <string_view>
+#include <system_error>
+#include <unordered_set>
#include <vector>
#include "android-base/logging.h"
+#include "android-base/parseint.h"
#include "android-base/result.h"
#include "android-base/strings.h"
#include "base/macros.h"
#include "base/scoped_cap.h"
+#include "fmt/format.h"
#include "processgroup/processgroup.h"
#include "system/thread_defs.h"
@@ -37,12 +42,18 @@
using ::android::base::ConsumePrefix;
using ::android::base::Join;
+using ::android::base::ParseInt;
using ::android::base::Result;
using ::android::base::Split;
+using ::fmt::literals::operator""_format; // NOLINT
+
constexpr const char* kUsage =
R"(A wrapper binary that configures the process and executes a command.
+By default, it closes all open file descriptors except stdin, stdout, and stderr. `--keep-fds` can
+be passed to keep some more file descriptors open.
+
Usage: art_exec [OPTIONS]... -- [COMMAND]...
Supported options:
@@ -54,6 +65,7 @@
PRIORITY is "background".
--drop-capabilities: Drop all root capabilities. Note that this has effect only if `art_exec` runs
with some root capabilities but not as the root user.
+ --keep-fds=FILE_DESCRIPTORS: A semicolon-separated list of file descriptors to keep open.
)";
constexpr int kErrorUsage = 100;
@@ -64,6 +76,7 @@
std::vector<std::string> task_profiles;
std::optional<int> priority = std::nullopt;
bool drop_capabilities = false;
+ std::unordered_set<int> keep_fds{fileno(stdin), fileno(stdout), fileno(stderr)};
};
[[noreturn]] void Usage(const std::string& error_msg) {
@@ -92,6 +105,14 @@
}
} else if (arg == "--drop-capabilities") {
options.drop_capabilities = true;
+ } else if (ConsumePrefix(&arg, "--keep-fds=")) {
+ for (const std::string& fd_str : Split(std::string(arg), ":")) {
+ int fd;
+ if (!ParseInt(fd_str, &fd)) {
+ Usage("Invalid fd " + fd_str);
+ }
+ options.keep_fds.insert(fd);
+ }
} else if (arg == "--") {
if (i + 1 >= argc) {
Usage("Missing command after '--'");
@@ -119,6 +140,33 @@
return {};
}
+Result<void> CloseFds(const std::unordered_set<int>& keep_fds) {
+ std::vector<int> open_fds;
+ std::error_code ec;
+ for (const std::filesystem::directory_entry& dir_entry :
+ std::filesystem::directory_iterator("/proc/self/fd", ec)) {
+ int fd;
+ if (!ParseInt(dir_entry.path().filename(), &fd)) {
+ return Errorf("Invalid entry in /proc/self/fd {}", dir_entry.path().filename());
+ }
+ open_fds.push_back(fd);
+ }
+ if (ec) {
+ return Errorf("Failed to list open FDs: {}", ec.message());
+ }
+ for (int fd : open_fds) {
+ if (keep_fds.find(fd) == keep_fds.end()) {
+ if (close(fd) != 0) {
+ Result<void> error = ErrnoErrorf("Failed to close FD {}", fd);
+ if (std::filesystem::exists("/proc/self/fd/{}"_format(fd))) {
+ return error;
+ }
+ }
+ }
+ }
+ return {};
+}
+
} // namespace
int main(int argc, char** argv) {
@@ -126,6 +174,11 @@
Options options = ParseOptions(argc, argv);
+ if (auto result = CloseFds(options.keep_fds); !result.ok()) {
+ LOG(ERROR) << "Failed to close open FDs: " << result.error();
+ return kErrorOther;
+ }
+
if (!options.task_profiles.empty()) {
if (!SetTaskProfiles(/*tid=*/0, options.task_profiles)) {
LOG(ERROR) << "Failed to set task profile";
diff --git a/libarttools/tools/art_exec_test.cc b/libarttools/tools/art_exec_test.cc
index 37a1070..4f440ef 100644
--- a/libarttools/tools/art_exec_test.cc
+++ b/libarttools/tools/art_exec_test.cc
@@ -29,6 +29,7 @@
#include "android-base/file.h"
#include "android-base/logging.h"
#include "android-base/scopeguard.h"
+#include "android-base/strings.h"
#include "base/common_art_test.h"
#include "base/file_utils.h"
#include "base/globals.h"
@@ -46,7 +47,11 @@
using ::android::base::make_scope_guard;
using ::android::base::ScopeGuard;
+using ::android::base::Split;
+using ::testing::AllOf;
+using ::testing::Contains;
using ::testing::HasSubstr;
+using ::testing::Not;
// clang-tidy incorrectly complaints about the using declaration while the user-defined literal is
// actually being used.
@@ -176,5 +181,36 @@
}
}
+TEST_F(ArtExecTest, CloseFds) {
+ std::unique_ptr<File> file1(OS::OpenFileForReading("/dev/zero"));
+ std::unique_ptr<File> file2(OS::OpenFileForReading("/dev/zero"));
+ std::unique_ptr<File> file3(OS::OpenFileForReading("/dev/zero"));
+ ASSERT_NE(file1, nullptr);
+ ASSERT_NE(file2, nullptr);
+ ASSERT_NE(file3, nullptr);
+
+ std::string filename = "/data/local/tmp/art-exec-test-XXXXXX";
+ ScratchFile scratch_file(new File(mkstemp(filename.data()), filename, /*check_usage=*/false));
+ ASSERT_GE(scratch_file.GetFd(), 0);
+
+ std::vector<std::string> args{art_exec_bin_,
+ "--keep-fds={}:{}"_format(file3->Fd(), file2->Fd()),
+ "--",
+ GetBin("sh"),
+ "-c",
+ "ls /proc/self/fd > " + filename};
+
+ std::string error_msg;
+ ASSERT_TRUE(Exec(args, &error_msg)) << error_msg;
+
+ std::string open_fds;
+ ASSERT_TRUE(android::base::ReadFileToString(filename, &open_fds));
+
+ EXPECT_THAT(Split(open_fds, "\n"),
+ AllOf(Not(Contains(std::to_string(file1->Fd()))),
+ Contains(std::to_string(file2->Fd())),
+ Contains(std::to_string(file3->Fd()))));
+}
+
} // namespace
} // namespace art
diff --git a/libarttools/tools/cmdline_builder.h b/libarttools/tools/cmdline_builder.h
index 13b79ca..fd11ee8 100644
--- a/libarttools/tools/cmdline_builder.h
+++ b/libarttools/tools/cmdline_builder.h
@@ -17,6 +17,8 @@
#ifndef ART_LIBARTTOOLS_TOOLS_CMDLINE_BUILDER_H_
#define ART_LIBARTTOOLS_TOOLS_CMDLINE_BUILDER_H_
+#include <algorithm>
+#include <iterator>
#include <string>
#include <string_view>
#include <vector>
@@ -135,6 +137,15 @@
return *this;
}
+ // Concatenates this builder with another. Returns the concatenated result and nullifies the input
+ // builder.
+ CmdlineBuilder& Concat(CmdlineBuilder&& other) {
+ elements_.reserve(elements_.size() + other.elements_.size());
+ std::move(other.elements_.begin(), other.elements_.end(), std::back_inserter(elements_));
+ other.elements_.clear();
+ return *this;
+ }
+
private:
std::vector<std::string> elements_;
};
diff --git a/libarttools/tools/cmdline_builder_test.cc b/libarttools/tools/cmdline_builder_test.cc
index 8509f73..5551860 100644
--- a/libarttools/tools/cmdline_builder_test.cc
+++ b/libarttools/tools/cmdline_builder_test.cc
@@ -16,6 +16,8 @@
#include "cmdline_builder.h"
+#include <utility>
+
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -115,6 +117,19 @@
EXPECT_THAT(args_.Get(), IsEmpty());
}
+TEST_F(CmdlineBuilderTest, Concat) {
+ args_.Add("--flag1");
+ args_.Add("--flag2");
+
+ CmdlineBuilder other;
+ other.Add("--flag3");
+ other.Add("--flag4");
+
+ args_.Concat(std::move(other));
+ EXPECT_THAT(args_.Get(), ElementsAre("--flag1", "--flag2", "--flag3", "--flag4"));
+ EXPECT_THAT(other.Get(), IsEmpty());
+}
+
} // namespace
} // namespace tools
} // namespace art