From 0c9466640ab60622d46928f37a594b0ed82ea0cf Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 18 Mar 2020 16:26:30 +0000 Subject: [PATCH 1/9] C++: Add IR test for strcpy/strcat. --- .../library-tests/ir/ir/PrintAST.expected | 70 +++++++++++++++++++ cpp/ql/test/library-tests/ir/ir/ir.cpp | 12 ++++ .../test/library-tests/ir/ir/raw_ir.expected | 56 +++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/cpp/ql/test/library-tests/ir/ir/PrintAST.expected b/cpp/ql/test/library-tests/ir/ir/PrintAST.expected index 2be5a2019f84..d7ee37f3edf8 100644 --- a/cpp/ql/test/library-tests/ir/ir/PrintAST.expected +++ b/cpp/ql/test/library-tests/ir/ir/PrintAST.expected @@ -8303,6 +8303,76 @@ ir.cpp: # 1219| Type = [IntType] int # 1219| ValueCategory = prvalue(load) # 1220| 4: [ReturnStmt] return ... +# 1224| [TopLevelFunction] char* strcpy(char*, char const*) +# 1224| params: +# 1224| 0: [Parameter] destination +# 1224| Type = [CharPointerType] char * +# 1224| 1: [Parameter] source +# 1224| Type = [PointerType] const char * +# 1225| [TopLevelFunction] char* strcat(char*, char const*) +# 1225| params: +# 1225| 0: [Parameter] destination +# 1225| Type = [CharPointerType] char * +# 1225| 1: [Parameter] source +# 1225| Type = [PointerType] const char * +# 1227| [TopLevelFunction] void test_strings(char*, char*) +# 1227| params: +# 1227| 0: [Parameter] s1 +# 1227| Type = [CharPointerType] char * +# 1227| 1: [Parameter] s2 +# 1227| Type = [CharPointerType] char * +# 1227| body: [Block] { ... } +# 1228| 0: [DeclStmt] declaration +# 1228| 0: [VariableDeclarationEntry] definition of buffer +# 1228| Type = [ArrayType] char[1024] +# 1228| init: [Initializer] initializer for buffer +# 1228| expr: [ArrayAggregateLiteral] {...} +# 1228| Type = [ArrayType] char[1024] +# 1228| ValueCategory = prvalue +# 1228| [0]: [CStyleCast] (char)... +# 1228| Conversion = [IntegralConversion] integral conversion +# 1228| Type = [PlainCharType] char +# 1228| Value = [CStyleCast] 0 +# 1228| ValueCategory = prvalue +# 1228| expr: [Literal] 0 +# 1228| Type = [IntType] int +# 1228| Value = [Literal] 0 +# 1228| ValueCategory = prvalue +# 1230| 1: [ExprStmt] ExprStmt +# 1230| 0: [FunctionCall] call to strcpy +# 1230| Type = [CharPointerType] char * +# 1230| ValueCategory = prvalue +# 1230| 0: [ArrayToPointerConversion] array to pointer conversion +# 1230| Type = [CharPointerType] char * +# 1230| ValueCategory = prvalue +# 1230| expr: [VariableAccess] buffer +# 1230| Type = [ArrayType] char[1024] +# 1230| ValueCategory = lvalue +# 1230| 1: [CStyleCast] (const char *)... +# 1230| Conversion = [PointerConversion] pointer conversion +# 1230| Type = [PointerType] const char * +# 1230| ValueCategory = prvalue +# 1230| expr: [VariableAccess] s1 +# 1230| Type = [CharPointerType] char * +# 1230| ValueCategory = prvalue(load) +# 1231| 2: [ExprStmt] ExprStmt +# 1231| 0: [FunctionCall] call to strcat +# 1231| Type = [CharPointerType] char * +# 1231| ValueCategory = prvalue +# 1231| 0: [ArrayToPointerConversion] array to pointer conversion +# 1231| Type = [CharPointerType] char * +# 1231| ValueCategory = prvalue +# 1231| expr: [VariableAccess] buffer +# 1231| Type = [ArrayType] char[1024] +# 1231| ValueCategory = lvalue +# 1231| 1: [CStyleCast] (const char *)... +# 1231| Conversion = [PointerConversion] pointer conversion +# 1231| Type = [PointerType] const char * +# 1231| ValueCategory = prvalue +# 1231| expr: [VariableAccess] s2 +# 1231| Type = [CharPointerType] char * +# 1231| ValueCategory = prvalue(load) +# 1232| 3: [ReturnStmt] return ... perf-regression.cpp: # 4| [CopyAssignmentOperator] Big& Big::operator=(Big const&) # 4| params: diff --git a/cpp/ql/test/library-tests/ir/ir/ir.cpp b/cpp/ql/test/library-tests/ir/ir/ir.cpp index c3a394f09857..34a977100746 100644 --- a/cpp/ql/test/library-tests/ir/ir/ir.cpp +++ b/cpp/ql/test/library-tests/ir/ir/ir.cpp @@ -1219,4 +1219,16 @@ void switch2Case_default(int x) { int z = y; } +// --- strings --- + +char *strcpy(char *destination, const char *source); +char *strcat(char *destination, const char *source); + +void test_strings(char *s1, char *s2) { + char buffer[1024] = {0}; + + strcpy(buffer, s1); + strcat(buffer, s2); +} + // semmle-extractor-options: -std=c++17 --clang diff --git a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected index f860574bb4c7..2b199cb618e1 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected @@ -6228,6 +6228,62 @@ ir.cpp: # 1205| v1205_9(void) = AliasedUse : ~mu1205_4 # 1205| v1205_10(void) = ExitFunction : +# 1227| void test_strings(char*, char*) +# 1227| Block 0 +# 1227| v1227_1(void) = EnterFunction : +# 1227| mu1227_2(unknown) = AliasedDefinition : +# 1227| mu1227_3(unknown) = InitializeNonLocal : +# 1227| mu1227_4(unknown) = UnmodeledDefinition : +# 1227| r1227_5(glval) = VariableAddress[s1] : +# 1227| mu1227_6(char *) = InitializeParameter[s1] : &:r1227_5 +# 1227| r1227_7(char *) = Load : &:r1227_5, ~mu1227_6 +# 1227| mu1227_8(unknown) = InitializeIndirection[s1] : &:r1227_7 +# 1227| r1227_9(glval) = VariableAddress[s2] : +# 1227| mu1227_10(char *) = InitializeParameter[s2] : &:r1227_9 +# 1227| r1227_11(char *) = Load : &:r1227_9, ~mu1227_10 +# 1227| mu1227_12(unknown) = InitializeIndirection[s2] : &:r1227_11 +# 1228| r1228_1(glval) = VariableAddress[buffer] : +# 1228| mu1228_2(char[1024]) = Uninitialized[buffer] : &:r1228_1 +# 1228| r1228_3(int) = Constant[0] : +# 1228| r1228_4(glval) = PointerAdd[1] : r1228_1, r1228_3 +# 1228| r1228_5(char) = Constant[0] : +# 1228| mu1228_6(char) = Store : &:r1228_4, r1228_5 +# 1228| r1228_7(int) = Constant[1] : +# 1228| r1228_8(glval) = PointerAdd[1] : r1228_1, r1228_7 +# 1228| r1228_9(unknown[1023]) = Constant[0] : +# 1228| mu1228_10(unknown[1023]) = Store : &:r1228_8, r1228_9 +# 1230| r1230_1(glval) = FunctionAddress[strcpy] : +# 1230| r1230_2(glval) = VariableAddress[buffer] : +# 1230| r1230_3(char *) = Convert : r1230_2 +# 1230| r1230_4(glval) = VariableAddress[s1] : +# 1230| r1230_5(char *) = Load : &:r1230_4, ~mu1227_4 +# 1230| r1230_6(char *) = Convert : r1230_5 +# 1230| r1230_7(char *) = Call : func:r1230_1, 0:r1230_3, 1:r1230_6 +# 1230| mu1230_8(unknown) = ^CallSideEffect : ~mu1227_4 +# 1230| v1230_9(void) = ^BufferReadSideEffect[0] : &:r1230_3, ~mu1227_4 +# 1230| v1230_10(void) = ^BufferReadSideEffect[1] : &:r1230_6, ~mu1227_4 +# 1230| mu1230_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1230_3 +# 1230| mu1230_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1230_6 +# 1231| r1231_1(glval) = FunctionAddress[strcat] : +# 1231| r1231_2(glval) = VariableAddress[buffer] : +# 1231| r1231_3(char *) = Convert : r1231_2 +# 1231| r1231_4(glval) = VariableAddress[s2] : +# 1231| r1231_5(char *) = Load : &:r1231_4, ~mu1227_4 +# 1231| r1231_6(char *) = Convert : r1231_5 +# 1231| r1231_7(char *) = Call : func:r1231_1, 0:r1231_3, 1:r1231_6 +# 1231| mu1231_8(unknown) = ^CallSideEffect : ~mu1227_4 +# 1231| v1231_9(void) = ^BufferReadSideEffect[0] : &:r1231_3, ~mu1227_4 +# 1231| v1231_10(void) = ^BufferReadSideEffect[1] : &:r1231_6, ~mu1227_4 +# 1231| mu1231_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1231_3 +# 1231| mu1231_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1231_6 +# 1232| v1232_1(void) = NoOp : +# 1227| v1227_13(void) = ReturnIndirection : &:r1227_7, ~mu1227_4 +# 1227| v1227_14(void) = ReturnIndirection : &:r1227_11, ~mu1227_4 +# 1227| v1227_15(void) = ReturnVoid : +# 1227| v1227_16(void) = UnmodeledUse : mu* +# 1227| v1227_17(void) = AliasedUse : ~mu1227_4 +# 1227| v1227_18(void) = ExitFunction : + perf-regression.cpp: # 6| void Big::Big() # 6| Block 0 From 935b8d96f8df153233513ec274cf8584bcfa9536 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 18 Mar 2020 16:39:22 +0000 Subject: [PATCH 2/9] C++: Offset .expected for cleaner diff. --- .../test/library-tests/ir/ir/raw_ir.expected | 106 +++++++++--------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected index 2b199cb618e1..3c10bd83d7fe 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected @@ -6230,59 +6230,59 @@ ir.cpp: # 1227| void test_strings(char*, char*) # 1227| Block 0 -# 1227| v1227_1(void) = EnterFunction : -# 1227| mu1227_2(unknown) = AliasedDefinition : -# 1227| mu1227_3(unknown) = InitializeNonLocal : -# 1227| mu1227_4(unknown) = UnmodeledDefinition : -# 1227| r1227_5(glval) = VariableAddress[s1] : -# 1227| mu1227_6(char *) = InitializeParameter[s1] : &:r1227_5 -# 1227| r1227_7(char *) = Load : &:r1227_5, ~mu1227_6 -# 1227| mu1227_8(unknown) = InitializeIndirection[s1] : &:r1227_7 -# 1227| r1227_9(glval) = VariableAddress[s2] : -# 1227| mu1227_10(char *) = InitializeParameter[s2] : &:r1227_9 -# 1227| r1227_11(char *) = Load : &:r1227_9, ~mu1227_10 -# 1227| mu1227_12(unknown) = InitializeIndirection[s2] : &:r1227_11 -# 1228| r1228_1(glval) = VariableAddress[buffer] : -# 1228| mu1228_2(char[1024]) = Uninitialized[buffer] : &:r1228_1 -# 1228| r1228_3(int) = Constant[0] : -# 1228| r1228_4(glval) = PointerAdd[1] : r1228_1, r1228_3 -# 1228| r1228_5(char) = Constant[0] : -# 1228| mu1228_6(char) = Store : &:r1228_4, r1228_5 -# 1228| r1228_7(int) = Constant[1] : -# 1228| r1228_8(glval) = PointerAdd[1] : r1228_1, r1228_7 -# 1228| r1228_9(unknown[1023]) = Constant[0] : -# 1228| mu1228_10(unknown[1023]) = Store : &:r1228_8, r1228_9 -# 1230| r1230_1(glval) = FunctionAddress[strcpy] : -# 1230| r1230_2(glval) = VariableAddress[buffer] : -# 1230| r1230_3(char *) = Convert : r1230_2 -# 1230| r1230_4(glval) = VariableAddress[s1] : -# 1230| r1230_5(char *) = Load : &:r1230_4, ~mu1227_4 -# 1230| r1230_6(char *) = Convert : r1230_5 -# 1230| r1230_7(char *) = Call : func:r1230_1, 0:r1230_3, 1:r1230_6 -# 1230| mu1230_8(unknown) = ^CallSideEffect : ~mu1227_4 -# 1230| v1230_9(void) = ^BufferReadSideEffect[0] : &:r1230_3, ~mu1227_4 -# 1230| v1230_10(void) = ^BufferReadSideEffect[1] : &:r1230_6, ~mu1227_4 -# 1230| mu1230_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1230_3 -# 1230| mu1230_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1230_6 -# 1231| r1231_1(glval) = FunctionAddress[strcat] : -# 1231| r1231_2(glval) = VariableAddress[buffer] : -# 1231| r1231_3(char *) = Convert : r1231_2 -# 1231| r1231_4(glval) = VariableAddress[s2] : -# 1231| r1231_5(char *) = Load : &:r1231_4, ~mu1227_4 -# 1231| r1231_6(char *) = Convert : r1231_5 -# 1231| r1231_7(char *) = Call : func:r1231_1, 0:r1231_3, 1:r1231_6 -# 1231| mu1231_8(unknown) = ^CallSideEffect : ~mu1227_4 -# 1231| v1231_9(void) = ^BufferReadSideEffect[0] : &:r1231_3, ~mu1227_4 -# 1231| v1231_10(void) = ^BufferReadSideEffect[1] : &:r1231_6, ~mu1227_4 -# 1231| mu1231_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1231_3 -# 1231| mu1231_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1231_6 -# 1232| v1232_1(void) = NoOp : -# 1227| v1227_13(void) = ReturnIndirection : &:r1227_7, ~mu1227_4 -# 1227| v1227_14(void) = ReturnIndirection : &:r1227_11, ~mu1227_4 -# 1227| v1227_15(void) = ReturnVoid : -# 1227| v1227_16(void) = UnmodeledUse : mu* -# 1227| v1227_17(void) = AliasedUse : ~mu1227_4 -# 1227| v1227_18(void) = ExitFunction : +# 1227| v1227_1(void) = EnterFunction : +# 1227| mu1227_2(unknown) = AliasedDefinition : +# 1227| mu1227_3(unknown) = InitializeNonLocal : +# 1227| mu1227_4(unknown) = UnmodeledDefinition : +# 1227| r1227_5(glval) = VariableAddress[s1] : +# 1227| mu1227_6(char *) = InitializeParameter[s1] : &:r1227_5 +# 1227| r1227_7(char *) = Load : &:r1227_5, ~mu1227_6 +# 1227| mu1227_8(unknown) = InitializeIndirection[s1] : &:r1227_7 +# 1227| r1227_9(glval) = VariableAddress[s2] : +# 1227| mu1227_10(char *) = InitializeParameter[s2] : &:r1227_9 +# 1227| r1227_11(char *) = Load : &:r1227_9, ~mu1227_10 +# 1227| mu1227_12(unknown) = InitializeIndirection[s2] : &:r1227_11 +# 1228| r1228_1(glval) = VariableAddress[buffer] : +# 1228| mu1228_2(char[1024]) = Uninitialized[buffer] : &:r1228_1 +# 1228| r1228_3(int) = Constant[0] : +# 1228| r1228_4(glval) = PointerAdd[1] : r1228_1, r1228_3 +# 1228| r1228_5(char) = Constant[0] : +# 1228| mu1228_6(char) = Store : &:r1228_4, r1228_5 +# 1228| r1228_7(int) = Constant[1] : +# 1228| r1228_8(glval) = PointerAdd[1] : r1228_1, r1228_7 +# 1228| r1228_9(unknown[1023]) = Constant[0] : +# 1228| mu1228_10(unknown[1023]) = Store : &:r1228_8, r1228_9 +# 1230| r1230_1(glval) = FunctionAddress[strcpy] : +# 1230| r1230_2(glval) = VariableAddress[buffer] : +# 1230| r1230_3(char *) = Convert : r1230_2 +# 1230| r1230_4(glval) = VariableAddress[s1] : +# 1230| r1230_5(char *) = Load : &:r1230_4, ~mu1227_4 +# 1230| r1230_6(char *) = Convert : r1230_5 +# 1230| r1230_7(char *) = Call : func:r1230_1, 0:r1230_3, 1:r1230_6 +# 1230| mu1230_8(unknown) = ^CallSideEffect : ~mu1227_4 +# 1230| v1230_9(void) = ^BufferReadSideEffect[0] : &:r1230_3, ~mu1227_4 +# 1230| v1230_10(void) = ^BufferReadSideEffect[1] : &:r1230_6, ~mu1227_4 +# 1230| mu1230_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1230_3 +# 1230| mu1230_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1230_6 +# 1231| r1231_1(glval) = FunctionAddress[strcat] : +# 1231| r1231_2(glval) = VariableAddress[buffer] : +# 1231| r1231_3(char *) = Convert : r1231_2 +# 1231| r1231_4(glval) = VariableAddress[s2] : +# 1231| r1231_5(char *) = Load : &:r1231_4, ~mu1227_4 +# 1231| r1231_6(char *) = Convert : r1231_5 +# 1231| r1231_7(char *) = Call : func:r1231_1, 0:r1231_3, 1:r1231_6 +# 1231| mu1231_8(unknown) = ^CallSideEffect : ~mu1227_4 +# 1231| v1231_9(void) = ^BufferReadSideEffect[0] : &:r1231_3, ~mu1227_4 +# 1231| v1231_10(void) = ^BufferReadSideEffect[1] : &:r1231_6, ~mu1227_4 +# 1231| mu1231_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1231_3 +# 1231| mu1231_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1231_6 +# 1232| v1232_1(void) = NoOp : +# 1227| v1227_13(void) = ReturnIndirection : &:r1227_7, ~mu1227_4 +# 1227| v1227_14(void) = ReturnIndirection : &:r1227_11, ~mu1227_4 +# 1227| v1227_15(void) = ReturnVoid : +# 1227| v1227_16(void) = UnmodeledUse : mu* +# 1227| v1227_17(void) = AliasedUse : ~mu1227_4 +# 1227| v1227_18(void) = ExitFunction : perf-regression.cpp: # 6| void Big::Big() From 6cc1c2341c8272c341c369995d9263518cc8e0f0 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 18 Mar 2020 12:53:35 +0000 Subject: [PATCH 3/9] C++: Add some SideEffect models. --- .../cpp/models/implementations/Strcat.qll | 18 ++++++++++++++- .../cpp/models/implementations/Strcpy.qll | 22 ++++++++++++++++++- .../test/library-tests/ir/ir/raw_ir.expected | 15 +++++-------- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll index d56ebf10bbca..998329a3e47f 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll @@ -1,11 +1,12 @@ import semmle.code.cpp.models.interfaces.ArrayFunction import semmle.code.cpp.models.interfaces.DataFlow import semmle.code.cpp.models.interfaces.Taint +import semmle.code.cpp.models.interfaces.SideEffect /** * The standard function `strcat` and its wide, sized, and Microsoft variants. */ -class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction { +class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction { StrcatFunction() { exists(string name | name = getName() | name = "strcat" or // strcat(dst, src) @@ -56,4 +57,19 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction { override predicate hasArrayWithNullTerminator(int param) { param = 1 } override predicate hasArrayWithUnknownSize(int param) { param = 0 } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + i = 0 and + buffer = true and + mustWrite = false + } + + predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + (i = 0 or i = 1) and + buffer = true + } } diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index d4d7bcf28817..5fd59e1df54c 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -1,11 +1,12 @@ import semmle.code.cpp.models.interfaces.ArrayFunction import semmle.code.cpp.models.interfaces.DataFlow import semmle.code.cpp.models.interfaces.Taint +import semmle.code.cpp.models.interfaces.SideEffect /** * The standard function `strcpy` and its wide, sized, and Microsoft variants. */ -class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction { +class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction { StrcpyFunction() { this.hasName("strcpy") or this.hasName("_mbscpy") or @@ -74,4 +75,23 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction { output.isReturnValueDeref() ) } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + i = 0 and + buffer = true and + mustWrite = true + } + + predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + i = 1 and + buffer = true + } + + ParameterIndex getParameterSizeIndex(ParameterIndex i) { + hasArrayWithVariableSize(i, result) + } } diff --git a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected index 3c10bd83d7fe..eb8bdec2a468 100644 --- a/cpp/ql/test/library-tests/ir/ir/raw_ir.expected +++ b/cpp/ql/test/library-tests/ir/ir/raw_ir.expected @@ -6259,11 +6259,8 @@ ir.cpp: # 1230| r1230_5(char *) = Load : &:r1230_4, ~mu1227_4 # 1230| r1230_6(char *) = Convert : r1230_5 # 1230| r1230_7(char *) = Call : func:r1230_1, 0:r1230_3, 1:r1230_6 -# 1230| mu1230_8(unknown) = ^CallSideEffect : ~mu1227_4 -# 1230| v1230_9(void) = ^BufferReadSideEffect[0] : &:r1230_3, ~mu1227_4 -# 1230| v1230_10(void) = ^BufferReadSideEffect[1] : &:r1230_6, ~mu1227_4 -# 1230| mu1230_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1230_3 -# 1230| mu1230_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1230_6 +# 1230| v1230_8(void) = ^BufferReadSideEffect[1] : &:r1230_6, ~mu1227_4 +# 1230| mu1230_9(unknown) = ^BufferMustWriteSideEffect[0] : &:r1230_3 # 1231| r1231_1(glval) = FunctionAddress[strcat] : # 1231| r1231_2(glval) = VariableAddress[buffer] : # 1231| r1231_3(char *) = Convert : r1231_2 @@ -6271,11 +6268,9 @@ ir.cpp: # 1231| r1231_5(char *) = Load : &:r1231_4, ~mu1227_4 # 1231| r1231_6(char *) = Convert : r1231_5 # 1231| r1231_7(char *) = Call : func:r1231_1, 0:r1231_3, 1:r1231_6 -# 1231| mu1231_8(unknown) = ^CallSideEffect : ~mu1227_4 -# 1231| v1231_9(void) = ^BufferReadSideEffect[0] : &:r1231_3, ~mu1227_4 -# 1231| v1231_10(void) = ^BufferReadSideEffect[1] : &:r1231_6, ~mu1227_4 -# 1231| mu1231_11(unknown) = ^BufferMayWriteSideEffect[0] : &:r1231_3 -# 1231| mu1231_12(unknown) = ^BufferMayWriteSideEffect[1] : &:r1231_6 +# 1231| v1231_8(void) = ^BufferReadSideEffect[0] : &:r1231_3, ~mu1227_4 +# 1231| v1231_9(void) = ^BufferReadSideEffect[1] : &:r1231_6, ~mu1227_4 +# 1231| mu1231_10(unknown) = ^BufferMayWriteSideEffect[0] : &:r1231_3 # 1232| v1232_1(void) = NoOp : # 1227| v1227_13(void) = ReturnIndirection : &:r1227_7, ~mu1227_4 # 1227| v1227_14(void) = ReturnIndirection : &:r1227_11, ~mu1227_4 From f32e84b1d0a2253b6df496346c591e5aace22372 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 18 Mar 2020 17:24:46 +0000 Subject: [PATCH 4/9] C++: Autoformat. --- cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index 5fd59e1df54c..80bc9ef29dbb 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -91,7 +91,5 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid buffer = true } - ParameterIndex getParameterSizeIndex(ParameterIndex i) { - hasArrayWithVariableSize(i, result) - } + ParameterIndex getParameterSizeIndex(ParameterIndex i) { hasArrayWithVariableSize(i, result) } } From b444383ed19c6fcea253d4df9abb9fa72a1630cb Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 19 Mar 2020 13:09:37 +0000 Subject: [PATCH 5/9] C++: Add 'override' specifiers where I missed them. --- .../src/semmle/code/cpp/models/implementations/Strcpy.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index 80bc9ef29dbb..aceda0584ad8 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -80,16 +80,16 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid override predicate hasOnlySpecificWriteSideEffects() { any() } - predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { i = 0 and buffer = true and mustWrite = true } - predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { i = 1 and buffer = true } - ParameterIndex getParameterSizeIndex(ParameterIndex i) { hasArrayWithVariableSize(i, result) } + override ParameterIndex getParameterSizeIndex(ParameterIndex i) { hasArrayWithVariableSize(i, result) } } From 88193dd389e7faf1193992aa9bcdb4f8554cc82d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Thu, 19 Mar 2020 13:32:17 +0000 Subject: [PATCH 6/9] C++: .expected change (desirable). --- .../StrncpyFlippedArgs/StrncpyFlippedArgs.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected index 0fe37d1fe056..d97567875188 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/StrncpyFlippedArgs/StrncpyFlippedArgs.expected @@ -12,5 +12,6 @@ | test.cpp:46:2:46:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | | test.cpp:47:2:47:9 | call to strcpy_s | Potentially unsafe call to strcpy_s; second argument should be size of destination. | | test.cpp:60:3:60:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | +| test.cpp:63:3:63:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:68:2:68:8 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | | test.cpp:79:3:79:9 | call to strncpy | Potentially unsafe call to strncpy; third argument should be size of destination. | From 9e117709bcc8efcb76000a93030c22663b8184a4 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 20 Mar 2020 14:59:57 +0000 Subject: [PATCH 7/9] C++: mustwrite = false. --- cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index aceda0584ad8..723e424263f6 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -83,7 +83,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { i = 0 and buffer = true and - mustWrite = true + mustWrite = false } override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { From ccf5e03fc857050f89e4db1d68586dcd40d1a9e2 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 20 Mar 2020 15:01:22 +0000 Subject: [PATCH 8/9] C++: Autoformat. --- cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index 723e424263f6..c7f0898f3582 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -91,5 +91,7 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid buffer = true } - override ParameterIndex getParameterSizeIndex(ParameterIndex i) { hasArrayWithVariableSize(i, result) } + override ParameterIndex getParameterSizeIndex(ParameterIndex i) { + hasArrayWithVariableSize(i, result) + } } From bb2ce6e5d9f8341565195c526912f1ee3f2ff53d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 20 Mar 2020 16:23:15 +0000 Subject: [PATCH 9/9] C++: More missing override tags. --- cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll index 998329a3e47f..d44b28f041da 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll @@ -62,13 +62,13 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid override predicate hasOnlySpecificWriteSideEffects() { any() } - predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { i = 0 and buffer = true and mustWrite = false } - predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { (i = 0 or i = 1) and buffer = true }