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/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,62 @@ bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
   return access == AccessLevel::Open;
 }
 
+static llvm::DenseMap<Identifier, std::pair<Identifier, bool>> DeclBypassMap;
+
 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;
+
+  // If in the same package, package optimization should be enabled
+  // for the module containing this decl.
+  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; disallow bypassing in case of an error
+    // since accessing memory layout directly will cause UB or crash in such case.
+    if (accessingModule &&
+        accessingModule != declModule &&
+        !getBaseName().isSpecial()) {
+
+      // First look up cached value
+      auto found = DeclBypassMap.find(getBaseIdentifier());
+      if (found != DeclBypassMap.end()) {
+        if (found->getSecond().first == accessingModule->getBaseIdentifier())
+          return found->getSecond().second;
+      }
+      
+      // If this decl contains members, check if the members were
+      // deserialized correctly.
+      if (auto IDC = dyn_cast<IterableDeclContext>(this)) {
+        // Members are deserialized lazily, so if un-deserialized,
+        // force load them all here.
+        IDC->checkDeserializeMemberErrorRecursively();
+        
+        // If member deserialization had an error, fail here.
+        if (IDC->hasDeserializeMemberError()) {
+          if (!DeclBypassMap.insert({getBaseIdentifier(), {accessingModule->getBaseIdentifier(), false}}).second) {
+            return false;
+          }
+          getASTContext().Diags.diagnose(getLoc(),
+                                         diag::cannot_bypass_resilience_due_to_deserialization_error,
+                                         getBaseIdentifier());
+          return false;
+        }
+      }
+      
+      if (!DeclBypassMap.insert({getBaseIdentifier(), {accessingModule->getBaseIdentifier(), true}}).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,51 @@ void IterableDeclContext::loadAllMembers() const {
     --s->getFrontendCounters().NumUnloadedLazyIterableDeclContexts;
 }
 
+// This recursively checks whether types of members were deserialized
+// correctly.
+void IterableDeclContext::checkDeserializeMemberErrorRecursively() {
+  if (!didDeserializeMembers()) {
+    // This needs to be set to force load all members.
+    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 are already
+  // 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) {
+            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 this decl's member.
+                  fieldIDC->checkDeserializeMemberErrorRecursively();
+
+                  if (fieldIDC->hasDeserializeMemberError()) {
+                    // If deserializing a member had an error, set
+                    // the error bit for this decl as well.
+                    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/Frontend/CompilerInvocation.cpp

@@ -1379,7 +1379,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
       Opts.AllowNonResilientAccess = false;
     }
   }
-
+  
   // HACK: The driver currently erroneously passes all flags to module interface
   // verification jobs. -experimental-skip-non-exportable-decls is not
   // appropriate for verification tasks and should be ignored, though.

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

@@ -8283,6 +8283,7 @@ ModuleFile::handleErrorAndSupplyMissingMember(ASTContext &context,
     return handleErrorAndSupplyMissingProtoMember(context, std::move(error),
                                                   containingProto);
   }
+  
   return handleErrorAndSupplyMissingMiscMember(std::move(error));
 }
 
@@ -8300,6 +8301,7 @@ void ModuleFile::loadAllMembers(Decl *container, uint64_t contextData) {
   if (diagnoseAndConsumeFatalIfNotSuccess(
           DeclTypeCursor.JumpToBit(contextData)))
     return;
+
   llvm::BitstreamEntry entry = fatalIfUnexpected(DeclTypeCursor.advance());
   if (entry.Kind != llvm::BitstreamEntry::Record)
     return diagnoseAndConsumeFatal();
@@ -8314,9 +8316,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) {
@@ -8325,15 +8330,25 @@ void ModuleFile::loadAllMembers(Decl *container, uint64_t contextData) {
       assert(next.get() && "unchecked error deserializing next member");
       members.push_back(next.get());
     } else {
+      
       if (!getContext().LangOpts.EnableDeserializationRecovery)
         fatal(next.takeError());
 
       Decl *suppliedMissingMember = handleErrorAndSupplyMissingMember(
           getContext(), container, next.takeError());
-      if (suppliedMissingMember)
+      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);