Disable ptrace on arm64e running iOS 14.2 and greater #22377

jmagman · 2020-11-07T02:27:12Z

Description

The ptrace trick may not be necessary on iOS 14.2 (beta release candidate) which seems to have added JIT compilation support. This means Flutter debug apps aren't sigkilled when launched without an attached debugger.

There's speculation that this is only true on A12 (and presumably later A-) chips. With #define TRACING_CHECKS_NECESSARY 0 a debug app launched from the home screen on my A12 iPad Pro, but on @xster's A9 iPhone 6s it still crashed.

engine/runtime/ptrace_check.h

Lines 13 to 15 in f0fb74b

    
           #define TRACING_CHECKS_NECESSARY    \ 
        
             OS_IOS && !TARGET_OS_SIMULATOR && \ 
        
                 (FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG)

The outstanding theory is JIT may only be working on arm64e devices running iOS 14.2.

Related Issues

Fixes flutter/flutter#69984

Testing

I have no idea how to test this any more than the rest of this ptrace file without adding some wrappers around sysctlbyname. Unfortunately I don't think we have any arm64e devicelab test devices, either.

flutter-dashboard · 2020-11-07T02:27:15Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jmagman · 2020-11-07T02:27:22Z

Let's hold off actually merging this until we know more about what devices still need tracing, but I wanted a PR ready to go.

jmagman · 2020-11-07T02:33:09Z

runtime/ptrace_check.cc

+  int major_version = 0;
+  char minor_letter = 'Z';
+
+  for (size_t index = 0; index < osversion_size; index++) {


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.

xster · 2020-11-09T17:43:01Z

runtime/ptrace_check.cc

+    return false;
+  }
+
+  // Check for iOS 14.2 and higher.


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.

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_);

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.

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.

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.

xster · 2020-11-09T17:50:36Z

runtime/ptrace_check.cc

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


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

xster · 2020-11-10T05:08:11Z

runtime/ptrace_check.cc

@@ -27,6 +27,7 @@
 #include <sys/sysctl.h>
 #include <sys/types.h>

+#include <mach/machine.h>


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

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

chinmaygarde

Construction issues aside, this must not land as you'd still to check if the entitlement is present at the specific iOS version. Apps without the entitlement in debug will still crash. I'd also rather link to Apple's documentation instead of relying on speculation right now.

chinmaygarde · 2020-11-10T18:22:15Z

runtime/ptrace_check.cc

@@ -27,6 +27,7 @@
 #include <sys/sysctl.h>
 #include <sys/types.h>

+#include <mach/machine.h>


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

chinmaygarde · 2020-11-10T18:24:45Z

runtime/ptrace_check.cc

+  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: "


Here and elsewhere: Early return in case of error.

chinmaygarde · 2020-11-10T18:34:34Z

runtime/ptrace_check.cc

+    return false;
+  }
+
+  // Check for iOS 14.2 and higher.


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.

chinmaygarde · 2020-11-10T18:34:58Z

runtime/ptrace_check.cc

+
+  // Check for iOS 14.2 and higher.
+  size_t osversion_size;
+  ::sysctlbyname("kern.osversion", NULL, &osversion_size, NULL, 0);


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

chinmaygarde · 2020-11-12T22:57:25Z

@jmagman Any updates? Have you tried this without the entitlement in the plist?

jmagman · 2020-11-12T23:07:12Z

@jmagman Any updates?

I haven't created the ObjC++ variant yet. On this PR it's working on an (arm64e) iPad Pro by launching without the debugger attached and running the app. And on an (arm64) iPhone 6s it's showing the warning screen.

Have you tried this without the entitlement in the plist?
you'd still to check if the entitlement is present at the specific iOS version.

Which entitlement? get-task-allow?

I'd also rather link to Apple's documentation instead of relying on speculation right now.

Yeah all I have is speculation about the A12 and higher chips.

jmagman · 2020-12-15T19:33:35Z

@zanderso Would you like me to keep working on this so users can launch apps from the home screen on new devices? Or should I abandon?

zanderso · 2020-12-15T19:50:16Z

I think we should probably abandon until we can get some more solid info about the devices where this is supported.

Disable ptrace on arm64e running iOS 14.2 and greater

1363bf3

jmagman self-assigned this Nov 7, 2020

google-cla bot added the cla: yes label Nov 7, 2020

jmagman added the platform-ios label Nov 7, 2020

jmagman mentioned this pull request Nov 7, 2020

Always launch in JIT mode on iOS 14.2 even when debugger isn't attached (remove ptrace check) flutter/flutter#69984

Closed

jmagman commented Nov 7, 2020

View reviewed changes

jmagman requested review from chinmaygarde and xster November 7, 2020 02:34

jmagman mentioned this pull request Nov 7, 2020

Add newer iOS devices to devicelab flutter/flutter#70004

Closed

xster reviewed Nov 9, 2020

View reviewed changes

xster approved these changes Nov 10, 2020

View reviewed changes

chinmaygarde suggested changes Nov 10, 2020

View reviewed changes

jmagman added the Work in progress (WIP) Not ready (yet) for review! label Nov 18, 2020

jmagman closed this Dec 15, 2020

jmagman deleted the ptrace_14-2 branch December 15, 2020 20:23

	#define TRACING_CHECKS_NECESSARY \
	OS_IOS && !TARGET_OS_SIMULATOR && \
	(FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG)

Disable ptrace on arm64e running iOS 14.2 and greater #22377

Disable ptrace on arm64e running iOS 14.2 and greater #22377

Uh oh!

Conversation

jmagman commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Testing

Uh oh!

flutter-dashboard bot commented Nov 7, 2020

Uh oh!

jmagman commented Nov 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Nov 12, 2020

Uh oh!

jmagman commented Nov 12, 2020

Uh oh!

jmagman commented Dec 15, 2020

Uh oh!

zanderso commented Dec 15, 2020

Uh oh!

Uh oh!

jmagman commented Nov 7, 2020 •

edited

Loading

jmagman commented Nov 7, 2020 •

edited

Loading

jmagman Nov 9, 2020 •

edited

Loading