From 533eeff7e8e145b8b7b82316242f248fabb2220d Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Wed, 27 May 2020 14:06:59 -0400 Subject: [PATCH 1/2] C++: Fix `MemoryLocation` with multiple `VirtualVariables` While investigating a bug with `TInstruction` sharing, I discovered that we had a case where alias analysis could create two `VirtualVariable`s for the same `Allocation`. For an indirect parameter allocation, we were using the type of the pointer variable as the type of the indirect allocation, instead of just `Unknown`. If the `IRType` of the pointer variable was the same type as the type of at least one access to the indirect allocation, we'd create both an `EntireAllocationVirtualVariable` and a `VariableVirtualVariable` for the allocation. I added a new consistency test to guard against this in the future. This also turned out to be the root cause of the one existing known consistency failure in the IR tests. --- .../internal/AliasConfiguration.qll | 2 +- .../aliased_ssa/internal/SSAConstruction.qll | 27 +++++++++++++++++++ .../internal/SSAConstruction.qll | 27 +++++++++++++++++++ .../ir/aliased_ssa_ssa_consistency.expected | 1 + ...iased_ssa_ssa_consistency_unsound.expected | 1 + .../ir/unaliased_ssa_ssa_consistency.expected | 1 + ...iased_ssa_ssa_consistency_unsound.expected | 1 + .../aliased_ssa_consistency_unsound.expected | 1 - .../ir/ssa/aliased_ssa_ir_unsound.expected | 4 +-- .../ssa/aliased_ssa_ssa_consistency.expected | 1 + ...iased_ssa_ssa_consistency_unsound.expected | 1 + .../unaliased_ssa_ssa_consistency.expected | 1 + ...iased_ssa_ssa_consistency_unsound.expected | 1 + .../internal/SSAConstruction.qll | 27 +++++++++++++++++++ .../ir/unaliased_ssa_ssa_consistency.expected | 1 + 15 files changed, 93 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll index e95086c89fc9..9121fb9f98b3 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll @@ -90,7 +90,7 @@ class IndirectParameterAllocation extends Allocation, TIndirectParameterAllocati final override string getUniqueId() { result = var.getUniqueId() } - final override IRType getIRType() { result = var.getIRType() } + final override IRType getIRType() { result instanceof IRUnknownType } final override predicate isReadOnly() { none() } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll index 48aa96c6c1ac..30414bb5db3a 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll @@ -913,6 +913,9 @@ private module CachedForDebugging { } module SSAConsistency { + /** + * Holds if a `MemoryOperand` has more than one `MemoryLocation` assigned by alias analysis. + */ query predicate multipleOperandMemoryLocations( OldIR::MemoryOperand operand, string message, OldIR::IRFunction func, string funcText ) { @@ -925,6 +928,9 @@ module SSAConsistency { ) } + /** + * Holds if a `MemoryLocation` does not have an associated `VirtualVariable`. + */ query predicate missingVirtualVariableForMemoryLocation( Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText ) { @@ -933,4 +939,25 @@ module SSAConsistency { funcText = Language::getIdentityString(func.getFunction()) and message = "Memory location has no virtual variable in function '$@'." } + + /** + * Holds if a `MemoryLocation` is a member of more than one `VirtualVariable`. + */ + query predicate multipleVirtualVariablesForMemoryLocation( + Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText + ) { + exists(int vvarCount | + vvarCount = strictcount(location.getVirtualVariable()) and + vvarCount > 1 and + func = location.getIRFunction() and + funcText = Language::getIdentityString(func.getFunction()) and + message = + "Memory location has " + vvarCount.toString() + " virtual variables in function '$@': (" + + concat(Alias::VirtualVariable vvar | + vvar = location.getVirtualVariable() + | + vvar.toString(), ", " + ) + ")." + ) + } } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll index 48aa96c6c1ac..30414bb5db3a 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll @@ -913,6 +913,9 @@ private module CachedForDebugging { } module SSAConsistency { + /** + * Holds if a `MemoryOperand` has more than one `MemoryLocation` assigned by alias analysis. + */ query predicate multipleOperandMemoryLocations( OldIR::MemoryOperand operand, string message, OldIR::IRFunction func, string funcText ) { @@ -925,6 +928,9 @@ module SSAConsistency { ) } + /** + * Holds if a `MemoryLocation` does not have an associated `VirtualVariable`. + */ query predicate missingVirtualVariableForMemoryLocation( Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText ) { @@ -933,4 +939,25 @@ module SSAConsistency { funcText = Language::getIdentityString(func.getFunction()) and message = "Memory location has no virtual variable in function '$@'." } + + /** + * Holds if a `MemoryLocation` is a member of more than one `VirtualVariable`. + */ + query predicate multipleVirtualVariablesForMemoryLocation( + Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText + ) { + exists(int vvarCount | + vvarCount = strictcount(location.getVirtualVariable()) and + vvarCount > 1 and + func = location.getIRFunction() and + funcText = Language::getIdentityString(func.getFunction()) and + message = + "Memory location has " + vvarCount.toString() + " virtual variables in function '$@': (" + + concat(Alias::VirtualVariable vvar | + vvar = location.getVirtualVariable() + | + vvar.toString(), ", " + ) + ")." + ) + } } diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency_unsound.expected b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency_unsound.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_ssa_consistency_unsound.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected +++ b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency_unsound.expected b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency_unsound.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency_unsound.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_consistency_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_consistency_unsound.expected index 90f8331598cf..e2db1e65034c 100644 --- a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_consistency_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_consistency_unsound.expected @@ -20,7 +20,6 @@ switchInstructionWithoutDefaultEdge notMarkedAsConflated wronglyMarkedAsConflated invalidOverlap -| ssa.cpp:301:27:301:30 | SideEffect | MemoryOperand 'SideEffect' has a `getDefinitionOverlap()` of 'MayPartiallyOverlap'. | ssa.cpp:301:5:301:8 | IR: main | int main(int, char**) | missingCanonicalLanguageType multipleCanonicalLanguageTypes missingIRType diff --git a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected index e20e499a12bd..b080823add69 100644 --- a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ir_unsound.expected @@ -1426,7 +1426,7 @@ ssa.cpp: # 302| m302_8(unknown) = Chi : total:m301_4, partial:m302_7 # 302| v302_9(void) = ^BufferReadSideEffect[1] : &:r302_5, ~m301_10 # 302| m302_10(unknown) = ^BufferMayWriteSideEffect[1] : &:r302_5 -# 302| m302_11(char *) = Chi : total:m301_10, partial:m302_10 +# 302| m302_11(unknown) = Chi : total:m301_10, partial:m302_10 # 303| r303_1(glval) = FunctionAddress[unknownFunction] : # 303| r303_2(glval) = VariableAddress[argc] : # 303| r303_3(int) = Load : &:r303_2, m301_6 @@ -1437,7 +1437,7 @@ ssa.cpp: # 303| m303_8(unknown) = Chi : total:m302_8, partial:m303_7 # 303| v303_9(void) = ^BufferReadSideEffect[1] : &:r303_5, ~m302_11 # 303| m303_10(unknown) = ^BufferMayWriteSideEffect[1] : &:r303_5 -# 303| m303_11(char *) = Chi : total:m302_11, partial:m303_10 +# 303| m303_11(unknown) = Chi : total:m302_11, partial:m303_10 # 304| r304_1(glval) = VariableAddress[#return] : # 304| r304_2(glval) = VariableAddress[argv] : # 304| r304_3(char **) = Load : &:r304_2, m301_8 diff --git a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency.expected b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency.expected +++ b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency_unsound.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ssa/aliased_ssa_ssa_consistency_unsound.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency.expected b/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency.expected +++ b/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency_unsound.expected b/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency_unsound.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency_unsound.expected +++ b/cpp/ql/test/library-tests/ir/ssa/unaliased_ssa_ssa_consistency_unsound.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll index 48aa96c6c1ac..30414bb5db3a 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll @@ -913,6 +913,9 @@ private module CachedForDebugging { } module SSAConsistency { + /** + * Holds if a `MemoryOperand` has more than one `MemoryLocation` assigned by alias analysis. + */ query predicate multipleOperandMemoryLocations( OldIR::MemoryOperand operand, string message, OldIR::IRFunction func, string funcText ) { @@ -925,6 +928,9 @@ module SSAConsistency { ) } + /** + * Holds if a `MemoryLocation` does not have an associated `VirtualVariable`. + */ query predicate missingVirtualVariableForMemoryLocation( Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText ) { @@ -933,4 +939,25 @@ module SSAConsistency { funcText = Language::getIdentityString(func.getFunction()) and message = "Memory location has no virtual variable in function '$@'." } + + /** + * Holds if a `MemoryLocation` is a member of more than one `VirtualVariable`. + */ + query predicate multipleVirtualVariablesForMemoryLocation( + Alias::MemoryLocation location, string message, OldIR::IRFunction func, string funcText + ) { + exists(int vvarCount | + vvarCount = strictcount(location.getVirtualVariable()) and + vvarCount > 1 and + func = location.getIRFunction() and + funcText = Language::getIdentityString(func.getFunction()) and + message = + "Memory location has " + vvarCount.toString() + " virtual variables in function '$@': (" + + concat(Alias::VirtualVariable vvar | + vvar = location.getVirtualVariable() + | + vvar.toString(), ", " + ) + ")." + ) + } } diff --git a/csharp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected b/csharp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected index 7c2d1faf6391..21782bd5ef14 100644 --- a/csharp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected +++ b/csharp/ql/test/library-tests/ir/ir/unaliased_ssa_ssa_consistency.expected @@ -1,2 +1,3 @@ multipleOperandMemoryLocations missingVirtualVariableForMemoryLocation +multipleVirtualVariablesForMemoryLocation From 01ef8795bf08ee16a1ee8e28c363a7d59b9c9163 Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Thu, 28 May 2020 17:24:38 -0400 Subject: [PATCH 2/2] C++: Updated fixed test expectation --- .../dataflow/fields/dataflow-ir-consistency.expected | 2 -- 1 file changed, 2 deletions(-) diff --git a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected index 9d0470bd4b25..ba7e3bc01257 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected @@ -1,7 +1,5 @@ uniqueEnclosingCallable uniqueTypeBound -| by_reference.cpp:106:21:106:41 | Chi | Node should have one type bound but has 2. | -| by_reference.cpp:126:21:126:40 | Chi | Node should have one type bound but has 2. | uniqueTypeRepr uniqueNodeLocation | D.cpp:1:17:1:17 | o | Node should have one location but has 3. |