Merge pull request #63096 from xymus/deser-safety-writing

[Serialization] Introduce main deserialization safety logic, on the writer side

Author: xymus

lib/Serialization/ModuleFile.cpp

@@ -885,8 +885,13 @@ void ModuleFile::lookupObjCMethods(
   auto found = *known;
   for (const auto &result : found) {
     // Deserialize the method and add it to the list.
-    if (auto func = dyn_cast_or_null<AbstractFunctionDecl>(
-                      getDecl(std::get<2>(result))))
+    auto declOrError = getDeclChecked(std::get<2>(result));
+    if (!declOrError) {
+        consumeError(declOrError.takeError());
+        continue;
+    }
+
+    if (auto func = dyn_cast_or_null<AbstractFunctionDecl>(declOrError.get()))
       results.push_back(func);
   }
 }

lib/Serialization/Serialization.cpp

@@ -70,6 +70,8 @@
 
 #include <vector>
 
+#define DEBUG_TYPE "Serialization"
+
 using namespace swift;
 using namespace swift::serialization;
 using namespace llvm::support;
@@ -3093,6 +3095,170 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     }
   }
 
+  /// Determine if \p decl is safe to deserialize when it's public
+  /// or otherwise needed by the client in normal builds, this should usually
+  /// correspond to logic in type-checking ensuring these safe decls don't
+  /// refer to implementation details. We have to be careful not to mark
+  /// anything needed by a client as unsafe as the client will reject reading
+  /// it, but at the same time keep the safety checks precise to avoid
+  /// XRef errors and such.
+  ///
+  /// \p decl should be either an \c ExtensionDecl or a \c ValueDecl.
+  static bool isDeserializationSafe(const Decl *decl) {
+    if (auto ext = dyn_cast<ExtensionDecl>(decl)) {
+      // Consider extensions as safe as their extended type.
+      auto nominalType = ext->getExtendedNominal();
+      if (!nominalType ||
+          !isDeserializationSafe(nominalType))
+        return false;
+
+      // We can mark the extension unsafe only if it has no public members.
+      auto members = ext->getMembers();
+      auto hasSafeMembers = std::any_of(members.begin(), members.end(),
+        [](const Decl *D) -> bool {
+          if (auto VD = dyn_cast<ValueDecl>(D))
+            return isDeserializationSafe(VD);
+          return true;
+        });
+      if (hasSafeMembers)
+        return true;
+
+      // We can mark the extension unsafe only if it has no  public
+      // conformances.
+      auto protocols = ext->getLocalProtocols(
+                                        ConformanceLookupKind::OnlyExplicit);
+      bool hasSafeConformances = std::any_of(protocols.begin(),
+                                             protocols.end(),
+                                             isDeserializationSafe);
+      if (hasSafeConformances)
+        return true;
+
+      // Truly empty extensions are safe, it may happen in swiftinterfaces.
+      if (members.empty() && protocols.size() == 0)
+        return true;
+
+      return false;
+    }
+
+    auto value = cast<ValueDecl>(decl);
+
+    // A decl is safe if formally accessible publicly.
+    auto accessScope = value->getFormalAccessScope(/*useDC=*/nullptr,
+                       /*treatUsableFromInlineAsPublic=*/true);
+    if (accessScope.isPublic())
+      return true;
+
+    // Testable allows access to internal details.
+    if (value->getDeclContext()->getParentModule()->isTestingEnabled() &&
+        accessScope.isInternal())
+      return true;
+
+    if (auto accessor = dyn_cast<AccessorDecl>(value))
+      // Accessors are as safe as their storage.
+      if (isDeserializationSafe(accessor->getStorage()))
+        return true;
+
+    // Frozen fields are always safe.
+    if (auto var = dyn_cast<VarDecl>(value)) {
+      if (var->isLayoutExposedToClients())
+        return true;
+
+      // Consider all lazy var storage as "safe".
+      // FIXME: We should keep track of what lazy var is associated to the
+      //        storage for them to preserve the same safeness.
+      if (var->isLazyStorageProperty())
+        return true;
+
+      // Property wrappers storage is as safe as the wrapped property.
+      if (VarDecl *wrapped = var->getOriginalWrappedProperty())
+        if (isDeserializationSafe(wrapped))
+          return true;
+    }
+
+    return false;
+  }
+
+  /// Write a \c DeserializationSafetyLayout record only when \p decl is unsafe
+  /// to deserialize.
+  ///
+  /// \sa isDeserializationSafe
+  void writeDeserializationSafety(const Decl *decl) {
+    using namespace decls_block;
+
+    auto DC = decl->getDeclContext();
+    if (!DC->getParentModule()->isResilient())
+      return;
+
+    // Everything should be safe in a swiftinterface. So, don't emit any safety
+    // record when building a swiftinterface in release builds. Debug builds
+    // instead print inconsistencies.
+    auto parentSF = DC->getParentSourceFile();
+    bool fromModuleInterface = parentSF &&
+                               parentSF->Kind == SourceFileKind::Interface;
+#if NDEBUG
+    if (fromModuleInterface)
+      return;
+#endif
+
+    // Private imports allow safe access to everything.
+    if (DC->getParentModule()->arePrivateImportsEnabled())
+      return;
+
+    // Ignore things with no access level.
+    // Note: There's likely room to report some of these as unsafe to prevent
+    //       failures.
+    if (isa<GenericTypeParamDecl>(decl) ||
+        isa<OpaqueTypeDecl>(decl) ||
+        isa<ParamDecl>(decl) ||
+        isa<EnumCaseDecl>(decl) ||
+        isa<EnumElementDecl>(decl))
+      return;
+
+    if (!isa<ValueDecl>(decl) && !isa<ExtensionDecl>(decl))
+      return;
+
+    // Don't look at decls inside functions and
+    // check the ValueDecls themselves.
+    auto declIsSafe = DC->isLocalContext() ||
+                      isDeserializationSafe(decl);
+#ifdef NDEBUG
+    // In release builds, bail right away if the decl is safe.
+    // In debug builds, wait to bail after the debug prints and asserts.
+    if (declIsSafe)
+      return;
+#endif
+
+    // Write a human readable name to an identifier.
+    SmallString<64> out;
+    llvm::raw_svector_ostream outStream(out);
+    if (auto val = dyn_cast<ValueDecl>(decl)) {
+      outStream << val->getName();
+    } else if (auto ext = dyn_cast<ExtensionDecl>(decl)) {
+      outStream << "extension ";
+      if (auto nominalType = ext->getExtendedNominal())
+        outStream << nominalType->getName();
+    }
+    auto name = S.getASTContext().getIdentifier(out);
+
+    LLVM_DEBUG(
+      llvm::dbgs() << "Serialization safety, "
+                   << (declIsSafe? "safe" : "unsafe")
+                   << ": '" << name << "'\n";
+      assert((declIsSafe || !fromModuleInterface) &&
+             "All swiftinterface decls should be deserialization safe");
+    );
+
+#ifndef NDEBUG
+    // Bail out here in debug builds, release builds would bailed out earlier.
+    if (declIsSafe)
+      return;
+#endif
+
+    auto abbrCode = S.DeclTypeAbbrCodes[DeserializationSafetyLayout::Code];
+    DeserializationSafetyLayout::emitRecord(S.Out, S.ScratchRecord, abbrCode,
+                                            S.addDeclBaseNameRef(name));
+  }
+
   void writeForeignErrorConvention(const ForeignErrorConvention &fec) {
     using namespace decls_block;
 
@@ -3410,6 +3576,8 @@ class Serializer::DeclSerializer : public DeclVisitor<DeclSerializer> {
     if (D->isInvalid())
       writeDeclErrorFlag();
 
+    writeDeserializationSafety(D);
+
     // Emit attributes (if any).
     for (auto Attr : D->getAttrs())
       writeDeclAttribute(D, Attr);

test/Serialization/Safety/lookupObjCMethods.swift

@@ -0,0 +1,12 @@
+// RUN: %target-swift-frontend -typecheck -verify %s \
+// RUN:   -enable-deserialization-safety \
+// RUN:   -Xllvm -debug-only=Serialization 2>&1 | %FileCheck %s
+
+// REQUIRES: objc_interop
+// REQUIRES: asserts
+
+import Foundation
+
+// Fails at reading __SwiftNativeNSEnumerator.init() on macOS.
+NSString.instancesRespond(to: "init") // expected-warning {{use of string literal for Objective-C selectors is deprecated; use '#selector' instead}}
+// CHECK: Skipping unsafe deserialization: 'init()'

test/Serialization/Safety/skip-reading-internal-anyobject.swift

@@ -0,0 +1,49 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// REQUIRES: asserts
+// REQUIRES: VENDOR=apple
+
+/// Build library.
+// RUN: %target-swift-frontend -emit-module %t/Lib.swift \
+// RUN:   -enable-library-evolution -swift-version 5 \
+// RUN:   -emit-module-path %t/Lib.swiftmodule
+
+/// Build client.
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -verify -Xllvm -debug-only=Serialization \
+// RUN:   -enable-deserialization-safety 2>&1 \
+// RUN:   | %FileCheck --check-prefixes=SAFE %s
+
+/// Decls skips by the deserialization safety logic.
+// SAFE-NOT: InternalClass
+// SAFE: Deserialized: 'PublicClass'
+// SAFE: Deserialized: 'publicFunc()'
+// SAFE: Skipping unsafe deserialization: 'privateFunc()'
+// SAFE: Skipping unsafe deserialization: 'fileprivateFunc()'
+// SAFE: Skipping unsafe deserialization: 'internalFunc()'
+
+//--- Lib.swift
+
+import Foundation
+
+@objc
+public class PublicClass : NSObject {
+    public func publicFunc() {}
+}
+
+@objc
+internal class InternalClass : NSObject {
+    private func privateFunc() {}
+    fileprivate func fileprivateFunc() {}
+    internal func internalFunc() {}
+}
+
+//--- Client.swift
+
+import Lib
+
+var x: AnyObject
+
+// Trigger a typo correction to read all members of all subtypes to NSObject.
+x.notAMember() // expected-error {{value of type 'AnyObject' has no member 'notAMember'}}

test/Serialization/Safety/skip-internal-details.swift renamed to test/Serialization/Safety/skip-reading-internal-details.swift

@@ -3,23 +3,31 @@
 
 // REQUIRES: asserts
 
+/// Build libraries.
 // RUN: %target-swift-frontend -emit-module %t/HiddenLib.swift \
-// RUN:   -enable-library-evolution \
+// RUN:   -enable-library-evolution -swift-version 5 \
 // RUN:   -emit-module-path %t/HiddenLib.swiftmodule
-
 // RUN: %target-swift-frontend -emit-module %t/Lib.swift -I %t \
 // RUN:   -enable-library-evolution \
 // RUN:   -emit-module-path %t/Lib.swiftmodule \
 // RUN:   -emit-module-interface-path %t/Lib.swiftinterface
 
+/// Build clients, with and without safety.
 // RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
-// RUN:   -verify -Xllvm -debug-only=Serialization 2>&1 \
+// RUN:   -verify -Xllvm -debug-only=Serialization \
+// RUN:   -disable-deserialization-safety 2>&1 \
 // RUN:   | %FileCheck --check-prefixes=NEEDED,UNSAFE %s
 
-// RUN: rm %t/Lib.swiftmodule
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -verify -Xllvm -debug-only=Serialization \
+// RUN:   -enable-deserialization-safety 2>&1 \
+// RUN:   | %FileCheck --check-prefixes=NEEDED,CLEAN,SAFE %s
 
+/// Build against the swiftinterface.
+// RUN: rm %t/Lib.swiftmodule
 // RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
-// RUN:   -verify -Xllvm -debug-only=Serialization 2>&1 \
+// RUN:   -verify -Xllvm -debug-only=Serialization \
+// RUN:   -disable-deserialization-safety 2>&1 \
 // RUN:   | %FileCheck --check-prefixes=NEEDED,CLEAN %s
 
 /// Decls part of the API needed by the client.
@@ -37,6 +45,12 @@
 // CLEAN-NOT: Deserialized: 'privateFunc()'
 // CLEAN-NOT: Deserialized: 'fileprivateFunc()'
 
+/// Decls skips by the deserialization safety logic.
+// SAFE: Skipping unsafe deserialization: 'internalFunc()'
+// SAFE: Skipping unsafe deserialization: 'privateFunc()'
+// SAFE: Skipping unsafe deserialization: 'fileprivateFunc()'
+// SAFE: Skipping unsafe deserialization: 'refToIOI()'
+
 //--- HiddenLib.swift
 
 public struct HiddenStruct {
@@ -47,7 +61,7 @@ public struct HiddenStruct {
 
 @_implementationOnly import HiddenLib
 
-public class PublicStruct {
+public struct PublicStruct {
     public init() {}
 
     public func publicFunc() {}