POSIX: don't allocate memory after forking.
Previously we would allocate memory in the child process. However, the
allocation might have happened while the malloc lock was held,
resulting in a deadlock.
This patch removes allocation from the child but probably makes Mac's
startup time slower until a Mac person can implement
dir_reader_posix.h.
TEST=Unittest for new code
BUG=36678
http://codereview.chromium.org/672003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41275 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: ef73044e42948214832dd11dd3326b738f816eb6
diff --git a/base/base.gyp b/base/base.gyp
index 2eec8e1..30d746d 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -61,7 +61,7 @@
'crypto/signature_verifier_unittest.cc',
'data_pack_unittest.cc',
'debug_util_unittest.cc',
- 'file_watcher_unittest.cc',
+ 'dir_reader_posix_unittest.cc',
'event_trace_consumer_win_unittest.cc',
'event_trace_controller_win_unittest.cc',
'event_trace_provider_win_unittest.cc',
@@ -70,6 +70,7 @@
'file_path_unittest.cc',
'file_util_unittest.cc',
'file_version_info_unittest.cc',
+ 'file_watcher_unittest.cc',
'gfx/rect_unittest.cc',
'gmock_unittest.cc',
'histogram_unittest.cc',
@@ -181,6 +182,7 @@
'../third_party/icu/icu.gyp:icudata',
],
'sources!': [
+ 'dir_reader_posix_unittest.cc',
'file_descriptor_shuffle_unittest.cc',
],
}, { # OS != "win"
diff --git a/base/base.gypi b/base/base.gypi
index c9d2e08..1fbb537 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -61,6 +61,9 @@
'debug_util_mac.cc',
'debug_util_posix.cc',
'debug_util_win.cc',
+ 'dir_reader_fallback.h',
+ 'dir_reader_linux.h',
+ 'dir_reader_posix.h',
'event_trace_consumer_win.h',
'event_trace_controller_win.cc',
'event_trace_controller_win.h',
diff --git a/base/dir_reader_fallback.h b/base/dir_reader_fallback.h
new file mode 100644
index 0000000..c8f02e6
--- /dev/null
+++ b/base/dir_reader_fallback.h
@@ -0,0 +1,30 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_FALLBACK_H_
+#define BASE_DIR_READER_FALLBACK_H_
+
+namespace base {
+
+class DirReaderFallback {
+ public:
+ // Open a directory. If |IsValid| is true, then |Next| can be called to start
+ // the iteration at the beginning of the directory.
+ explicit DirReaderFallback(const char* directory_path) { }
+ // After construction, IsValid returns true iff the directory was
+ // successfully opened.
+ bool IsValid() const { return false; }
+ // Move to the next entry returning false if the iteration is complete.
+ bool Next() { return false; }
+ // Return the name of the current directory entry.
+ const char* name() { return 0;}
+ // Return the file descriptor which is being used.
+ int fd() const { return -1; }
+ // Returns true if this is a no-op fallback class (for testing).
+ static bool IsFallback() { return true; }
+};
+
+} // namespace base
+
+#endif // BASE_DIR_READER_FALLBACK_H_
diff --git a/base/dir_reader_linux.h b/base/dir_reader_linux.h
new file mode 100644
index 0000000..7fd534f
--- /dev/null
+++ b/base/dir_reader_linux.h
@@ -0,0 +1,97 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_LINUX_H_
+#define BASE_DIR_READER_LINUX_H_
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+#include "base/logging.h"
+#include "base/eintr_wrapper.h"
+
+// See the comments in dir_reader_posix.h about this.
+
+namespace base {
+
+struct linux_dirent {
+ uint64_t d_ino;
+ int64_t d_off;
+ unsigned short d_reclen;
+ unsigned char d_type;
+ char d_name[0];
+};
+
+class DirReaderLinux {
+ public:
+ explicit DirReaderLinux(const char* directory_path)
+ : fd_(open(directory_path, O_RDONLY | O_DIRECTORY)),
+ offset_(0),
+ size_(0) {
+ }
+
+ ~DirReaderLinux() {
+ if (fd_ >= 0) {
+ if (HANDLE_EINTR(close(fd_)))
+ RAW_LOG(ERROR, "Failed to close directory handle");
+ }
+ }
+
+ bool IsValid() const {
+ return fd_ >= 0;
+ }
+
+ // Move to the next entry returning false if the iteration is complete.
+ bool Next() {
+ if (size_) {
+ linux_dirent* dirent = reinterpret_cast<linux_dirent*>(&buf_[offset_]);
+ offset_ += dirent->d_reclen;
+ }
+
+ if (offset_ != size_)
+ return true;
+
+ const int r = syscall(__NR_getdents64, fd_, buf_, sizeof(buf_));
+ if (r == 0)
+ return false;
+ if (r == -1) {
+ DPLOG(FATAL) << "getdents64 returned an error: " << errno;
+ return false;
+ }
+ size_ = r;
+ offset_ = 0;
+ return true;
+ }
+
+ const char* name() const {
+ if (!size_)
+ return NULL;
+
+ const linux_dirent* dirent =
+ reinterpret_cast<const linux_dirent*>(&buf_[offset_]);
+ return dirent->d_name;
+ }
+
+ int fd() const {
+ return fd_;
+ }
+
+ static bool IsFallback() {
+ return false;
+ }
+
+ private:
+ const int fd_;
+ unsigned char buf_[512];
+ size_t offset_, size_;
+
+ DISALLOW_COPY_AND_ASSIGN(DirReaderLinux);
+};
+
+} // namespace base
+
+#endif // BASE_DIR_READER_LINUX_H_
diff --git a/base/dir_reader_posix.h b/base/dir_reader_posix.h
new file mode 100644
index 0000000..e9ad11e
--- /dev/null
+++ b/base/dir_reader_posix.h
@@ -0,0 +1,30 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_DIR_READER_POSIX_H_
+#define BASE_DIR_READER_POSIX_H_
+
+#include "build/build_config.h"
+
+// This header provides a class, DirReaderPosix, which allows one to open and
+// read from directories without allocating memory. For the interface, see
+// the generic fallback in dir_reader_fallback.h.
+
+#if defined(OS_LINUX)
+#include "base/dir_reader_linux.h"
+#else
+#include "base/dir_reader_fallback.h"
+#endif
+
+namespace base {
+
+#if defined(OS_LINUX)
+typedef DirReaderLinux DirReaderPosix;
+#else
+typedef DirReaderFallback DirReaderPosix;
+#endif
+
+} // namespace base
+
+#endif // BASE_DIR_READER_POSIX_H_
diff --git a/base/dir_reader_posix_unittest.cc b/base/dir_reader_posix_unittest.cc
new file mode 100644
index 0000000..04f7243
--- /dev/null
+++ b/base/dir_reader_posix_unittest.cc
@@ -0,0 +1,91 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/dir_reader_posix.h"
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "base/logging.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using base::DirReaderPosix;
+
+namespace {
+typedef testing::Test DirReaderPosixUnittest;
+
+TEST(DirReaderPosixUnittest, Read) {
+ static const unsigned kNumFiles = 100;
+
+ if (DirReaderPosix::IsFallback())
+ return;
+
+ char kDirTemplate[] = "/tmp/org.chromium.dir-reader-posix-XXXXXX";
+ const char* dir = mkdtemp(kDirTemplate);
+ CHECK(dir);
+
+ const int prev_wd = open(".", O_RDONLY | O_DIRECTORY);
+ CHECK_GE(prev_wd, 0);
+
+ PCHECK(chdir(dir) == 0);
+
+ for (unsigned i = 0; i < kNumFiles; i++) {
+ char buf[16];
+ snprintf(buf, sizeof(buf), "%d", i);
+ const int fd = open(buf, O_CREAT | O_RDONLY | O_EXCL, 0600);
+ PCHECK(fd >= 0);
+ PCHECK(close(fd) == 0);
+ }
+
+ std::set<unsigned> seen;
+
+ DirReaderPosix reader(dir);
+ EXPECT_TRUE(reader.IsValid());
+
+ if (!reader.IsValid())
+ return;
+
+ bool seen_dot = false, seen_dotdot = false;
+
+ for (; reader.Next(); ) {
+ if (strcmp(reader.name(), ".") == 0) {
+ seen_dot = true;
+ continue;
+ }
+ if (strcmp(reader.name(), "..") == 0) {
+ seen_dotdot = true;
+ continue;
+ }
+
+ SCOPED_TRACE(testing::Message() << "reader.name(): " << reader.name());
+
+ char *endptr;
+ const unsigned long value = strtoul(reader.name(), &endptr, 10);
+
+ EXPECT_FALSE(*endptr);
+ EXPECT_LT(value, kNumFiles);
+ EXPECT_EQ(0u, seen.count(value));
+ seen.insert(value);
+ }
+
+ for (unsigned i = 0; i < kNumFiles; i++) {
+ char buf[16];
+ snprintf(buf, sizeof(buf), "%d", i);
+ PCHECK(unlink(buf) == 0);
+ }
+
+ PCHECK(rmdir(dir) == 0);
+
+ PCHECK(fchdir(prev_wd) == 0);
+ PCHECK(close(prev_wd) == 0);
+
+ EXPECT_TRUE(seen_dot);
+ EXPECT_TRUE(seen_dotdot);
+ EXPECT_EQ(kNumFiles, seen.size());
+}
+
+} // anonymous namespace
diff --git a/base/file_descriptor_shuffle.cc b/base/file_descriptor_shuffle.cc
index e722a29..2bb156b 100644
--- a/base/file_descriptor_shuffle.cc
+++ b/base/file_descriptor_shuffle.cc
@@ -12,28 +12,36 @@
namespace base {
-bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
- InjectionDelegate* delegate) {
- InjectiveMultimap m(m_in);
- std::vector<int> extra_fds;
+bool PerformInjectiveMultimapDestructive(
+ InjectiveMultimap* m, InjectionDelegate* delegate) {
+ static const size_t kMaxExtraFDs = 16;
+ int extra_fds[kMaxExtraFDs];
+ unsigned next_extra_fd = 0;
- for (InjectiveMultimap::iterator i = m.begin(); i != m.end(); ++i) {
+ // DANGER: this function may not allocate.
+
+ for (InjectiveMultimap::iterator i = m->begin(); i != m->end(); ++i) {
int temp_fd = -1;
// We DCHECK the injectiveness of the mapping.
- for (InjectiveMultimap::iterator j = i + 1; j != m.end(); ++j) {
+ for (InjectiveMultimap::iterator j = i + 1; j != m->end(); ++j) {
DCHECK(i->dest != j->dest) << "Both fd " << i->source
<< " and " << j->source << " map to " << i->dest;
}
const bool is_identity = i->source == i->dest;
- for (InjectiveMultimap::iterator j = i + 1; j != m.end(); ++j) {
+ for (InjectiveMultimap::iterator j = i + 1; j != m->end(); ++j) {
if (!is_identity && i->dest == j->source) {
if (temp_fd == -1) {
if (!delegate->Duplicate(&temp_fd, i->dest))
return false;
- extra_fds.push_back(temp_fd);
+ if (next_extra_fd < kMaxExtraFDs) {
+ extra_fds[next_extra_fd++] = temp_fd;
+ } else {
+ RAW_LOG(ERROR, "PerformInjectiveMultimapDestructive overflowed "
+ "extra_fds. Leaking file descriptors!");
+ }
}
j->source = temp_fd;
@@ -58,14 +66,18 @@
delegate->Close(i->source);
}
- for (std::vector<int>::const_iterator
- i = extra_fds.begin(); i != extra_fds.end(); ++i) {
- delegate->Close(*i);
- }
+ for (unsigned i = 0; i < next_extra_fd; i++)
+ delegate->Close(extra_fds[i]);
return true;
}
+bool PerformInjectiveMultimap(const InjectiveMultimap& m_in,
+ InjectionDelegate* delegate) {
+ InjectiveMultimap m(m_in);
+ return PerformInjectiveMultimapDestructive(&m, delegate);
+}
+
bool FileDescriptorTableInjection::Duplicate(int* result, int fd) {
*result = HANDLE_EINTR(dup(fd));
return *result >= 0;
diff --git a/base/file_descriptor_shuffle.h b/base/file_descriptor_shuffle.h
index e0cb88b..e1c93cd 100644
--- a/base/file_descriptor_shuffle.h
+++ b/base/file_descriptor_shuffle.h
@@ -69,9 +69,13 @@
bool PerformInjectiveMultimap(const InjectiveMultimap& map,
InjectionDelegate* delegate);
-static inline bool ShuffleFileDescriptors(const InjectiveMultimap& map) {
+bool PerformInjectiveMultimapDestructive(InjectiveMultimap* map,
+ InjectionDelegate* delegate);
+
+// This function will not call malloc but will mutate |map|
+static inline bool ShuffleFileDescriptors(InjectiveMultimap* map) {
FileDescriptorTableInjection delegate;
- return PerformInjectiveMultimap(map, &delegate);
+ return PerformInjectiveMultimapDestructive(map, &delegate);
}
} // namespace base
diff --git a/base/process_util.h b/base/process_util.h
index cf4bd01..f471ac0 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -111,12 +111,6 @@
#endif
#if defined(OS_POSIX)
-// Sets all file descriptors to close on exec except for stdin, stdout
-// and stderr.
-// TODO(agl): remove this function
-// WARNING: do not use. It's inherently race-prone in the face of
-// multi-threading.
-void SetAllFDsToCloseOnExec();
// Close all file descriptors, expect those which are a destination in the
// given multimap. Only call this function in a child process where you know
// that there aren't any other threads.
@@ -185,6 +179,16 @@
const file_handle_mapping_vector& fds_to_remap,
bool wait, ProcessHandle* process_handle);
+// AlterEnvironment returns a modified environment vector, constructed from the
+// given environment and the list of changes given in |changes|. Each key in
+// the environment is matched against the first element of the pairs. In the
+// event of a match, the value is replaced by the second of the pair, unless
+// the second is empty, in which case the key-value is removed.
+//
+// The returned array is allocated using new[] and must be freed by the caller.
+char** AlterEnvironment(const environment_vector& changes,
+ const char* const* const env);
+
#if defined(OS_MACOSX)
// Similar to the above, but also returns the new process's task_t if
// |task_handle| is not NULL. If |task_handle| is not NULL, the caller is
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index fdac7a2..118ee8c 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -18,6 +18,7 @@
#include "base/compiler_specific.h"
#include "base/debug_util.h"
+#include "base/dir_reader_posix.h"
#include "base/eintr_wrapper.h"
#include "base/logging.h"
#include "base/platform_thread.h"
@@ -28,8 +29,13 @@
#include "base/time.h"
#include "base/waitable_event.h"
+
#if defined(OS_MACOSX)
+#include <crt_externs.h>
+#define environ (*_NSGetEnviron())
#include "base/mach_ipc_mac.h"
+#else
+extern char** environ;
#endif
const int kMicrosecondsPerSecond = 1000000;
@@ -173,24 +179,26 @@
};
typedef scoped_ptr_malloc<DIR, ScopedDIRClose> ScopedDIR;
-void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
#if defined(OS_LINUX)
static const rlim_t kSystemDefaultMaxFds = 8192;
- static const char fd_dir[] = "/proc/self/fd";
+ static const char kFDDir[] = "/proc/self/fd";
#elif defined(OS_MACOSX)
static const rlim_t kSystemDefaultMaxFds = 256;
- static const char fd_dir[] = "/dev/fd";
+ static const char kFDDir[] = "/dev/fd";
#elif defined(OS_SOLARIS)
static const rlim_t kSystemDefaultMaxFds = 8192;
- static const char fd_dir[] = "/dev/fd";
+ static const char kFDDir[] = "/dev/fd";
#elif defined(OS_FREEBSD)
static const rlim_t kSystemDefaultMaxFds = 8192;
- static const char fd_dir[] = "/dev/fd";
+ static const char kFDDir[] = "/dev/fd";
#elif defined(OS_OPENBSD)
static const rlim_t kSystemDefaultMaxFds = 256;
- static const char fd_dir[] = "/dev/fd";
+ static const char kFDDir[] = "/dev/fd";
#endif
- std::set<int> saved_fds;
+
+void CloseSuperfluousFds(const base::InjectiveMultimap& saved_mapping) {
+ // DANGER: no calls to malloc are allowed from now on:
+ // http://crbug.com/36678
// Get the maximum number of FDs possible.
struct rlimit nofile;
@@ -206,25 +214,20 @@
if (max_fds > INT_MAX)
max_fds = INT_MAX;
- // Don't close stdin, stdout and stderr
- saved_fds.insert(STDIN_FILENO);
- saved_fds.insert(STDOUT_FILENO);
- saved_fds.insert(STDERR_FILENO);
+ DirReaderPosix fd_dir(kFDDir);
- for (base::InjectiveMultimap::const_iterator
- i = saved_mapping.begin(); i != saved_mapping.end(); ++i) {
- saved_fds.insert(i->dest);
- }
-
- ScopedDIR dir_closer(opendir(fd_dir));
- DIR *dir = dir_closer.get();
- if (NULL == dir) {
- DLOG(ERROR) << "Unable to open " << fd_dir;
-
+ if (!fd_dir.IsValid()) {
// Fallback case: Try every possible fd.
for (rlim_t i = 0; i < max_fds; ++i) {
const int fd = static_cast<int>(i);
- if (saved_fds.find(fd) != saved_fds.end())
+ if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO)
+ continue;
+ InjectiveMultimap::const_iterator i;
+ for (i = saved_mapping.begin(); i != saved_mapping.end(); i++) {
+ if (fd == i->dest)
+ break;
+ }
+ if (i != saved_mapping.end())
continue;
// Since we're just trying to close anything we can find,
@@ -233,20 +236,27 @@
}
return;
}
- int dir_fd = dirfd(dir);
- struct dirent *ent;
- while ((ent = readdir(dir))) {
+ const int dir_fd = fd_dir.fd();
+
+ for ( ; fd_dir.Next(); ) {
// Skip . and .. entries.
- if (ent->d_name[0] == '.')
+ if (fd_dir.name()[0] == '.')
continue;
char *endptr;
errno = 0;
- const long int fd = strtol(ent->d_name, &endptr, 10);
- if (ent->d_name[0] == 0 || *endptr || fd < 0 || errno)
+ const long int fd = strtol(fd_dir.name(), &endptr, 10);
+ if (fd_dir.name()[0] == 0 || *endptr || fd < 0 || errno)
continue;
- if (saved_fds.find(fd) != saved_fds.end())
+ if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO)
+ continue;
+ InjectiveMultimap::const_iterator i;
+ for (i = saved_mapping.begin(); i != saved_mapping.end(); i++) {
+ if (fd == i->dest)
+ break;
+ }
+ if (i != saved_mapping.end())
continue;
if (fd == dir_fd)
continue;
@@ -262,41 +272,6 @@
}
}
-// Sets all file descriptors to close on exec except for stdin, stdout
-// and stderr.
-// TODO(agl): Remove this function. It's fundamentally broken for multithreaded
-// apps.
-void SetAllFDsToCloseOnExec() {
-#if defined(OS_LINUX)
- const char fd_dir[] = "/proc/self/fd";
-#elif defined(OS_MACOSX) || defined(OS_FREEBSD) || defined(OS_OPENBSD) || \
- defined(OS_SOLARIS)
- const char fd_dir[] = "/dev/fd";
-#endif
- ScopedDIR dir_closer(opendir(fd_dir));
- DIR *dir = dir_closer.get();
- if (NULL == dir) {
- DLOG(ERROR) << "Unable to open " << fd_dir;
- return;
- }
-
- struct dirent *ent;
- while ((ent = readdir(dir))) {
- // Skip . and .. entries.
- if (ent->d_name[0] == '.')
- continue;
- int i = atoi(ent->d_name);
- // We don't close stdin, stdout or stderr.
- if (i <= STDERR_FILENO)
- continue;
-
- int flags = fcntl(i, F_GETFD);
- if ((flags == -1) || (fcntl(i, F_SETFD, flags | FD_CLOEXEC) == -1)) {
- DLOG(ERROR) << "fcntl failure.";
- }
- }
-}
-
#if defined(OS_MACOSX)
static std::string MachErrorCode(kern_return_t err) {
return StringPrintf("0x%x %s", err, mach_error_string(err));
@@ -358,21 +333,142 @@
}
bool LaunchApp(const std::vector<std::string>& argv,
- const environment_vector& environ,
+ const environment_vector& env_changes,
const file_handle_mapping_vector& fds_to_remap,
bool wait, ProcessHandle* process_handle) {
return LaunchAppAndGetTask(
- argv, environ, fds_to_remap, wait, NULL, process_handle);
+ argv, env_changes, fds_to_remap, wait, NULL, process_handle);
}
#endif // defined(OS_MACOSX)
+char** AlterEnvironment(const environment_vector& changes,
+ const char* const* const env) {
+ unsigned count = 0;
+ unsigned size = 0;
+
+ // First assume that all of the current environment will be included.
+ for (unsigned i = 0; env[i]; i++) {
+ const char *const pair = env[i];
+ count++;
+ size += strlen(pair) + 1 /* terminating NUL */;
+ }
+
+ for (environment_vector::const_iterator
+ j = changes.begin(); j != changes.end(); j++) {
+ bool found = false;
+ const char *pair;
+
+ for (unsigned i = 0; env[i]; i++) {
+ pair = env[i];
+ const char *const equals = strchr(pair, '=');
+ if (!equals)
+ continue;
+ const unsigned keylen = equals - pair;
+ if (keylen == j->first.size() &&
+ memcmp(pair, j->first.data(), keylen) == 0) {
+ found = true;
+ break;
+ }
+ }
+
+ // if found, we'll either be deleting or replacing this element.
+ if (found) {
+ count--;
+ size -= strlen(pair) + 1;
+ if (j->second.size())
+ found = false;
+ }
+
+ // if !found, then we have a new element to add.
+ if (!found && j->second.size() > 0) {
+ count++;
+ size += j->first.size() + 1 /* '=' */ + j->second.size() + 1 /* NUL */;
+ }
+ }
+
+ count++; // for the final NULL
+ uint8_t *buffer = new uint8_t[sizeof(char*) * count + size];
+ char **const ret = reinterpret_cast<char**>(buffer);
+ unsigned k = 0;
+ char *scratch = reinterpret_cast<char*>(buffer + sizeof(char*) * count);
+
+ for (unsigned i = 0; env[i]; i++) {
+ const char *const pair = env[i];
+ const char *const equals = strchr(pair, '=');
+ if (!equals) {
+ const unsigned len = strlen(pair);
+ ret[k++] = scratch;
+ memcpy(scratch, pair, len + 1);
+ scratch += len + 1;
+ continue;
+ }
+ const unsigned keylen = equals - pair;
+ bool handled = false;
+ for (environment_vector::const_iterator
+ j = changes.begin(); j != changes.end(); j++) {
+ if (j->first.size() == keylen &&
+ memcmp(j->first.data(), pair, keylen) == 0) {
+ if (!j->second.empty()) {
+ ret[k++] = scratch;
+ memcpy(scratch, pair, keylen + 1);
+ scratch += keylen + 1;
+ memcpy(scratch, j->second.c_str(), j->second.size() + 1);
+ scratch += j->second.size() + 1;
+ }
+ handled = true;
+ break;
+ }
+ }
+
+ if (!handled) {
+ const unsigned len = strlen(pair);
+ ret[k++] = scratch;
+ memcpy(scratch, pair, len + 1);
+ scratch += len + 1;
+ }
+ }
+
+ // Now handle new elements
+ for (environment_vector::const_iterator
+ j = changes.begin(); j != changes.end(); j++) {
+ if (j->second.size() == 0)
+ continue;
+
+ bool found = false;
+ for (unsigned i = 0; env[i]; i++) {
+ const char *const pair = env[i];
+ const char *const equals = strchr(pair, '=');
+ if (!equals)
+ continue;
+ const unsigned keylen = equals - pair;
+ if (keylen == j->first.size() &&
+ memcmp(pair, j->first.data(), keylen) == 0) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found) {
+ ret[k++] = scratch;
+ memcpy(scratch, j->first.data(), j->first.size());
+ scratch += j->first.size();
+ *scratch++ = '=';
+ memcpy(scratch, j->second.c_str(), j->second.size() + 1);
+ scratch += j->second.size() + 1;
+ }
+ }
+
+ ret[k] = NULL;
+ return ret;
+}
+
#if defined(OS_MACOSX)
bool LaunchAppAndGetTask(
#else
bool LaunchApp(
#endif
const std::vector<std::string>& argv,
- const environment_vector& environ,
+ const environment_vector& env_changes,
const file_handle_mapping_vector& fds_to_remap,
bool wait,
#if defined(OS_MACOSX)
@@ -380,6 +476,12 @@
#endif
ProcessHandle* process_handle) {
pid_t pid;
+ InjectiveMultimap fd_shuffle1, fd_shuffle2;
+ fd_shuffle1.reserve(fds_to_remap.size());
+ fd_shuffle2.reserve(fds_to_remap.size());
+ scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+ scoped_array<char*> new_environ(AlterEnvironment(env_changes, environ));
+
#if defined(OS_MACOSX)
if (task_handle == NULL) {
pid = fork();
@@ -403,45 +505,44 @@
RestoreDefaultExceptionHandler();
#endif
- InjectiveMultimap fd_shuffle;
+#if 0
+ // When debugging it can be helpful to check that we really aren't making
+ // any hidden calls to malloc.
+ void *malloc_thunk =
+ reinterpret_cast<void*>(reinterpret_cast<intptr_t>(malloc) & ~4095);
+ mprotect(malloc_thunk, 4096, PROT_READ | PROT_WRITE | PROT_EXEC);
+ memset(reinterpret_cast<void*>(malloc), 0xff, 8);
+#endif
+
+ // DANGER: no calls to malloc are allowed from now on:
+ // http://crbug.com/36678
+
for (file_handle_mapping_vector::const_iterator
it = fds_to_remap.begin(); it != fds_to_remap.end(); ++it) {
- fd_shuffle.push_back(InjectionArc(it->first, it->second, false));
+ fd_shuffle1.push_back(InjectionArc(it->first, it->second, false));
+ fd_shuffle2.push_back(InjectionArc(it->first, it->second, false));
}
- for (environment_vector::const_iterator it = environ.begin();
- it != environ.end(); ++it) {
- if (it->first.empty())
- continue;
-
- if (it->second.empty()) {
- unsetenv(it->first.c_str());
- } else {
- setenv(it->first.c_str(), it->second.c_str(), 1);
- }
- }
+ environ = new_environ.get();
// Obscure fork() rule: in the child, if you don't end up doing exec*(),
// you call _exit() instead of exit(). This is because _exit() does not
// call any previously-registered (in the parent) exit handlers, which
// might do things like block waiting for threads that don't even exist
// in the child.
- if (!ShuffleFileDescriptors(fd_shuffle))
+
+ // fd_shuffle1 is mutated by this call because it cannot malloc.
+ if (!ShuffleFileDescriptors(&fd_shuffle1))
_exit(127);
- // If we are using the SUID sandbox, it sets a magic environment variable
- // ("SBX_D"), so we remove that variable from the environment here on the
- // off chance that it's already set.
- unsetenv("SBX_D");
+ CloseSuperfluousFds(fd_shuffle2);
- CloseSuperfluousFds(fd_shuffle);
-
- scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
for (size_t i = 0; i < argv.size(); i++)
argv_cstr[i] = const_cast<char*>(argv[i].c_str());
argv_cstr[argv.size()] = NULL;
execvp(argv_cstr[0], argv_cstr.get());
- PLOG(ERROR) << "LaunchApp: execvp(" << argv_cstr[0] << ") failed";
+ RAW_LOG(ERROR, "LaunchApp: failed to execvp:");
+ RAW_LOG(ERROR, argv_cstr[0]);
_exit(127);
} else {
// Parent process
@@ -634,6 +735,12 @@
bool do_search_path) {
int pipe_fd[2];
pid_t pid;
+ InjectiveMultimap fd_shuffle1, fd_shuffle2;
+ const std::vector<std::string>& argv = cl.argv();
+ scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
+
+ fd_shuffle1.reserve(3);
+ fd_shuffle2.reserve(3);
// Either |do_search_path| should be false or |envp| should be null, but not
// both.
@@ -652,6 +759,8 @@
#if defined(OS_MACOSX)
RestoreDefaultExceptionHandler();
#endif
+ // DANGER: no calls to malloc are allowed from now on:
+ // http://crbug.com/36678
// Obscure fork() rule: in the child, if you don't end up doing exec*(),
// you call _exit() instead of exit(). This is because _exit() does not
@@ -662,18 +771,20 @@
if (dev_null < 0)
_exit(127);
- InjectiveMultimap fd_shuffle;
- fd_shuffle.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
- fd_shuffle.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
- fd_shuffle.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
+ fd_shuffle1.push_back(InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
+ fd_shuffle1.push_back(InjectionArc(dev_null, STDERR_FILENO, true));
+ fd_shuffle1.push_back(InjectionArc(dev_null, STDIN_FILENO, true));
+ // Adding another element here? Remeber to increase the argument to
+ // reserve(), above.
- if (!ShuffleFileDescriptors(fd_shuffle))
+ std::copy(fd_shuffle1.begin(), fd_shuffle1.end(),
+ std::back_inserter(fd_shuffle2));
+
+ if (!ShuffleFileDescriptors(&fd_shuffle1))
_exit(127);
- CloseSuperfluousFds(fd_shuffle);
+ CloseSuperfluousFds(fd_shuffle2);
- const std::vector<std::string> argv = cl.argv();
- scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
for (size_t i = 0; i < argv.size(); i++)
argv_cstr[i] = const_cast<char*>(argv[i].c_str());
argv_cstr[argv.size()] = NULL;
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 3170b6a..7b9a7af 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -294,6 +294,107 @@
DPCHECK(ret == 0);
}
+static std::string TestLaunchApp(const base::environment_vector& env_changes) {
+ std::vector<std::string> args;
+ base::file_handle_mapping_vector fds_to_remap;
+ ProcessHandle handle;
+
+ args.push_back("bash");
+ args.push_back("-c");
+ args.push_back("echo $BASE_TEST");
+
+ int fds[2];
+ PCHECK(pipe(fds) == 0);
+
+ fds_to_remap.push_back(std::make_pair(fds[1], 1));
+ EXPECT_TRUE(LaunchApp(args, env_changes, fds_to_remap,
+ true /* wait for exit */, &handle));
+ PCHECK(close(fds[1]) == 0);
+
+ char buf[512];
+ const ssize_t n = HANDLE_EINTR(read(fds[0], buf, sizeof(buf)));
+ PCHECK(n > 0);
+ return std::string(buf, n);
+}
+
+static const char kLargeString[] =
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789"
+ "0123456789012345678901234567890123456789012345678901234567890123456789";
+
+TEST_F(ProcessUtilTest, LaunchApp) {
+ base::environment_vector env_changes;
+
+ env_changes.push_back(std::make_pair(std::string("BASE_TEST"),
+ std::string("bar")));
+ EXPECT_EQ("bar\n", TestLaunchApp(env_changes));
+ env_changes.clear();
+
+ EXPECT_EQ(0, setenv("BASE_TEST", "testing", 1 /* override */));
+ EXPECT_EQ("testing\n", TestLaunchApp(env_changes));
+
+ env_changes.push_back(std::make_pair(std::string("BASE_TEST"),
+ std::string("")));
+ EXPECT_EQ("\n", TestLaunchApp(env_changes));
+
+ env_changes[0].second = "foo";
+ EXPECT_EQ("foo\n", TestLaunchApp(env_changes));
+
+ env_changes.clear();
+ EXPECT_EQ(0, setenv("BASE_TEST", kLargeString, 1 /* override */));
+ EXPECT_EQ(std::string(kLargeString) + "\n", TestLaunchApp(env_changes));
+
+ env_changes.push_back(std::make_pair(std::string("BASE_TEST"),
+ std::string("wibble")));
+ EXPECT_EQ("wibble\n", TestLaunchApp(env_changes));
+}
+
+TEST_F(ProcessUtilTest, AlterEnvironment) {
+ static const char* empty[] = { NULL };
+ static const char* a2[] = { "A=2", NULL };
+ base::environment_vector changes;
+ char** e;
+
+ e = AlterEnvironment(changes, empty);
+ EXPECT_TRUE(e[0] == NULL);
+ delete[] e;
+
+ changes.push_back(std::make_pair(std::string("A"), std::string("1")));
+ e = AlterEnvironment(changes, empty);
+ EXPECT_EQ(std::string("A=1"), e[0]);
+ EXPECT_TRUE(e[1] == NULL);
+ delete[] e;
+
+ changes.clear();
+ changes.push_back(std::make_pair(std::string("A"), std::string("")));
+ e = AlterEnvironment(changes, empty);
+ EXPECT_TRUE(e[0] == NULL);
+ delete[] e;
+
+ changes.clear();
+ e = AlterEnvironment(changes, a2);
+ EXPECT_EQ(std::string("A=2"), e[0]);
+ EXPECT_TRUE(e[1] == NULL);
+ delete[] e;
+
+ changes.clear();
+ changes.push_back(std::make_pair(std::string("A"), std::string("1")));
+ e = AlterEnvironment(changes, a2);
+ EXPECT_EQ(std::string("A=1"), e[0]);
+ EXPECT_TRUE(e[1] == NULL);
+ delete[] e;
+
+ changes.clear();
+ changes.push_back(std::make_pair(std::string("A"), std::string("")));
+ e = AlterEnvironment(changes, a2);
+ EXPECT_TRUE(e[0] == NULL);
+ delete[] e;
+}
+
TEST_F(ProcessUtilTest, GetAppOutput) {
std::string output;
EXPECT_TRUE(GetAppOutput(CommandLine(FilePath("true")), &output));