Skip to content

[Serialization] Prepare the infrastructure for the deserialization safety feature #62886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions include/swift/Basic/LangOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,15 @@ namespace swift {
bool EnableTargetOSChecking = true;

/// Whether to attempt to recover from missing cross-references and other
/// errors when deserializing from a Swift module.
/// errors when deserializing from a binary swiftmodule file.
///
/// This is a staging flag; eventually it will be removed.
/// This flag should only be used in testing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this isn't actually true since we need it enabled in order to have EnableDeserializationSafety work. Plus at the very least LLDB would like want this regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, thanks for pointing it out, it should say This flag should only be disabled in testing.. We always want this enabled unless we want to cause a crash.

bool EnableDeserializationRecovery = true;

/// Enable early skipping deserialization of decls that are marked as
/// unsafe to read.
bool EnableDeserializationSafety = false;

/// Whether to enable the new operator decl and precedencegroup lookup
/// behavior. This is a staging flag, and will be removed in the future.
bool EnableNewOperatorLookup = false;
Expand Down
7 changes: 7 additions & 0 deletions include/swift/Option/FrontendOptions.td
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,13 @@ def disable_deserialization_recovery :
Flag<["-"], "disable-deserialization-recovery">,
HelpText<"Don't attempt to recover from missing xrefs (etc) in swiftmodules">;

def enable_deserialization_safety :
Flag<["-"], "enable-deserialization-safety">,
HelpText<"Avoid reading potentially unsafe decls in swiftmodules">;
def disable_deserialization_safety :
Flag<["-"], "disable-deserialization-safety">,
HelpText<"Don't avoid reading potentially unsafe decls in swiftmodules">;

def enable_experimental_string_processing :
Flag<["-"], "enable-experimental-string-processing">,
HelpText<"Enable experimental string processing">;
Expand Down
6 changes: 6 additions & 0 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,12 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
= A->getOption().matches(OPT_enable_deserialization_recovery);
}

if (auto A = Args.getLastArg(OPT_enable_deserialization_safety,
OPT_disable_deserialization_safety)) {
Opts.EnableDeserializationSafety
= A->getOption().matches(OPT_enable_deserialization_safety);
}

// Whether '/.../' regex literals are enabled. This implies experimental
// string processing.
if (Args.hasArg(OPT_enable_bare_slash_regex)) {
Expand Down
5 changes: 5 additions & 0 deletions lib/Frontend/Frontend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,11 @@ bool CompilerInstance::setUpASTContextIfNeeded() {

registerIRGenSILTransforms(*Context);

if (Invocation.getFrontendOptions().RequestedAction ==
FrontendOptions::ActionType::MergeModules ||
Invocation.getLangOptions().DebuggerSupport)
Invocation.getLangOptions().EnableDeserializationSafety = false;

if (setUpModuleLoaders())
return true;

Expand Down
2 changes: 2 additions & 0 deletions lib/Serialization/DeclTypeRecordNodes.def
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ OTHER(LOCAL_DISCRIMINATOR, 127)
OTHER(PRIVATE_DISCRIMINATOR, 128)
OTHER(FILENAME_FOR_PRIVATE, 129)

OTHER(DESERIALIZATION_SAFETY, 130)

// 140 is unused; was ABSTRACT_PROTOCOL_CONFORMANCE
OTHER(NORMAL_PROTOCOL_CONFORMANCE, 141)
OTHER(SPECIALIZED_PROTOCOL_CONFORMANCE, 142)
Expand Down
21 changes: 21 additions & 0 deletions lib/Serialization/Deserialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ const char DeclAttributesDidNotMatch::ID = '\0';
void DeclAttributesDidNotMatch::anchor() {}
const char InvalidRecordKindError::ID = '\0';
void InvalidRecordKindError::anchor() {}
const char UnsafeDeserializationError::ID = '\0';
void UnsafeDeserializationError::anchor() {}

/// Skips a single record in the bitstream.
///
Expand Down Expand Up @@ -4750,6 +4752,14 @@ ModuleFile::getDeclChecked(
IDC->setDeclID(DID);
}
}

LLVM_DEBUG(
if (auto *VD = dyn_cast_or_null<ValueDecl>(declOrOffset.get())) {
llvm::dbgs() << "Deserialized: '";
llvm::dbgs() << VD->getName();
llvm::dbgs() << "'\n";
});

return declOrOffset;
}

Expand Down Expand Up @@ -5403,6 +5413,17 @@ llvm::Error DeclDeserializer::deserializeDeclCommon() {
IdentifierID filenameID;
decls_block::FilenameForPrivateLayout::readRecord(scratch, filenameID);
filenameForPrivate = MF.getIdentifierText(filenameID);
} else if (recordID == decls_block::DESERIALIZATION_SAFETY) {
IdentifierID declID;
decls_block::DeserializationSafetyLayout::readRecord(scratch, declID);

if (MF.getResilienceStrategy() == ResilienceStrategy::Resilient &&
MF.getContext().LangOpts.EnableDeserializationSafety) {
auto name = MF.getIdentifier(declID);
LLVM_DEBUG(llvm::dbgs() << "Skipping unsafe deserialization: '"
<< name << "'\n");
return llvm::make_error<UnsafeDeserializationError>(name);
}
} else {
return llvm::Error::success();
}
Expand Down
21 changes: 21 additions & 0 deletions lib/Serialization/DeserializationErrors.h
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,27 @@ class InvalidRecordKindError :
}
};

// Decl was not deserialized because it's an internal detail maked as unsafe
// at serialization.
class UnsafeDeserializationError : public llvm::ErrorInfo<UnsafeDeserializationError, DeclDeserializationError> {
friend ErrorInfo;
static const char ID;
void anchor() override;

public:
UnsafeDeserializationError(Identifier name) {
this->name = name;
}

void log(raw_ostream &OS) const override {
OS << "Decl '" << name << "' is unsafe to deserialize";
}

std::error_code convertToErrorCode() const override {
return llvm::inconvertibleErrorCode();
}
};

LLVM_NODISCARD
static inline std::unique_ptr<llvm::ErrorInfoBase>
takeErrorInfo(llvm::Error error) {
Expand Down
7 changes: 6 additions & 1 deletion lib/Serialization/ModuleFormat.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
/// Don't worry about adhering to the 80-column limit for this line.
const uint16_t SWIFTMODULE_VERSION_MINOR = 731; // macro definition
const uint16_t SWIFTMODULE_VERSION_MINOR = 732; // deserialization safety

/// A standard hash seed used for all string hashes in a serialized module.
///
Expand Down Expand Up @@ -1809,6 +1809,11 @@ namespace decls_block {
IdentifierIDField // the file name, as an identifier
>;

using DeserializationSafetyLayout = BCRecordLayout<
DESERIALIZATION_SAFETY,
IdentifierIDField // name to debug access to unsafe decl
>;

using NormalProtocolConformanceLayout = BCRecordLayout<
NORMAL_PROTOCOL_CONFORMANCE,
DeclIDField, // the protocol
Expand Down
1 change: 1 addition & 0 deletions lib/Serialization/Serialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5336,6 +5336,7 @@ void Serializer::writeAllDeclsAndTypes() {
registerDeclTypeAbbr<LocalDiscriminatorLayout>();
registerDeclTypeAbbr<PrivateDiscriminatorLayout>();
registerDeclTypeAbbr<FilenameForPrivateLayout>();
registerDeclTypeAbbr<DeserializationSafetyLayout>();
registerDeclTypeAbbr<MembersLayout>();
registerDeclTypeAbbr<XRefLayout>();

Expand Down
70 changes: 70 additions & 0 deletions test/Serialization/Safety/skip-internal-details.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// RUN: %empty-directory(%t)
// RUN: split-file %s %t

// REQUIRES: asserts

// RUN: %target-swift-frontend -emit-module %t/HiddenLib.swift \
// RUN: -enable-library-evolution \
// 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

// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
// RUN: -verify -Xllvm -debug-only=Serialization 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 2>&1 \
// RUN: | %FileCheck --check-prefixes=NEEDED,CLEAN %s

/// Decls part of the API needed by the client.
// NEEDED-NOT: Deserialized: 'refToIOI()'
// NEEDED: Deserialized: 'PublicStruct'
// NEEDED: Deserialized: 'publicFunc()'

/// Internal details dangerous to load.
// UNSAFE: Deserialized: 'internalFunc()'
// UNSAFE: Deserialized: 'privateFunc()'
// UNSAFE: Deserialized: 'fileprivateFunc()'

/// Decls removed by rebuilding from the swiftinterface.
// CLEAN-NOT: Deserialized: 'internalFunc()'
// CLEAN-NOT: Deserialized: 'privateFunc()'
// CLEAN-NOT: Deserialized: 'fileprivateFunc()'

//--- HiddenLib.swift

public struct HiddenStruct {
public init() {}
}

//--- Lib.swift

@_implementationOnly import HiddenLib

public class PublicStruct {
public init() {}

public func publicFunc() {}
internal func internalFunc() {}
private func privateFunc() {}
fileprivate func fileprivateFunc() {}

internal func refToIOI() -> HiddenStruct {
return HiddenStruct();
}
}

//--- Client.swift

import Lib

var x = PublicStruct()

// Trigger a typo correction that reads all members.
x.notAMember() // expected-error {{value of type 'PublicStruct' has no member 'notAMember'}}
1 change: 1 addition & 0 deletions tools/lldb-moduleimport-test/lldb-moduleimport-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ static bool validateModule(
llvm::outs() << "error: loadFromSerializedAST() failed\n";
return false;
}
CI.getLangOptions().EnableDeserializationSafety = false;

if (Verbose) {
if (!info.shortVersion.empty())
Expand Down
1 change: 1 addition & 0 deletions tools/sil-opt/SILOpt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ int main(int argc, char **argv) {
Invocation.getLangOptions().DisableAvailabilityChecking = true;
Invocation.getLangOptions().EnableAccessControl = false;
Invocation.getLangOptions().EnableObjCAttrRequiresFoundation = false;
Invocation.getLangOptions().EnableDeserializationSafety = false;
if (auto overrideKind = getASTOverrideKind()) {
Invocation.getLangOptions().ASTVerifierOverride = *overrideKind;
}
Expand Down
4 changes: 4 additions & 0 deletions tools/swift-ide-test/swift-ide-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3002,6 +3002,10 @@ static int doPrintModules(const CompilerInvocation &InitInvok,
bool SynthesizeExtensions) {
CompilerInvocation Invocation(InitInvok);

// Read everything from loaded modules, including internal details to
// test the behavior around non-public access levels.
Invocation.getLangOptions().EnableDeserializationSafety = false;

CompilerInstance CI;
// Display diagnostics to stderr.
PrintingDiagnosticConsumer PrintDiags;
Expand Down