Skip to content

C++/C#: Make escape analysis unsound by default #2667

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 8 commits into from
Jan 28, 2020
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: 8 additions & 0 deletions config/identical-files.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@
"cpp/ql/src/semmle/code/cpp/ir/implementation/IRType.qll",
"csharp/ql/src/semmle/code/csharp/ir/implementation/IRType.qll"
],
"IR IRConfiguration": [
"cpp/ql/src/semmle/code/cpp/ir/implementation/IRConfiguration.qll",
"csharp/ql/src/semmle/code/csharp/ir/implementation/IRConfiguration.qll"
],
"IR UseSoundEscapeAnalysis": [
"cpp/ql/src/semmle/code/cpp/ir/implementation/UseSoundEscapeAnalysis.qll",
"csharp/ql/src/semmle/code/csharp/ir/implementation/UseSoundEscapeAnalysis.qll"
],
"IR Operand Tag": [
"cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll",
"csharp/ql/src/semmle/code/csharp/ir/implementation/internal/OperandTag.qll"
Expand Down
19 changes: 19 additions & 0 deletions cpp/ql/src/semmle/code/cpp/ir/implementation/IRConfiguration.qll
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
/**
* Module used to configure the IR generation process.
*/

private import internal.IRConfigurationInternal

private newtype TIRConfiguration = MkIRConfiguration()
Expand All @@ -13,3 +17,18 @@ class IRConfiguration extends TIRConfiguration {
*/
predicate shouldCreateIRForFunction(Language::Function func) { any() }
}

private newtype TIREscapeAnalysisConfiguration = MkIREscapeAnalysisConfiguration()

/**
* The query can extend this class to control what escape analysis is used when generating SSA.
*/
class IREscapeAnalysisConfiguration extends TIREscapeAnalysisConfiguration {
string toString() { result = "IREscapeAnalysisConfiguration" }

/**
* Holds if the escape analysis done by SSA construction should be sound. By default, the SSA is
* built assuming that no variable's address ever escapes.
*/
predicate useSoundEscapeAnalysis() { none() }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import IRConfiguration

/**
* Overrides the default IR configuration to use sound escape analysis, instead of assuming that
* variable addresses never escape.
*/
class SoundEscapeAnalysisConfiguration extends IREscapeAnalysisConfiguration {
override predicate useSoundEscapeAnalysis() { any() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ private import AliasAnalysisInternal
private import cpp
private import InputIR
private import semmle.code.cpp.ir.internal.IntegerConstant as Ints
private import semmle.code.cpp.ir.implementation.IRConfiguration
private import semmle.code.cpp.models.interfaces.Alias

private class IntValue = Ints::IntValue;
Expand Down Expand Up @@ -277,9 +278,14 @@ private predicate automaticVariableAddressEscapes(IRAutomaticVariable var) {
* analysis.
*/
predicate variableAddressEscapes(IRVariable var) {
automaticVariableAddressEscapes(var.(IRAutomaticVariable))
exists(IREscapeAnalysisConfiguration config |
config.useSoundEscapeAnalysis() and
automaticVariableAddressEscapes(var.(IRAutomaticVariable))
)
or
// All variables with static storage duration have their address escape.
// All variables with static storage duration have their address escape, even when escape analysis
// is allowed to be unsound. Otherwise, we won't have a definition for any non-escaped global
// variable. Normally, we rely on `AliasedDefinition` to handle that.
not var instanceof IRAutomaticVariable
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class PropertyProvider extends IRPropertyProvider {
exists(
MemoryLocation useLocation, IRBlock predBlock, IRBlock defBlock, int defIndex, Overlap overlap
|
hasPhiOperandDefinition(_, useLocation, block, predBlock, defBlock, defIndex, overlap) and
hasPhiOperandDefinition(_, useLocation, block, predBlock, defBlock, defIndex) and
key = "PhiUse[" + useLocation.toString() + " from " + predBlock.getDisplayIndex().toString() +
"]" and
result = defBlock.getDisplayIndex().toString() + "_" + defIndex + " (" + overlap.toString() +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,93 +43,34 @@ abstract class TranslatedDeclarationEntry extends TranslatedElement, TTranslated
* Represents the IR translation of the declaration of a local variable,
* including its initialization, if any.
*/
abstract class TranslatedVariableDeclaration extends TranslatedElement, InitializationContext {
abstract class TranslatedLocalVariableDeclaration extends TranslatedVariableInitialization {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a rename without an associated change to what the values of the class are. Is that just a clarification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

/**
* Gets the local variable being declared.
*/
abstract LocalVariable getVariable();

override TranslatedElement getChild(int id) { id = 0 and result = getInitialization() }
final override Type getTargetType() { result = getVariableType(getVariable()) }

override Instruction getFirstInstruction() {
result = getInstruction(InitializerVariableAddressTag())
}

override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
tag = InitializerVariableAddressTag() and
opcode instanceof Opcode::VariableAddress and
resultType = getTypeForGLValue(getVariableType(getVariable()))
or
hasUninitializedInstruction() and
tag = InitializerStoreTag() and
opcode instanceof Opcode::Uninitialized and
resultType = getTypeForPRValue(getVariableType(getVariable()))
}

override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
(
tag = InitializerVariableAddressTag() and
kind instanceof GotoEdge and
if hasUninitializedInstruction()
then result = getInstruction(InitializerStoreTag())
else result = getInitialization().getFirstInstruction()
)
or
hasUninitializedInstruction() and
kind instanceof GotoEdge and
tag = InitializerStoreTag() and
(
result = getInitialization().getFirstInstruction()
or
not exists(getInitialization()) and result = getParent().getChildSuccessor(this)
)
}

override Instruction getChildSuccessor(TranslatedElement child) {
child = getInitialization() and result = getParent().getChildSuccessor(this)
}

override IRVariable getInstructionVariable(InstructionTag tag) {
(
tag = InitializerVariableAddressTag()
or
hasUninitializedInstruction() and tag = InitializerStoreTag()
) and
result = getIRUserVariable(getFunction(), getVariable())
}

override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
hasUninitializedInstruction() and
tag = InitializerStoreTag() and
operandTag instanceof AddressOperandTag and
result = getInstruction(InitializerVariableAddressTag())
}

override Instruction getTargetAddress() {
result = getInstruction(InitializerVariableAddressTag())
}

override Type getTargetType() { result = getVariableType(getVariable()) }

private TranslatedInitialization getInitialization() {
final override TranslatedInitialization getInitialization() {
result = getTranslatedInitialization(getVariable()
.getInitializer()
.getExpr()
.getFullyConverted())
}

private predicate hasUninitializedInstruction() {
not exists(getInitialization()) or
getInitialization() instanceof TranslatedListInitialization or
getInitialization() instanceof TranslatedConstructorInitialization or
getInitialization().(TranslatedStringLiteralInitialization).zeroInitRange(_, _)
final override Instruction getInitializationSuccessor() {
result = getParent().getChildSuccessor(this)
}

final override IRVariable getIRVariable() {
result = getIRUserVariable(getFunction(), getVariable())
}
}

/**
* Represents the IR translation of a local variable declaration within a declaration statement.
*/
class TranslatedVariableDeclarationEntry extends TranslatedVariableDeclaration,
class TranslatedVariableDeclarationEntry extends TranslatedLocalVariableDeclaration,
TranslatedDeclarationEntry {
LocalVariable var;

Expand All @@ -151,7 +92,7 @@ TranslatedRangeBasedForVariableDeclaration getTranslatedRangeBasedForVariableDec
/**
* Represents the IR translation of a compiler-generated variable in a range-based `for` loop.
*/
class TranslatedRangeBasedForVariableDeclaration extends TranslatedVariableDeclaration,
class TranslatedRangeBasedForVariableDeclaration extends TranslatedLocalVariableDeclaration,
TTranslatedRangeBasedForVariableDeclaration {
RangeBasedForStmt forStmt;
LocalVariable var;
Expand Down Expand Up @@ -181,7 +122,7 @@ TranslatedConditionDecl getTranslatedConditionDecl(ConditionDeclExpr expr) {
* }
* ```
*/
class TranslatedConditionDecl extends TranslatedVariableDeclaration, TTranslatedConditionDecl {
class TranslatedConditionDecl extends TranslatedLocalVariableDeclaration, TTranslatedConditionDecl {
ConditionDeclExpr conditionDeclExpr;

TranslatedConditionDecl() { this = TTranslatedConditionDecl(conditionDeclExpr) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1932,47 +1932,31 @@ abstract class TranslatedThrowExpr extends TranslatedNonConstantExpr {
* IR translation of a `throw` expression with an argument
* (e.g. `throw std::bad_alloc()`).
*/
class TranslatedThrowValueExpr extends TranslatedThrowExpr, InitializationContext {
class TranslatedThrowValueExpr extends TranslatedThrowExpr, TranslatedVariableInitialization {
TranslatedThrowValueExpr() { not expr instanceof ReThrowExpr }

override TranslatedElement getChild(int id) { id = 0 and result = getInitialization() }

override Instruction getFirstInstruction() {
result = getInstruction(InitializerVariableAddressTag())
}

override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
TranslatedThrowExpr.super.hasInstruction(opcode, tag, resultType)
or
tag = InitializerVariableAddressTag() and
opcode instanceof Opcode::VariableAddress and
resultType = getTypeForGLValue(getExceptionType())
TranslatedVariableInitialization.super.hasInstruction(opcode, tag, resultType)
}

override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
result = TranslatedThrowExpr.super.getInstructionSuccessor(tag, kind)
or
tag = InitializerVariableAddressTag() and
result = getInitialization().getFirstInstruction() and
kind instanceof GotoEdge
}

override Instruction getChildSuccessor(TranslatedElement child) {
child = getInitialization() and
result = getInstruction(ThrowTag())
result = TranslatedVariableInitialization.super.getInstructionSuccessor(tag, kind)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand what's going on here (the successor relations within each piece are exactly the ones in the respective superclass, and the connection between them is injected into TranslatedVariableInitialization by getInitializationSuccessor) but it seems a bit spooky to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a cleaner way of doing this would have been to make the actual (un)initialization part a separate element that is a child of the original element, i.e., composition instead of inheritance. That seemed like an even bigger refactoring in a change where I hadn't expected to do any refactoring to begin with. I can go back and improve this, but I'd prefer to do it in a follow-up post-January.

}

override IRVariable getInstructionVariable(InstructionTag tag) {
tag = InitializerVariableAddressTag() and
result = getIRTempVariable(expr, ThrowTempVar())
}
final override Instruction getInitializationSuccessor() { result = getInstruction(ThrowTag()) }

final override predicate hasTempVariable(TempVariableTag tag, CppType type) {
tag = ThrowTempVar() and
type = getTypeForPRValue(getExceptionType())
}

final override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
result = TranslatedVariableInitialization.super.getInstructionOperand(tag, operandTag)
or
tag = ThrowTag() and
(
operandTag instanceof AddressOperandTag and
Expand All @@ -1989,16 +1973,14 @@ class TranslatedThrowValueExpr extends TranslatedThrowExpr, InitializationContex
result = getTypeForPRValue(getExceptionType())
}

override Instruction getTargetAddress() {
result = getInstruction(InitializerVariableAddressTag())
}

override Type getTargetType() { result = getExceptionType() }

TranslatedInitialization getInitialization() {
final override TranslatedInitialization getInitialization() {
result = getTranslatedInitialization(expr.getExpr().getFullyConverted())
}

final override IRVariable getIRVariable() { result = getIRTempVariable(expr, ThrowTempVar()) }

final override Opcode getThrowOpcode() { result instanceof Opcode::ThrowValue }

private Type getExceptionType() { result = expr.getType() }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,98 @@ abstract class InitializationContext extends TranslatedElement {
abstract Type getTargetType();
}

/**
* Base class for any element that initializes a stack variable. Examples include local variable
* declarations, `return` statements, and `throw` expressions.
*/
abstract class TranslatedVariableInitialization extends TranslatedElement, InitializationContext {
final override TranslatedElement getChild(int id) { id = 0 and result = getInitialization() }

final override Instruction getFirstInstruction() {
result = getInstruction(InitializerVariableAddressTag())
}

override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
tag = InitializerVariableAddressTag() and
opcode instanceof Opcode::VariableAddress and
resultType = getTypeForGLValue(getTargetType())
or
hasUninitializedInstruction() and
tag = InitializerStoreTag() and
opcode instanceof Opcode::Uninitialized and
resultType = getTypeForPRValue(getTargetType())
}

override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
(
tag = InitializerVariableAddressTag() and
kind instanceof GotoEdge and
if hasUninitializedInstruction()
then result = getInstruction(InitializerStoreTag())
else result = getInitialization().getFirstInstruction()
)
or
hasUninitializedInstruction() and
kind instanceof GotoEdge and
tag = InitializerStoreTag() and
(
result = getInitialization().getFirstInstruction()
or
not exists(getInitialization()) and result = getInitializationSuccessor()
)
}

final override Instruction getChildSuccessor(TranslatedElement child) {
child = getInitialization() and result = getInitializationSuccessor()
}

override Instruction getInstructionOperand(InstructionTag tag, OperandTag operandTag) {
hasUninitializedInstruction() and
tag = InitializerStoreTag() and
operandTag instanceof AddressOperandTag and
result = getInstruction(InitializerVariableAddressTag())
}

final override IRVariable getInstructionVariable(InstructionTag tag) {
(
tag = InitializerVariableAddressTag()
or
hasUninitializedInstruction() and tag = InitializerStoreTag()
) and
result = getIRVariable()
}

final override Instruction getTargetAddress() {
result = getInstruction(InitializerVariableAddressTag())
}

/**
* Get the initialization for the variable.
*/
abstract TranslatedInitialization getInitialization();

/**
* Get the `IRVariable` to be initialized. This may be an `IRTempVariable`.
*/
abstract IRVariable getIRVariable();

/**
* Gets the `Instruction` to be executed immediately after the initialization.
*/
abstract Instruction getInitializationSuccessor();

/**
* Holds if this initialization requires an `Uninitialized` instruction to be emitted before
* evaluating the initializer.
*/
final predicate hasUninitializedInstruction() {
not exists(getInitialization()) or
getInitialization() instanceof TranslatedListInitialization or
getInitialization() instanceof TranslatedConstructorInitialization or
getInitialization().(TranslatedStringLiteralInitialization).zeroInitRange(_, _)
}
}

/**
* Represents the IR translation of any initialization, whether from an
* initializer list or from a direct initializer.
Expand Down
Loading