From 49076773514dbc4910f81ed9894943d996ff6074 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 27 Apr 2020 11:30:17 +0100 Subject: [PATCH 1/3] C++: Try to improve QLDoc on deconstructSizeExpr. --- .../cpp/models/implementations/Allocation.qll | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll index 8f40cf9f1f8f..5f1513cb45df 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll @@ -271,12 +271,15 @@ class OperatorNewAllocationFunction extends AllocationFunction { } /** - * The predicate analyzes a `sizeExpr`, which is an argument to an allocation - * function like malloc, and tries to split it into an expression `lengthExpr` - * that describes the length of the allocated array, and the size of the allocated - * element type `sizeof`. - * If this is not possible, the allocation is considered to be of size 1 and of - * length `sizeExpr`. + * Holds if `sizeExpr` is an expression consisting of a subexpression + * `lengthExpr` multiplied by a constant `sizeof` that is the result of a + * `sizeof()` expression. Alternatively if there isn't a suitable `sizeof()` + * expression, `lengthExpr = sizeExpr` and `sizeof = 1`. For example: + * ``` + * malloc(a * 2 * sizeof(char32_t)); + * ``` + * In this case if the `sizeExpr` is the argument to `malloc`, the `lengthExpr` + * is `a * 2` and `sizeof` is `4`. */ private predicate deconstructSizeExpr(Expr sizeExpr, Expr lengthExpr, int sizeof) { if From 200d7ed360572c8a3d6306f50a535ef9fea07cf5 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 27 Apr 2020 11:36:28 +0100 Subject: [PATCH 2/3] C++: Remove if-else. --- .../cpp/models/implementations/Allocation.qll | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll index 5f1513cb45df..782800d0fa24 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll @@ -282,25 +282,21 @@ class OperatorNewAllocationFunction extends AllocationFunction { * is `a * 2` and `sizeof` is `4`. */ private predicate deconstructSizeExpr(Expr sizeExpr, Expr lengthExpr, int sizeof) { - if - sizeExpr instanceof MulExpr and - exists(SizeofOperator sizeofOp, Expr lengthOp | - sizeofOp = sizeExpr.(MulExpr).getAnOperand() and - lengthOp = sizeExpr.(MulExpr).getAnOperand() and - not lengthOp instanceof SizeofOperator and - exists(sizeofOp.getValue().toInt()) - ) - then - exists(SizeofOperator sizeofOp | - sizeofOp = sizeExpr.(MulExpr).getAnOperand() and - lengthExpr = sizeExpr.(MulExpr).getAnOperand() and - not lengthExpr instanceof SizeofOperator and - sizeof = sizeofOp.getValue().toInt() - ) - else ( - lengthExpr = sizeExpr and - sizeof = 1 + exists(SizeofOperator sizeofOp | + sizeofOp = sizeExpr.(MulExpr).getAnOperand() and + lengthExpr = sizeExpr.(MulExpr).getAnOperand() and + not lengthExpr instanceof SizeofOperator and + sizeof = sizeofOp.getValue().toInt() ) + or + not exists(SizeofOperator sizeofOp, Expr lengthOp | + sizeofOp = sizeExpr.(MulExpr).getAnOperand() and + lengthOp = sizeExpr.(MulExpr).getAnOperand() and + not lengthOp instanceof SizeofOperator and + exists(sizeofOp.getValue().toInt()) + ) and + lengthExpr = sizeExpr and + sizeof = 1 } /** From 9b4884dfafa2a3aac8bfe0927d96e1d224024b1d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Fri, 1 May 2020 14:17:50 +0100 Subject: [PATCH 3/3] C++: Backticks. --- cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql | 2 +- cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql | 2 +- cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql index 7cc00c05379f..d49eef4202ad 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScaling.ql @@ -21,7 +21,7 @@ where destBase = baseType(destType) and destBase.getSize() != sourceBase.getSize() and not dest.isInMacroExpansion() and - // If the source type is a char* or void* then don't + // If the source type is a `char*` or `void*` then don't // produce a result, because it is likely to be a false // positive. not sourceBase instanceof CharType and diff --git a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql index f96598edd9ec..d333a2b37bc2 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/IncorrectPointerScalingChar.ql @@ -21,7 +21,7 @@ where destBase = baseType(destType) and destBase.getSize() != sourceBase.getSize() and not dest.isInMacroExpansion() and - // If the source type is a char* or void* then don't + // If the source type is a `char*` or `void*` then don't // produce a result, because it is likely to be a false // positive. not sourceBase instanceof CharType and diff --git a/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql b/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql index 0d5df60f1985..d356ba7bbc43 100644 --- a/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql +++ b/cpp/ql/src/Security/CWE/CWE-468/SuspiciousAddWithSizeof.ql @@ -24,7 +24,7 @@ private predicate isCharSzPtrExpr(Expr e) { from Expr sizeofExpr, Expr e where // If we see an addWithSizeof then we expect the type of - // the pointer expression to be char* or void*. Otherwise it + // the pointer expression to be `char*` or `void*`. Otherwise it // is probably a mistake. addWithSizeof(e, sizeofExpr, _) and not isCharSzPtrExpr(e) select sizeofExpr,