Conditionalize the fix for SR-14635 (#40822)

* Refactor Bincompat

Organize everything around internal functions that test for
a particular OS version.

Correctly handle cases where we don't know the version of the app.

Make all bincompat functions consistently return `true` for the
legacy semantics, `false` for new semantics.  Consistently name
them all to reflect this.

* Conditionalize the support for SR-14635

SR-14635 pointed out a hole in the updated dynamic casting logic
that allowed certain casts that should have been illegal.

In particular, when casting certain types to Obj-C protocols,
the Swift value gets boxed; we would permit the cast to succeed
whenever the resulting box satisfied the protocol.  For example,
this allowed any Swift value to be cast to `NSCopying` regardless of
whether or not it implemented the required `copy(with:)` method.

This was fixed in #37683 to reject such casts but of course some folks were
depending on this behavior to pass Swift data into Obj-C functions.
(The properly supported approach for passing arbitrary Swift data into
Obj-C functions is to cast the Swift value to `AnyObject`.)

This change makes that new behavior conditional.  For now,
the legacy semantics are enabled on Apple platforms and the
new semantics are in use everywhere else.  This will allow
us to gradually enable enforcement of the new behavior over
time.

* Just skip this test on Apple platforms, since it is inconsistently implemented there (and is therefore not really testable)

Author: tbkka

include/swift/Runtime/Bincompat.h

@@ -23,16 +23,20 @@ namespace bincompat {
 /// Whether protocol conformance iteration should be reversed, to prefer
 /// conformances from images that are later in the list over earlier ones.
 /// Default is false starting with Swift 5.4.
-bool workaroundProtocolConformanceReverseIteration();
+bool useLegacyProtocolConformanceReverseIteration();
 
 /// Whether we should crash when we encounter a non-nullable Obj-C
 /// reference with a null value as the source of a cast.
 /// Default is true starting with Swift 5.4.
-bool unexpectedObjCNullWhileCastingIsFatal();
+bool useLegacyPermissiveObjCNullSemanticsInCasting();
 
 /// Whether we should use the legacy semantics for casting nil optionals
 /// to nested optionals
-bool useLegacyOptionalNilInjection();
+bool useLegacyOptionalNilInjectionInCasting();
+
+/// Whether to use legacy semantics when boxing Swift values for
+/// Obj-C interop
+bool useLegacyObjCBoxingInCasting();
 
 } // namespace bincompat
 

stdlib/public/runtime/Bincompat.cpp

@@ -33,22 +33,44 @@ namespace runtime {
 
 namespace bincompat {
 
+#if BINARY_COMPATIBILITY_APPLE
+enum sdk_test {
+  oldOS, // Can't tell the app SDK used because this is too old an OS
+  oldApp,
+  newApp
+};
+static enum sdk_test isAppAtLeast(dyld_build_version_t version) {
+  if (__builtin_available(macOS 11.3, iOS 14.5, tvOS 14.5, watchOS 7.4, *)) {
+    // Query the SDK version used to build the currently-running executable
+    if (dyld_program_sdk_at_least(version)) {
+      return newApp;
+    } else {
+      return oldApp;
+    }
+  }
+  // Older Apple OS lack the ability to test the SDK version of the running app
+  return oldOS;
+}
+
+static enum sdk_test isAppAtLeastSpring2021() {
+    const dyld_build_version_t spring_2021_os_versions = {0xffffffff, 0x007e50301};
+    return isAppAtLeast(spring_2021_os_versions);
+}
+#endif
+
 // Should we mimic the old override behavior when scanning protocol conformance records?
 
 // Old apps expect protocol conformances to override each other in a particular
 // order.  Starting with Swift 5.4, that order has changed as a result of
 // significant performance improvements to protocol conformance scanning.  If
 // this returns `true`, the protocol conformance scan will do extra work to
 // mimic the old override behavior.
-bool workaroundProtocolConformanceReverseIteration() {
+bool useLegacyProtocolConformanceReverseIteration() {
 #if BINARY_COMPATIBILITY_APPLE
-  // If this is a newer Apple OS ...
-  if (__builtin_available(macOS 11.3, iOS 14.5, tvOS 14.5, watchOS 7.4, *)) {
-    const dyld_build_version_t spring_2021_os_versions = {0xffffffff, 0x007e50301};
-    // ... but the app was compiled before Spring 2021, use the legacy behavior.
-    return !dyld_program_sdk_at_least(spring_2021_os_versions);
-  } else {
-    return false; // Use new (non-legacy) behavior on old Apple OSes
+  switch (isAppAtLeastSpring2021()) {
+  case oldOS: return false; // New (non-legacy) behavior on old OSes
+  case oldApp: return true; // Legacy behavior for pre-Spring 2021 apps on new OS
+  case newApp: return false; // New behavior for new apps
   }
 #else
   return false; // Never use the legacy behavior on non-Apple OSes
@@ -63,18 +85,15 @@ bool workaroundProtocolConformanceReverseIteration() {
 // declared non-nullable.  Such null pointers can lead to undefined behavior
 // later on.  Starting in Swift 5.4, these unexpected null pointers are fatal
 // runtime errors, but this is selectively disabled for old apps.
-bool unexpectedObjCNullWhileCastingIsFatal() {
+bool useLegacyPermissiveObjCNullSemanticsInCasting() {
 #if BINARY_COMPATIBILITY_APPLE
-  // If this is a new enough Apple OS ...
-  if (__builtin_available(macOS 11.3, iOS 14.5, tvOS 14.5, watchOS 7.4, *)) {
-    const dyld_build_version_t spring_2021_os_versions = {0xffffffff, 0x007e50301};
-    // ... use strict behavior for apps compiled on or after Spring 2021.
-    return dyld_program_sdk_at_least(spring_2021_os_versions);
-  } else {
-    return false; // Use permissive behavior on old Apple OS
+  switch (isAppAtLeastSpring2021()) {
+  case oldOS: return true; // Permissive (legacy) behavior on old OS
+  case oldApp: return true; // Permissive (legacy) behavior for old apps
+  case newApp: return false; // Strict behavior for new apps
   }
 #else
-  return true;  // Always use the strict behavior on non-Apple OSes
+  return false;  // Always use the strict behavior on non-Apple OSes
 #endif
 }
 
@@ -87,21 +106,39 @@ bool unexpectedObjCNullWhileCastingIsFatal() {
 // Earlier versions of the Swift runtime did not do this if the source
 // optional was nil.  In that case, the outer target optional would be
 // set to nil.
-bool useLegacyOptionalNilInjection() {
+bool useLegacyOptionalNilInjectionInCasting() {
 #if BINARY_COMPATIBILITY_APPLE
-  // If this is a new enough Apple OS ...
-  if (__builtin_available(macOS 11.3, iOS 14.5, tvOS 14.5, watchOS 7.4, *)) {
-    const dyld_build_version_t spring_2021_os_versions = {0xffffffff, 0x007e50301};
-    // It's using Spring 2021 or later SDK, so don't use the legacy behavior.
-    return !dyld_program_sdk_at_least(spring_2021_os_versions);
-  } else {
-    return true; // Use the legacy behavior on old Apple OS
+  switch (isAppAtLeastSpring2021()) {
+  case oldOS: return true; // Legacy behavior on old OS
+  case oldApp: return true; // Legacy behavior for old apps
+  case newApp: return false; // Consistent behavior for new apps
   }
 #else
   return false;  // Always use the 5.4 behavior on non-Apple OSes
 #endif
 }
 
+// Should casting be strict about protocol conformance when
+// boxing Swift values to pass to Obj-C?
+
+// Earlier versions of the Swift runtime would allow you to
+// cast a swift value to e.g., `NSCopying` or `NSObjectProtocol`
+// even if that value did not actually conform.  This was
+// due to the fact that the `__SwiftValue` box type itself
+// conformed to these protocols.
+
+// But this was not really sound, as it implies for example that
+// `x is NSCopying` is always `true` regardless of whether
+// `x` actually has the `copyWithZone()` method required
+// by that protocol.
+bool useLegacyObjCBoxingInCasting() {
+#if BINARY_COMPATIBILITY_APPLE
+  return true; // For now, continue using the legacy behavior on Apple OSes
+#else
+  return false; // Always use the new behavior on non-Apple OSes
+#endif
+}
+
 } // namespace bincompat
 
 } // namespace runtime

stdlib/public/runtime/DynamicCast.cpp

@@ -129,13 +129,7 @@ static HeapObject * getNonNullSrcObject(OpaqueValue *srcValue,
   const char * const msg = "Found unexpected null pointer value"
                     " while trying to cast value of type '%s' (%p)"
                     " to '%s' (%p)%s\n";
-  if (runtime::bincompat::unexpectedObjCNullWhileCastingIsFatal()) {
-    // By default, Swift 5.4 and later issue a fatal error.
-    swift::fatalError(/* flags = */ 0, msg,
-                      srcTypeName.c_str(), srcType,
-                      destTypeName.c_str(), destType,
-                      "");
-  } else {
+  if (runtime::bincompat::useLegacyPermissiveObjCNullSemanticsInCasting()) {
     // In backwards compatibility mode, this code will warn and return the null
     // reference anyway: If you examine the calls to the function, you'll see
     // that most callers fail the cast in that case, but a few casts (e.g., with
@@ -145,6 +139,12 @@ static HeapObject * getNonNullSrcObject(OpaqueValue *srcValue,
                    srcTypeName.c_str(), srcType,
                    destTypeName.c_str(), destType,
                    ": Continuing with null object, but expect problems later.");
+  } else {
+    // By default, Swift 5.4 and later issue a fatal error.
+    swift::fatalError(/* flags = */ 0, msg,
+                      srcTypeName.c_str(), srcType,
+                      destTypeName.c_str(), destType,
+                      "");
   }
   return object;
 }
@@ -1092,7 +1092,7 @@ tryCastUnwrappingOptionalBoth(
     srcValue, /*emptyCases=*/1);
   auto sourceIsNil = (sourceEnumCase != 0);
   if (sourceIsNil) {
-    if (runtime::bincompat::useLegacyOptionalNilInjection()) {
+    if (runtime::bincompat::useLegacyOptionalNilInjectionInCasting()) {
       auto destInnerType = cast<EnumMetadata>(destType)->getGenericArgs()[0];
       // Set .none at the outer level
       destInnerType->vw_storeEnumTagSinglePayload(destLocation, 1, 1);
@@ -1554,30 +1554,47 @@ tryCastToClassExistentialViaSwiftValue(
   }
 
   default: {
+    // We can always box when the destination is a simple
+    // (unconstrained) `AnyObject`.
     if (destExistentialType->NumProtocols != 0) {
-      // The destination is a class-constrained protocol type
-      // and the source is not a class, so....
-      return DynamicCastResult::Failure;
-    } else {
-      // This is a simple (unconstrained) `AnyObject` so we can populate
-      // it by stuffing a non-class instance into a __SwiftValue box
+      // But if there are constraints...
+      if (!runtime::bincompat::useLegacyObjCBoxingInCasting()) {
+        // ... never box if we're not supporting legacy semantics.
+        return DynamicCastResult::Failure;
+      }
+      // Legacy behavior: We used to permit casts to a constrained (existential)
+      // type if the resulting `__SwiftValue` box conformed to the target type.
+      // This is no longer supported, since it caused `x is NSCopying` to be
+      // true even when x does not in fact implement the requirements of
+      // `NSCopying`.
 #if SWIFT_OBJC_INTEROP
-      auto object = bridgeAnythingToSwiftValueObject(
-        srcValue, srcType, takeOnSuccess);
-      destExistentialLocation->Value = object;
-      if (takeOnSuccess) {
-        return DynamicCastResult::SuccessViaTake;
-      } else {
-        return DynamicCastResult::SuccessViaCopy;
+      if (!findSwiftValueConformances(
+            destExistentialType, destExistentialLocation->getWitnessTables())) {
+        return DynamicCastResult::Failure;
+      }
+#else
+      if (!swift_swiftValueConformsTo(destType, destType)) {
+        return DynamicCastResult::Failure;
       }
-# else
-      // Note: Code below works correctly on both Obj-C and non-Obj-C platforms,
-      // but the code above is slightly faster on Obj-C platforms.
-      auto object = _bridgeAnythingToObjectiveC(srcValue, srcType);
-      destExistentialLocation->Value = object;
-      return DynamicCastResult::SuccessViaCopy;
 #endif
     }
+
+#if SWIFT_OBJC_INTEROP
+    auto object = bridgeAnythingToSwiftValueObject(
+      srcValue, srcType, takeOnSuccess);
+    destExistentialLocation->Value = object;
+    if (takeOnSuccess) {
+      return DynamicCastResult::SuccessViaTake;
+    } else {
+      return DynamicCastResult::SuccessViaCopy;
+    }
+# else
+    // Note: Code below works correctly on both Obj-C and non-Obj-C platforms,
+    // but the code above is slightly faster on Obj-C platforms.
+    auto object = _bridgeAnythingToObjectiveC(srcValue, srcType);
+    destExistentialLocation->Value = object;
+    return DynamicCastResult::SuccessViaCopy;
+#endif
   }
   }
 }

stdlib/public/runtime/ProtocolConformance.cpp

@@ -428,7 +428,7 @@ struct ConformanceState {
 
   ConformanceState() {
     scanSectionsBackwards =
-        runtime::bincompat::workaroundProtocolConformanceReverseIteration();
+        runtime::bincompat::useLegacyProtocolConformanceReverseIteration();
 
 #if USE_DYLD_SHARED_CACHE_CONFORMANCE_TABLES
     if (__builtin_available(macOS 12.0, iOS 15.0, tvOS 15.0, watchOS 8.0, *)) {

test/Casting/Casts.swift

@@ -956,7 +956,15 @@ CastsTests.test("Recursive AnyHashable") {
 
 // SR-14635 (aka rdar://78224322)
 #if _runtime(_ObjC)
-CastsTests.test("Do not overuse __SwiftValue") {
+CastsTests.test("Do not overuse __SwiftValue")
+.skip(.osxAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.iOSAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.iOSSimulatorAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.tvOSAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.tvOSSimulatorAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.watchOSAny("SR-14635 not yet fully enabled for Apple OSes"))
+.skip(.watchOSSimulatorAny("SR-14635 not yet fully enabled for Apple OSes"))
+.code {
   struct Bar {}
   // This used to succeed because of overeager __SwiftValue
   // boxing (and __SwiftValue does satisfy NSCopying)