Skip to content

Commit

Permalink
Revert of Stack trace symbolization for Chrome sandbox processes. (ht…
Browse files Browse the repository at this point in the history
…tps://codereview.chromium.org/184273012/)

Reason for revert:
I believe this has broken ProcMapsTest.ParseProcMapsInvalidParameter test in base_unittests, see http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29/builds/18196

Original issue's description:
> Stack trace symbolization for Chrome sandbox processes.
> 
> Patching the POSIX implementation of base::debug::StackTrace to properly
> symbolize stack traces when used in Chrome sandbox processes. The main
> reason symbolization doesn't work today is because sandboxed processes
> cannot open proc/self/maps and the object files that contain the symbols.
> 
> The proposed solution is to open all necessary files before enabling the
> sandbox and then keep their file descriptors around for the duration of
> the sandboxed process.  This cannot be done in  shipping/retail builds
> because it has security implications so for the time being this code will
> run only in DEBUG builds of Chrome.
> 
> Originally, the code review was started under a different account.  For
> more information take a look at: https://codereview.chromium.org/156043003/
> 
> BUG=341966
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258548

TBR=jamesr@chromium.org,jln@chromium.org,ajwong@chromium.org,rsesek@chromium.org,ivanpe@google.com
NOTREECHECKS=true
NOTRY=true
BUG=341966

Review URL: https://codereview.chromium.org/208363002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@258587 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
vsevik@chromium.org committed Mar 21, 2014
1 parent baeb218 commit f458c2f
Show file tree
Hide file tree
Showing 6 changed files with 8 additions and 328 deletions.
6 changes: 1 addition & 5 deletions base/debug/proc_maps_linux.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ bool ReadProcMaps(std::string* proc_maps) {

bool ParseProcMaps(const std::string& input,
std::vector<MappedMemoryRegion>* regions_out) {
CHECK(regions_out);
std::vector<MappedMemoryRegion> regions;

// This isn't async safe nor terribly efficient, but it doesn't need to be at
Expand All @@ -102,10 +101,8 @@ bool ParseProcMaps(const std::string& input,
for (size_t i = 0; i < lines.size(); ++i) {
// Due to splitting on '\n' the last line should be empty.
if (i == lines.size() - 1) {
if (!lines[i].empty()) {
DLOG(WARNING) << "Last line not empty";
if (!lines[i].empty())
return false;
}
break;
}

Expand All @@ -128,7 +125,6 @@ bool ParseProcMaps(const std::string& input,
if (sscanf(line, "%" SCNxPTR "-%" SCNxPTR " %4c %llx %hhx:%hhx %ld %n",
&region.start, &region.end, permissions, &region.offset,
&dev_major, &dev_minor, &inode, &path_index) < 7) {
DPLOG(WARNING) << "sscanf failed for line: " << line;
return false;
}

Expand Down
38 changes: 0 additions & 38 deletions base/debug/proc_maps_linux_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -277,43 +277,5 @@ TEST(ProcMapsTest, InvalidInput) {
}
}

TEST(ProcMapsTest, ParseProcMapsInvalidParameter) {
// The default style "fast" does not support multi-threaded tests.
::testing::FLAGS_gtest_death_test_style = "threadsafe";
ASSERT_DEATH(ParseProcMaps("", NULL), "");
}

TEST(ProcMapsTest, ParseProcMapsEmptyString) {
std::vector<MappedMemoryRegion> regions;
EXPECT_TRUE(ParseProcMaps("", &regions));
EXPECT_EQ(0ULL, regions.size());
}

// Testing a couple of remotely possible weird things in the input:
// - Line ending with \r\n or \n\r.
// - File name contains quotes.
// - File name has whitespaces.
TEST(ProcMapsTest, ParseProcMapsWeirdCorrectInput) {
std::vector<MappedMemoryRegion> regions;
const std::string kContents =
"00400000-0040b000 r-xp 00000000 fc:00 2106562 "
" /bin/cat\r\n"
"7f53b7dad000-7f53b7f62000 r-xp 00000000 fc:00 263011 "
" /lib/x86_64-linux-gnu/libc-2.15.so\n\r"
"7f53b816d000-7f53b818f000 r-xp 00000000 fc:00 264284 "
" /lib/x86_64-linux-gnu/ld-2.15.so\n"
"7fff9c7ff000-7fff9c800000 r-xp 00000000 00:00 0 "
" \"vd so\"\n"
"ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 "
" [vsys call]\n";
EXPECT_TRUE(ParseProcMaps(kContents, &regions));
EXPECT_EQ(5ULL, regions.size());
EXPECT_EQ("/bin/cat", regions[0].path);
EXPECT_EQ("/lib/x86_64-linux-gnu/libc-2.15.so", regions[1].path);
EXPECT_EQ("/lib/x86_64-linux-gnu/ld-2.15.so", regions[2].path);
EXPECT_EQ("\"vd so\"", regions[3].path);
EXPECT_EQ("[vsys call]", regions[4].path);
}

} // namespace debug
} // namespace base
9 changes: 0 additions & 9 deletions base/debug/stack_trace.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,6 @@ namespace debug {
// unit_tests only! This is not thread-safe: only call from main thread.
BASE_EXPORT bool EnableInProcessStackDumping();

// A different version of EnableInProcessStackDumping that also works for
// sandboxed processes. For more details take a look at the description
// of EnableInProcessStackDumping.
// Calling this function on Linux opens /proc/self/maps and caches its
// contents. In DEBUG builds, this function also opens the object files that
// are loaded in memory and caches their file descriptors (this cannot be
// done in official builds because it has security implications).
BASE_EXPORT bool EnableInProcessStackDumpingForSandbox();

// A stacktrace can be helpful in debugging. For example, you can include a
// stacktrace member in a object (probably around #ifndef NDEBUG) so that you
// can later see where the given object was created from.
Expand Down
Loading

0 comments on commit f458c2f

Please sign in to comment.