Fix memory corruption in base_unittests in some configuration. (issue 2365333002 by sdefresne@chromium.org)

sdef...@chromium.org

unread,

Sep 26, 2016, 8:43:10 AM9/26/16

to prim...@chromium.org, chromium...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Reviewers: Primiano Tucci
CL: https://codereview.chromium.org/2365333002/

Message:
Please take a look.

Description:
Fix memory corruption in base_unittests in some configuration.

On some combination of devices and version of OS (currently 64-bit iPad
running iOS 10), sysctlbyname("vm.pagesize", ...) fails. This cause the
wrapper ProcessMemoryDump::GetSystemPageSize to call base::GetPageSize,
which is incorrect when used for mincore() on iOS.

Instead use vm_kernel_page_size as recommended by Apple Staff on Apple
developer forums: https://forums.developer.apple.com/thread/47532.

BUG=542671

Affected files (+7, -13 lines):
M base/trace_event/process_memory_dump.cc

Index: base/trace_event/process_memory_dump.cc
diff --git a/base/trace_event/process_memory_dump.cc b/base/trace_event/process_memory_dump.cc
index 07142118d3e0e1f03ebf310573c99e3423ac5531..63d1340e42e3222c3ee99e92a7e676279cffb8a1 100644
--- a/base/trace_event/process_memory_dump.cc
+++ b/base/trace_event/process_memory_dump.cc
@@ -18,7 +18,7 @@
#include "build/build_config.h"

#if defined(OS_IOS)
-#include <sys/sysctl.h>
+#include <mach/vm_page_size.h>
#endif

#if defined(OS_POSIX)
@@ -57,19 +57,13 @@ bool ProcessMemoryDump::is_black_hole_non_fatal_for_testing_ = false;
size_t ProcessMemoryDump::GetSystemPageSize() {
#if defined(OS_IOS)
// On iOS, getpagesize() returns the user page sizes, but for allocating
- // arrays for mincore(), kernel page sizes is needed. sysctlbyname() should
- // be used for this. Refer to crbug.com/542671 and Apple rdar://23651782
- int pagesize;
- size_t pagesize_len;
- int status = sysctlbyname("vm.pagesize", NULL, &pagesize_len, nullptr, 0);
- if (!status && pagesize_len == sizeof(pagesize)) {
- if (!sysctlbyname("vm.pagesize", &pagesize, &pagesize_len, nullptr, 0))
- return pagesize;
- }
- LOG(ERROR) << "sysctlbyname(\"vm.pagesize\") failed.";
- // Falls back to getpagesize() although it may be wrong in certain cases.
-#endif // defined(OS_IOS)
+ // arrays for mincore(), kernel page sizes is needed. Use vm_kernel_page_size
+ // as recommended by Apple, https://forums.developer.apple.com/thread/47532/.
+ // Refer to http://crbug.com/542671 and Apple rdar://23651782
+ return vm_kernel_page_size;
+#else
return base::GetPageSize();
+#endif // defined(OS_IOS)
}

// static

sdef...@chromium.org

unread,

Sep 26, 2016, 8:43:53 AM9/26/16

to prim...@chromium.org, chromium...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

On 2016/09/26 12:43:09, sdefresne wrote:
> Please take a look.

Note that "vm.pagesize" is not documented, so I think that we should not rely on
it, and this is why I unconditionally use vm_kernel_page_size here.

https://codereview.chromium.org/2365333002/

prim...@chromium.org

unread,

Sep 26, 2016, 10:24:20 AM9/26/16

to sdef...@chromium.org, chromium...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

Feels like a deja-vu from https://codereview.chromium.org/1793943002 where
somebody pointed out it was corrupting memory and allegedly fixed this.
I have no idea how iOS works, so I'm going to trust whatever my iOS friends tell
me.
On a side note I am more and more scared by iOS, if I can't even rely on how
bigs pages are.

LGTM :)

https://codereview.chromium.org/2365333002/diff/1/base/trace_event/process_memory_dump.cc
File base/trace_event/process_memory_dump.cc (left):

https://codereview.chromium.org/2365333002/diff/1/base/trace_event/process_memory_dump.cc#oldcode72
base/trace_event/process_memory_dump.cc:72: return base::GetPageSize();
I liked this the old way, i.e.
#if special case
...
return x
#endif
return x // general case

instead of the #else,
but not a huge deal.

https://codereview.chromium.org/2365333002/

sdef...@chromium.org

unread,

Sep 26, 2016, 10:43:20 AM9/26/16

to prim...@chromium.org, chromium...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

This is exactly the same issue (in fact, I did reopen the same bug).

The issue is that iOS has two different "pagesize". The real physical page size
(returned by getpagesize() or sysctlbyname("hw.pagesize", ...). But as mentioned
in the linked thread, so low level mach API (like mincore()) use a different
pagesize. The previous fix use "vm.pagesize" to get that other pagesize, but
this function may fail, and in that case, the incorrect value returned by
getpagesize() is used leading to the same memory corruption.

The new CL use a global variable that is initialized as part of initializing the
mach runtime library
(https://opensource.apple.com/source/xnu/xnu-2050.7.9/libsyscall/mach/mach_init.c).
This is the same value returned by "vm.pagesize" when it works (though it
sometimes fails, which we noticed when we started running builds on iOS 10
iPads).

TL;DR: page size is returned by getpagesize() but mach API does not respect it
and use a different value that is exported as vm_kernel_page_size.

https://codereview.chromium.org/2365333002/

sdef...@chromium.org

unread,

Sep 26, 2016, 10:44:52 AM9/26/16

to prim...@chromium.org, chromium...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org

https://codereview.chromium.org/2365333002/diff/1/base/trace_event/process_memory_dump.cc
File base/trace_event/process_memory_dump.cc (left):

https://codereview.chromium.org/2365333002/diff/1/base/trace_event/process_memory_dump.cc#oldcode72
base/trace_event/process_memory_dump.cc:72: return base::GetPageSize();

On 2016/09/26 14:24:05, Primiano Tucci wrote:
> I liked this the old way, i.e.
> #if special case
> ...
> return x
> #endif
> return x // general case
>
> instead of the #else,
> but not a huge deal.

Won't this cause a warning by the compiler about unreachable code? And
returning base::GetPageSize() here on iOS is incorrect. I'd prefer to
make it clear that it should have a different implementation on iOS.

https://codereview.chromium.org/2365333002/