Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Disable ptrace on arm64e running iOS 14.2 and greater #22377

Closed
jmagman wants to merge 1 commit into from
Closed

Disable ptrace on arm64e running iOS 14.2 and greater #22377

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions runtime/ptrace_check.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <sys/sysctl.h>
#include <sys/types.h>

#include <mach/machine.h>
Copy link
Member

@xster xster Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be conditionally imported and used? @chinmaygarde might have a better idea of whether this is available on all our target platforms.

Copy link
Member

@chinmaygarde chinmaygarde Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. TRACING_CHECKS_NECESSARY is unset on all other platforms.

#include <mutex>

#include "flutter/fml/build_config.h"
Expand Down Expand Up @@ -114,7 +115,65 @@ static bool EnableTracingManually(const Settings& vm_settings) {
return true;
}

static bool IsTracingCheckUnnecessaryOnVersion() {
#if !OS_IOS || TARGET_OS_SIMULATOR
return true;
#endif

// Check for arm64e.
cpu_type_t cputype = 0;
size_t cputype_size = sizeof(cpu_type_t);
if (::sysctlbyname("hw.cputype", &cputype, &cputype_size, nullptr, 0) < 0) {
FML_LOG(ERROR) << "Could not execute sysctl() to get CPU type: "
Copy link
Member

@chinmaygarde chinmaygarde Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere: Early return in case of error.

<< strerror(errno);
}

cpu_subtype_t cpusubtype = 0;
if (::sysctlbyname("hw.cpusubtype", &cpusubtype, &cputype_size, nullptr, 0) <
0) {
FML_LOG(ERROR) << "Could not execute sysctl() to get CPU subtype: "
<< strerror(errno);
}

// Tracing is necessary unless the device is arm64e (A12 chip or higher).
if (cputype != CPU_TYPE_ARM64 || cpusubtype != CPU_SUBTYPE_ARM64E) {
return false;
Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ultra-nit: there's a triple negation at this point :) false to != arm64 to unnecessary. Can we simplify slightly?

}

// Check for iOS 14.2 and higher.
Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to you but it feels like we're building in a lot of platform specific knowledge at this point. How about just letting this file statically take a SetOSVersionMayNeedTracingCheck injection or something and FlutterEngine can just set it to https://developer.apple.com/documentation/foundation/nsprocessinfo/1414876-isoperatingsystematleastversion? Could save us some work.

Copy link
Member Author

@jmagman jmagman Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this were all in obj-c++ world then I wouldn't have bothered with all these syctl checks. But it would need to be injected from:

engine/runtime/dart_vm.cc

Line 333 in e78e405

const bool tracing_result = EnableTracingIfNecessary(settings_);

Copy link
Member

@xster xster Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what I meant injecting it statically (since all these stuff is static anyway). Since the engine calls first, we know it would be set before dart calls it again.

Copy link
Member Author

@jmagman jmagman Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that the lower level code would require a static variable to be set before it can be called, without enforcing that ordering itself. Seems like an easy opportunity for bugs if this ever gets refactored. Will defer to @chinmaygarde's opinion here.

If we decide to keep this kern.osversion parsing I'll add a unit test.

Copy link
Member

@chinmaygarde chinmaygarde Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the initiative in doing this in C, but let's make this an ObjC++ TU and use NSOperatingSystemVersion instead. You can remove the TRACING_CHECKS_NECESSARY macro and add a stub C++ file for non-Darwin platforms.

size_t osversion_size;
::sysctlbyname("kern.osversion", NULL, &osversion_size, NULL, 0);
Copy link
Member

@chinmaygarde chinmaygarde Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unchecked error code. Though, I'd suggest using the ObjC++ variant instead.

char osversionBuffer[osversion_size];

if (::sysctlbyname("kern.osversion", osversionBuffer, &osversion_size, NULL,
0) < 0) {
FML_LOG(ERROR) << "Could not execute sysctl() to get current OS version: "
<< strerror(errno);

return false;
}

int major_version = 0;
char minor_letter = 'Z';

for (size_t index = 0; index < osversion_size; index++) {
Copy link
Member Author

@jmagman jmagman Nov 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parsing felt like an interview question (atoi!), happy to try something more idiomatic if ya'll can think of a better way to do it. I started with an istringstream but it actually wound up being identical logic so it didn't seem worth the extra object.

char version_char = osversionBuffer[index];
// Find the minor version build letter.
if (isalpha(version_char)) {
major_version = atoi((const char*)osversionBuffer);
minor_letter = toupper(version_char);
break;
}
}
// 18B92 is iOS 14.2 beta release candidate where tracing became unnecessary.
return major_version > 18 || (major_version == 18 && minor_letter >= 'B');
}

static bool EnableTracingIfNecessaryOnce(const Settings& vm_settings) {
if (IsTracingCheckUnnecessaryOnVersion()) {
return true;
}

if (IsLaunchedByFlutterCLI(vm_settings)) {
return true;
}
Expand Down