Package optimization allows bypassing resilience, but that assumes the memory layout of the

decl being accessed is correct. When this assumption fails due to a deserialization error
of its members, the use site accesses the layout with a wrong field offset, resulting in
UB or a crash. The deserialization error is currently not caught at compile time due to
LangOpts.EnableDeserializationRecovery being enabled by default to allow for recovery of some
of the deserialization errors at a later time. In case of member deserialization, however,
it's not necessarily recovered later on.

This PR tracks whether member deserialization had an error, and uses that info recursively
in resilience bypassing check and fails with a diagnostic.

Resolves rdar://132411524

Author: elsh

include/swift/AST/Decl.h

@@ -2828,6 +2828,7 @@ class ValueDecl : public Decl {
 
   friend class Decl;
   SourceLoc getLocFromSource() const { return NameLoc; }
+
 protected:
   ValueDecl(DeclKind K,
             llvm::PointerUnion<DeclContext *, ASTContext *> context,

include/swift/AST/DeclContext.h

@@ -806,6 +806,18 @@ class IterableDeclContext {
   /// while skipping the body of this context.
   unsigned HasDerivativeDeclarations : 1;
 
+  /// Members of a decl are deserialized lazily. This is set when
+  /// deserialization of all members is done, regardless of errors.
+  unsigned DeserializedMembers : 1;
+
+  /// Deserialization errors are attempted to be recovered later or
+  /// silently dropped due to `EnableDeserializationRecovery` being
+  /// on by default. The following flag is set when deserializing
+  /// members fails regardless of the `EnableDeserializationRecovery`
+  /// value and is used to prevent decl containing such members from
+  /// being accessed non-resiliently.
+  unsigned HasDeserializeMemberError : 1;
+
   template<class A, class B, class C>
   friend struct ::llvm::CastInfo;
 
@@ -824,6 +836,8 @@ class IterableDeclContext {
     HasDerivativeDeclarations = 0;
     HasNestedClassDeclarations = 0;
     InFreestandingMacroArgument = 0;
+    DeserializedMembers = 0;
+    HasDeserializeMemberError = 0;
   }
 
   /// Determine the kind of iterable context we have.
@@ -833,6 +847,16 @@ class IterableDeclContext {
 
   bool hasUnparsedMembers() const;
 
+  void setDeserializedMembers(bool deserialized) { DeserializedMembers = deserialized; }
+  bool didDeserializeMembers() const { return DeserializedMembers; }
+
+  void setHasDeserializeMemberError(bool hasError) { HasDeserializeMemberError = hasError; }
+  bool hasDeserializeMemberError() const { return HasDeserializeMemberError; }
+
+  /// This recursively checks whether types of this decl's members
+  /// were deserialized correctly.
+  void checkDeserializeMemberErrorRecursively();
+
   bool maybeHasOperatorDeclarations() const {
     return HasOperatorDeclarations;
   }

include/swift/AST/DiagnosticsSema.def

@@ -4726,6 +4726,11 @@ NOTE(ambiguous_because_of_trailing_closure,none,
      "avoid using a trailing closure}0 to call %1",
      (bool, const ValueDecl *))
 
+// In-package resilience bypassing
+ERROR(cannot_bypass_resilience_due_to_deserialization_error,none,
+      "cannot bypass resilience when accessing %0 because some of its members failed to deserialize",
+      (Identifier))
+
 // Cannot capture inout-ness of a parameter
 // Partial application of foreign functions not supported
 ERROR(partial_application_of_function_invalid,none,

lib/AST/Decl.cpp

@@ -4572,15 +4572,76 @@ bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
   return access == AccessLevel::Open;
 }
 
+/// Used to track whether accessing this decl from another module is allowed resilience bypassing.
+static llvm::DenseMap<Identifier, Identifier> DeclResilienceBypassMap;
+
 bool ValueDecl::bypassResilienceInPackage(ModuleDecl *accessingModule) const {
-  // If the defining module is built with package-cmo, bypass
-  // resilient access from the use site that belongs to a module
-  // in the same package.
+  // To allow bypassing resilience when accessing this decl from a
+  // use site, the module of the use site should be in the same package
+  // as the module of this decl.
   auto declModule = getModuleContext();
-  return declModule->inSamePackage(accessingModule) &&
-         declModule->isResilient() &&
-         declModule->allowNonResilientAccess() &&
-         declModule->serializePackageEnabled();
+  if (!declModule->inSamePackage(accessingModule))
+    return false;
+
+  // First look up the cached value
+  if (accessingModule &&
+      accessingModule != declModule &&
+      !getBaseName().isSpecial()) {
+    auto found = DeclResilienceBypassMap.find(getBaseIdentifier());
+    if (found != DeclResilienceBypassMap.end()) {
+      if (found->getSecond() == accessingModule->getBaseIdentifier())
+        return true;
+    }
+  }
+
+  // Package optimization allows bypassing resilience, but it assumes the
+  // memory layout of the decl being accessed is correct. When this assumption
+  // fails due to a deserialization error of its members, the use site accesses
+  // the layout of the decl with a wrong field offset, resulting in UB or a crash.
+  // The deserialization error is currently not caught at compile time due to
+  // LangOpts.EnableDeserializationRecovery being enabled by default (to allow
+  // for recovery of some of the deserialization errors at a later time). In case
+  // of member deserialization, however, it's not necessarily recovered later on
+  // and is silently dropped.
+  // The following tracks errors in member deserialization by recursively loading
+  // members of a type (if not done) and checking whether the type's members, and
+  // their respective types (recursively), encountered deserialization failures.
+  // If any such type is found, it fails and emits a diagnostic at compile time.
+  // Simply disallowing resilience bypassing here and continuing is insufficient
+  // because it would later (during SIL deserialiaztion) require skipping instructions
+  // that were valid in the imported module but are no longer valid at the client
+  // module due to type requirement mismatch; addressing this would involve exhaustive
+  // instruction checks that can be complex and error-prone.
+  if (declModule->isResilient() &&
+      declModule->allowNonResilientAccess() &&
+      declModule->serializePackageEnabled()) {
+    // If this decl is accessed from another module, check if deserializing
+    // members of this decl had an error; if it errored, the decl now has
+    // an incorrect memory layout, and accessing it directly would most likely
+    // use a wrong field offset, resulting in UB or crash. To prevent this,
+    // fail and emit diagnostic here.
+    if (accessingModule &&
+        accessingModule != declModule &&
+        !getBaseName().isSpecial()) {
+      if (auto IDC = dyn_cast<IterableDeclContext>(this)) {
+        // Recursively check if members and their members have failing
+        // deserialization.
+        IDC->checkDeserializeMemberErrorRecursively();
+        // If member deserialization had an error, fail and emit diag here.
+        if (IDC->hasDeserializeMemberError()) {
+          getASTContext().Diags.diagnose(getLoc(),
+                                         diag::cannot_bypass_resilience_due_to_deserialization_error,
+                                         getBaseIdentifier());
+          return false;
+        }
+      }
+      if (!DeclResilienceBypassMap.insert({getBaseIdentifier(), accessingModule->getBaseIdentifier()}).second) {
+        return false;
+      }
+    }
+    return true;
+  }
+  return false;
 }
 
 /// Given the formal access level for using \p VD, compute the scope where

lib/AST/DeclContext.cpp

@@ -1173,6 +1173,50 @@ void IterableDeclContext::loadAllMembers() const {
     --s->getFrontendCounters().NumUnloadedLazyIterableDeclContexts;
 }
 
+// This checks whether types of members and their respective members
+// (recursively) were deserialized correctly.
+void IterableDeclContext::checkDeserializeMemberErrorRecursively() {
+  if (!didDeserializeMembers()) {
+    // This needs to be set to force load all members if not done already.
+    setHasLazyMembers(true);
+    // Call getMembers to actually load them all.
+    auto members = getMembers();
+    assert(!hasLazyMembers());
+    assert(didDeserializeMembers());
+  }
+  // Members could have been deserialized from other calls, so we
+  // still need to check for an error here even if they might have
+  // already been deserialized.
+  if (!hasDeserializeMemberError()) {
+    // If members are already loaded, getMembers call should be inexpensive.
+    for (auto member: getMembers()) {
+      if (auto *PBD = dyn_cast<PatternBindingDecl>(member)) {
+        for (auto i : range(PBD->getNumPatternEntries())) {
+          auto pattern = PBD->getPattern(i);
+          pattern->forEachVariable([&](const VarDecl *VD) {
+            // In case of Optional, looking up the unwrapped type.
+            if (auto actualType =
+                VD->getInterfaceType()->getCanonicalType().getOptionalObjectType()) {
+              if (auto fieldNominal = actualType->getNominalOrBoundGenericNominal()) {
+                if (auto fieldIDC = dyn_cast<IterableDeclContext>(fieldNominal)) {
+                  // Recursively check on the type of a member.
+                  fieldIDC->checkDeserializeMemberErrorRecursively();
+                  // If the member had deserialization failure, mark the
+                  // same for its containing type.
+                  if (fieldIDC->hasDeserializeMemberError()) {
+                    setHasDeserializeMemberError(true);
+                    return;
+                  }
+                }
+              }
+            }
+          });
+        }
+      }
+    }
+  }
+}
+
 bool IterableDeclContext::wasDeserialized() const {
   const DeclContext *DC = getAsGenericContext();
   if (auto F = dyn_cast<FileUnit>(DC->getModuleScopeContext())) {

lib/AST/TypeSubstitution.cpp

@@ -997,8 +997,7 @@ ReplaceOpaqueTypesWithUnderlyingTypes::shouldPerformSubstitution(
   // resilient expansion if the context's and the opaque type's module are in
   // the same package.
   if (contextExpansion == ResilienceExpansion::Maximal &&
-      module->isResilient() && module->serializePackageEnabled() &&
-      module->inSamePackage(contextModule))
+      namingDecl->bypassResilienceInPackage(contextModule))
     return OpaqueSubstitutionKind::SubstituteSamePackageMaximalResilience;
 
   // Allow general replacement from non resilient modules. Otherwise, disallow.

lib/ClangImporter/ImportDecl.cpp

@@ -9713,17 +9713,22 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
   // Check whether we're importing an Objective-C container of some sort.
   auto objcContainer =
     dyn_cast_or_null<clang::ObjCContainerDecl>(D->getClangDecl());
+  auto *IDC = dyn_cast<IterableDeclContext>(D);
 
   // If not, we're importing globals-as-members into an extension.
   if (objcContainer) {
     loadAllMembersOfSuperclassIfNeeded(dyn_cast<ClassDecl>(D));
     loadAllMembersOfObjcContainer(D, objcContainer);
+    if (IDC) // Set member deserialization status
+      IDC->setDeserializedMembers(true);
     return;
   }
 
   if (isa_and_nonnull<clang::RecordDecl>(D->getClangDecl())) {
     loadAllMembersOfRecordDecl(cast<NominalTypeDecl>(D),
                                cast<clang::RecordDecl>(D->getClangDecl()));
+    if (IDC) // Set member deserialization status
+      IDC->setDeserializedMembers(true);
     return;
   }
 
@@ -9734,6 +9739,8 @@ ClangImporter::Implementation::loadAllMembers(Decl *D, uint64_t extra) {
   }
 
   loadAllMembersIntoExtension(D, extra);
+  if (IDC) // Set member deserialization status
+    IDC->setDeserializedMembers(true);
 }
 
 void ClangImporter::Implementation::loadAllMembersIntoExtension(

lib/SIL/IR/TypeLowering.cpp

@@ -2442,13 +2442,8 @@ namespace {
         // The same should happen if the type was resilient and serialized in
         // another module in the same package with package-cmo enabled, which
         // treats those modules to be in the same resilience domain.
-        auto declModule = D->getModuleContext();
-        bool sameModule = (declModule == &TC.M);
-        bool serializedPackage = declModule != &TC.M &&
-                                 declModule->inSamePackage(&TC.M) &&
-                                 declModule->isResilient() &&
-                                 declModule->serializePackageEnabled();
-        auto inSameResilienceDomain = sameModule || serializedPackage;
+        auto inSameResilienceDomain = D->getModuleContext() == &TC.M ||
+                                      D->bypassResilienceInPackage(&TC.M);
         if (inSameResilienceDomain)
           properties.addSubobject(RecursiveProperties::forResilient());
 

lib/Serialization/Deserialization.cpp

@@ -8314,9 +8314,12 @@ void ModuleFile::loadAllMembers(Decl *container, uint64_t contextData) {
   ArrayRef<uint64_t> rawMemberIDs;
   decls_block::MembersLayout::readRecord(memberIDBuffer, rawMemberIDs);
 
-  if (rawMemberIDs.empty())
+  if (rawMemberIDs.empty()) {
+    // No members; set the state of member deserialization to done.
+    if (!IDC->didDeserializeMembers())
+      IDC->setDeserializedMembers(true);
     return;
-
+  }
   SmallVector<Decl *, 16> members;
   members.reserve(rawMemberIDs.size());
   for (DeclID rawID : rawMemberIDs) {
@@ -8332,8 +8335,16 @@ void ModuleFile::loadAllMembers(Decl *container, uint64_t contextData) {
           getContext(), container, next.takeError());
       if (suppliedMissingMember)
         members.push_back(suppliedMissingMember);
+
+      // Not all members can be discovered as missing
+      // members as checked above, so set the error bit
+      // here.
+      IDC->setHasDeserializeMemberError(true);
     }
   }
+  // Set the status of member deserialization to Done.
+  if (!IDC->didDeserializeMembers())
+    IDC->setDeserializedMembers(true);
 
   for (auto member : members)
     IDC->addMember(member);