Commit 8a5898a8 authored by Egor Pasko's avatar Egor Pasko Committed by Commit Bot

Revert "Reland: PR_SET_DUMPABLE 1 briefly if opening pagemap fails"

This reverts commit f889004d.

Reason for revert: http://crbug.com/1130358#c13

Original change's description:
> Reland: PR_SET_DUMPABLE 1 briefly if opening pagemap fails
> 
> This is often necessary on Android to read /proc/self/pagemap. Without
> prctl(PR_SET_DUMPABLE) opening this file fails in 94% of cases on
> Canary. This potentially leads to bias in reporting
> MappedAndResidentMemoryFootprint2.
> 
> What happened since it last landed as http://crrev.com/808298:
> * Setting the process back to non-dumpable was often returning an error
>   and hitting a (P)CHECK
> * In crash dumps logcat started right at the failure, so it is not
>   possible to see whether prctl(PR_SET_DUMPABLE, 1) succeeded prior to
>   the failure, my guess is that it failed too
> * In the reland, avoid prctl(SET_DUMPABLE, 0) if prctl(SET_DUMPABLE, 1)
>   failed before that
> 
> Bug: 1070618
> Change-Id: Ic9bfbdad6c72a1388eb80c3b6c9e6c772c8e431c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2421730
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: ssid <ssid@chromium.org>
> Commit-Queue: Egor Pasko <pasko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#809373}

TBR=pasko@chromium.org,ssid@chromium.org,rsesek@chromium.org

Change-Id: I3ae4695cca87eb4e878a195f84352fd5f19889d6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1070618
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2425053Reviewed-by: default avatarEgor Pasko <pasko@chromium.org>
Commit-Queue: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809744}
parent bdae1773
...@@ -5,8 +5,6 @@ ...@@ -5,8 +5,6 @@
#include <dlfcn.h> #include <dlfcn.h>
#include <fcntl.h> #include <fcntl.h>
#include <stdint.h> #include <stdint.h>
#include <sys/prctl.h>
#include <memory> #include <memory>
#include "base/android/library_loader/anchor_functions.h" #include "base/android/library_loader/anchor_functions.h"
...@@ -230,44 +228,6 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file, ...@@ -230,44 +228,6 @@ uint32_t ReadLinuxProcSmapsFile(FILE* smaps_file,
return num_valid_regions; return num_valid_regions;
} }
// RAII class making the current process dumpable via prctl(PR_SET_DUMPABLE, 1),
// in case it is not currently dumpable as described in proc(5) and prctl(2).
// Noop if the original dumpable state could not be determined.
class ScopedProcessSetDumpable {
public:
ScopedProcessSetDumpable() {
int result = prctl(PR_GET_DUMPABLE, 0, 0, 0, 0);
if (result < 0) {
PLOG(ERROR) << "prctl";
AvoidPrctlOnDestruction();
return;
}
was_dumpable_ = result > 0;
if (!was_dumpable_) {
if (prctl(PR_SET_DUMPABLE, 1, 0, 0, 0) != 0) {
PLOG(ERROR) << "prctl";
// PR_SET_DUMPABLE is often disallowed, avoid crashing in this case.
AvoidPrctlOnDestruction();
}
}
}
ScopedProcessSetDumpable(const ScopedProcessSetDumpable&) = delete;
ScopedProcessSetDumpable& operator=(const ScopedProcessSetDumpable&) = delete;
~ScopedProcessSetDumpable() {
if (!was_dumpable_) {
PCHECK(prctl(PR_SET_DUMPABLE, 0, 0, 0, 0));
}
}
private:
void AvoidPrctlOnDestruction() { was_dumpable_ = true; }
bool was_dumpable_;
};
} // namespace } // namespace
FILE* g_proc_smaps_for_testing = nullptr; FILE* g_proc_smaps_for_testing = nullptr;
...@@ -368,14 +328,8 @@ OSMetrics::MappedAndResidentPagesDumpState OSMetrics::GetMappedAndResidentPages( ...@@ -368,14 +328,8 @@ OSMetrics::MappedAndResidentPagesDumpState OSMetrics::GetMappedAndResidentPages(
base::ScopedFILE pagemap_file(fopen(kPagemap, "r")); base::ScopedFILE pagemap_file(fopen(kPagemap, "r"));
if (!pagemap_file.get()) { if (!pagemap_file.get()) {
{ DLOG(WARNING) << "Could not open " << kPagemap;
ScopedProcessSetDumpable set_dumpable; return OSMetrics::MappedAndResidentPagesDumpState::kAccessPagemapDenied;
pagemap_file.reset(fopen(kPagemap, "r"));
}
if (!pagemap_file.get()) {
DLOG(WARNING) << "Could not open " << kPagemap;
return OSMetrics::MappedAndResidentPagesDumpState::kAccessPagemapDenied;
}
} }
const size_t kPageSize = base::GetPageSize(); const size_t kPageSize = base::GetPageSize();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment