From 102c676f3f7549226ed9bbf82abe4fa5fbaa6614 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 02:47:07 +0100 Subject: [PATCH 01/13] Implement Memory2 queries --- .vscode/tasks.json | 1 + ...atePointersThatDoNotReferToTheSameArray.md | 106 ++++++++ ...atePointersThatDoNotReferToTheSameArray.ql | 22 ++ ...actPointersThatDoNotReferToTheSameArray.md | 106 ++++++++ ...actPointersThatDoNotReferToTheSameArray.ql | 22 ++ .../rules/EXP42-C/DoNotComparePaddingData.md | 135 ++++++++++ .../rules/EXP42-C/DoNotComparePaddingData.ql | 21 ++ .../CloseFilesWhenTheyAreNoLongerNeeded.md | 2 +- .../CloseFilesWhenTheyAreNoLongerNeeded.ql | 159 +---------- .../FreeMemoryWhenNoLongerNeededCert.md | 122 +++++++++ .../FreeMemoryWhenNoLongerNeededCert.ql | 23 ++ ...uctsWithAFlexibleArrayMemberDynamically.md | 249 ++++++++++++++++++ ...uctsWithAFlexibleArrayMemberDynamically.ql | 80 ++++++ ...uctsWithAFlexibleArrayMemberDynamically.md | 249 ++++++++++++++++++ ...uctsWithAFlexibleArrayMemberDynamically.ql | 114 ++++++++ .../OnlyFreeMemoryAllocatedDynamicallyCert.md | 168 ++++++++++++ .../OnlyFreeMemoryAllocatedDynamicallyCert.ql | 23 ++ ...DoNotModifyAlignmentOfMemoryWithRealloc.md | 173 ++++++++++++ ...DoNotModifyAlignmentOfMemoryWithRealloc.ql | 58 ++++ ...intersThatDoNotReferToTheSameArray.testref | 1 + ...intersThatDoNotReferToTheSameArray.testref | 1 + .../EXP42-C/DoNotComparePaddingData.testref | 1 + .../CloseFilesWhenTheyAreNoLongerNeeded.qlref | 1 - ...loseFilesWhenTheyAreNoLongerNeeded.testref | 1 + .../FreeMemoryWhenNoLongerNeededCert.testref | 1 + ...thAFlexibleArrayMemberDynamically.expected | 7 + ...sWithAFlexibleArrayMemberDynamically.qlref | 1 + ...thAFlexibleArrayMemberDynamically.expected | 2 + ...sWithAFlexibleArrayMemberDynamically.qlref | 1 + c/cert/test/rules/MEM33-C/test.c | 54 ++++ ...FreeMemoryAllocatedDynamicallyCert.testref | 1 + ...odifyAlignmentOfMemoryWithRealloc.expected | 18 ++ ...otModifyAlignmentOfMemoryWithRealloc.qlref | 1 + c/cert/test/rules/MEM36-C/test.c | 24 ++ c/common/src/codingstandards/c/Variable.qll | 28 +- ...leHandleWhenNoLongerNeededShared.expected} | 0 ...CloseFileHandleWhenNoLongerNeededShared.ql | 2 + .../test.c | 0 ...reeMemoryWhenNoLongerNeededShared.expected | 4 + .../FreeMemoryWhenNoLongerNeededShared.ql | 2 + .../freememorywhennolongerneededshared/test.c | 82 ++++++ .../MemcmpUsedToComparePaddingData.expected | 1 + .../MemcmpUsedToComparePaddingData.ql | 2 + .../memcmpusedtocomparepaddingdata/test.c | 20 ++ ...eMemoryAllocatedDynamicallyShared.expected | 6 + ...nlyFreeMemoryAllocatedDynamicallyShared.ql | 2 + .../test.c | 47 ++++ .../CloseFileHandleWhenNoLongerNeededMisra.ql | 23 ++ .../FreeMemoryWhenNoLongerNeededMisra.ql | 23 ++ ...OnlyFreeMemoryAllocatedDynamicallyMisra.ql | 23 ++ ...eFileHandleWhenNoLongerNeededMisra.testref | 1 + .../FreeMemoryWhenNoLongerNeededMisra.testref | 1 + ...reeMemoryAllocatedDynamicallyMisra.testref | 1 + .../2023-03-14-fio42-c-fix-logic-error.md | 3 + .../MemcmpUsedToAccessObjectRepresentation.md | 150 +---------- .../MemcmpUsedToAccessObjectRepresentation.ql | 20 +- ...ngDefaultOperatorNewForOverAlignedTypes.ql | 1 + ...pUsedToAccessObjectRepresentation.expected | 1 - ...mcmpUsedToAccessObjectRepresentation.qlref | 1 - ...mpUsedToAccessObjectRepresentation.testref | 1 + ...yUsedToAccessObjectRepresentation.expected | 2 +- ...tUsedToAccessObjectRepresentation.expected | 2 +- cpp/cert/test/rules/EXP62-CPP/test.cpp | 20 -- .../src/codingstandards/cpp/Allocations.qll | 18 ++ .../cpp/exclusions/c/Memory2.qll | 197 ++++++++++++++ .../cpp/exclusions/c/RuleMetadata.qll | 3 + ...loseFileHandleWhenNoLongerNeededShared.qll | 168 ++++++++++++ .../FreeMemoryWhenNoLongerNeededShared.qll | 198 ++++++++++++++ .../MemcmpUsedToComparePaddingData.qll | 27 ++ ...lyFreeMemoryAllocatedDynamicallyShared.qll | 127 +++++++++ .../MemcmpUsedToComparePaddingData.expected | 1 + .../MemcmpUsedToComparePaddingData.ql | 2 + .../memcmpusedtocomparepaddingdata/test.cpp | 30 +++ rule_packages/c/IO1.json | 1 + rule_packages/c/Memory2.json | 214 +++++++++++++++ rule_packages/cpp/Representation.json | 1 + rules.csv | 2 +- 77 files changed, 3055 insertions(+), 351 deletions(-) create mode 100644 c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md create mode 100644 c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql create mode 100644 c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md create mode 100644 c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql create mode 100644 c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md create mode 100644 c/cert/src/rules/EXP42-C/DoNotComparePaddingData.ql create mode 100644 c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md create mode 100644 c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.ql create mode 100644 c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.md create mode 100644 c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql create mode 100644 c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md create mode 100644 c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql create mode 100644 c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md create mode 100644 c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql create mode 100644 c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md create mode 100644 c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql create mode 100644 c/cert/test/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.testref create mode 100644 c/cert/test/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.testref create mode 100644 c/cert/test/rules/EXP42-C/DoNotComparePaddingData.testref delete mode 100644 c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.qlref create mode 100644 c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.testref create mode 100644 c/cert/test/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.testref create mode 100644 c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.expected create mode 100644 c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.qlref create mode 100644 c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.expected create mode 100644 c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.qlref create mode 100644 c/cert/test/rules/MEM33-C/test.c create mode 100644 c/cert/test/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.testref create mode 100644 c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.expected create mode 100644 c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.qlref create mode 100644 c/cert/test/rules/MEM36-C/test.c rename c/{cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.expected => common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.expected} (100%) create mode 100644 c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql rename c/{cert/test/rules/FIO42-C => common/test/rules/closefilehandlewhennolongerneededshared}/test.c (100%) create mode 100644 c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.expected create mode 100644 c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql create mode 100644 c/common/test/rules/freememorywhennolongerneededshared/test.c create mode 100644 c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected create mode 100644 c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql create mode 100644 c/common/test/rules/memcmpusedtocomparepaddingdata/test.c create mode 100644 c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected create mode 100644 c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql create mode 100644 c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c create mode 100644 c/misra/src/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.ql create mode 100644 c/misra/src/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.ql create mode 100644 c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql create mode 100644 c/misra/test/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.testref create mode 100644 c/misra/test/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.testref create mode 100644 c/misra/test/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.testref create mode 100644 change_notes/2023-03-14-fio42-c-fix-logic-error.md delete mode 100644 cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.expected delete mode 100644 cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.qlref create mode 100644 cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.testref create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/Memory2.qll create mode 100644 cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll create mode 100644 cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll create mode 100644 cpp/common/src/codingstandards/cpp/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.qll create mode 100644 cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll create mode 100644 cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected create mode 100644 cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql create mode 100644 cpp/common/test/rules/memcmpusedtocomparepaddingdata/test.cpp create mode 100644 rule_packages/c/Memory2.json diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 2730f99e87..e745a350ed 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -251,6 +251,7 @@ "Pointers1", "Pointers2", "Pointers3", + "Representation", "Scope", "SideEffects1", "SideEffects2", diff --git a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md new file mode 100644 index 0000000000..90d073c18f --- /dev/null +++ b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md @@ -0,0 +1,106 @@ +# ARR36-C: Do not subtract two pointers that do not refer to the same array + +This query implements the CERT-C rule ARR36-C: + +> Do not subtract or compare two pointers that do not refer to the same array + + + +## Description + +When two pointers are subtracted, both must point to elements of the same array object or just one past the last element of the array object (C Standard, 6.5.6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]); the result is the difference of the subscripts of the two array elements. Otherwise, the operation is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48).) + +Similarly, comparing pointers using the relational operators `<`, `<=`, `>=`, and `>` gives the positions of the pointers relative to each other. Subtracting or comparing pointers that do not refer to the same array is undefined behavior. (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48) and [undefined behavior 53](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_53).) + +Comparing pointers using the equality operators `==` and `!=` has well-defined semantics regardless of whether or not either of the pointers is null, points into the same object, or points one past the last element of an array object or function. + +## Noncompliant Code Example + +In this noncompliant code example, pointer subtraction is used to determine how many free elements are left in the `nums` array: + +```cpp +#include + +enum { SIZE = 32 }; + +void func(void) { + int nums[SIZE]; + int end; + int *next_num_ptr = nums; + size_t free_elements; + + /* Increment next_num_ptr as array fills */ + + free_elements = &end - next_num_ptr; +} +``` +This program incorrectly assumes that the `nums` array is adjacent to the `end` variable in memory. A compiler is permitted to insert padding bits between these two variables or even reorder them in memory. + +## Compliant Solution + +In this compliant solution, the number of free elements is computed by subtracting `next_num_ptr` from the address of the pointer past the `nums` array. While this pointer may not be dereferenced, it may be used in pointer arithmetic. + +```cpp +#include +enum { SIZE = 32 }; + +void func(void) { + int nums[SIZE]; + int *next_num_ptr = nums; + size_t free_elements; + + /* Increment next_num_ptr as array fills */ + + free_elements = &(nums[SIZE]) - next_num_ptr; +} +``` + +## Exceptions + +**ARR36-C-EX1:**Comparing two pointers to distinct members of the same `struct` object is allowed. Pointers to structure members declared later in the structure compare greater-than pointers to members declared earlier in the structure. + +## Risk Assessment + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR36-C Medium Probable Medium P8 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 pointer-subtraction Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-ARR36 Can detect operations on pointers that are unrelated
CodeSonar 7.2p0 LANG.STRUCT.CUP LANG.STRUCT.SUP Comparison of Unrelated Pointers Subtraction of Unrelated Pointers
Coverity 2017.07 MISRA C 2004 17.2 MISRA C 2004 17.3 MISRA C 2012 18.2 MISRA C 2012 18.3 Implemented
Helix QAC 2022.4 C0487, C0513 DF2668, DF2669, DF2761, DF2762, DF2763, DF2766, DF2767, DF2768, DF2771, DF2772, DF2773
Klocwork 2022.4 MISRA.PTR.ARITH
LDRA tool suite 9.7.1 437 S, 438 S Fully implemented
Parasoft C/C++test 2022.2 CERT_C-ARR36-aCERT_C-ARR36-b Do not subtract two pointers that do not address elements of the same array Do not compare two unrelated pointers
Polyspace Bug Finder R2023a CERT C: Rule ARR36-C Checks for subtraction or comparison between pointers to different arrays (rule partially covered)
PRQA QA-C 9.7 0487, 0513, 2668, 2669, 2761, 2762, 2763, 2766, 2767, 2768, 2771, 2772, 2773 Fully implemented
PVS-Studio 7.23 V736 , V782
RuleChecker 22.04 pointer-subtraction Partially checked
TrustInSoft Analyzer 1.38 differing_blocks Exhaustively verified (see the compliant and the non-compliant example ).
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+ARR36-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C CTR54-CPP. Do not subtract iterators that do not refer to the same container Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961 Subtracting or comparing two pointers that do not refer to the same array \[ptrobj\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-469 , Use of Pointer Subtraction to Determine Size 2017-07-10: CERT: Exact
CWE 3.11 CWE-469 , Use of Pointer Subtraction to Determine Size 2018-10-18:CERT: CWE subset of rule
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-469 and ARR36-C** + +CWE-469 = Subset(ARR36-C) + +ARR36-C = Union(CWE-469, list) where list = + +* Pointer comparisons using the relational operators `<`, `<=`, `>=`, and `>`, where the pointers do not refer to the same array + +## Bibliography + +
\[ Banahan 2003 \] Section 5.3, "Pointers" Section 5.7, "Expressions Involving Pointers"
\[ ISO/IEC 9899:2011 \] 6.5.6, "Additive Operators"
+ + +## Implementation notes + +None + +## References + +* CERT-C: [ARR36-C: Do not subtract or compare two pointers that do not refer to the same array](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql new file mode 100644 index 0000000000..5b346c02dd --- /dev/null +++ b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql @@ -0,0 +1,22 @@ +/** + * @id c/cert/do-not-relate-pointers-that-do-not-refer-to-the-same-array + * @name ARR36-C: Do not subtract two pointers that do not refer to the same array + * @description Comparison using the >, >=, <, and <= operators between pointers referring to + * differing arrays results in undefined behavior. + * @kind problem + * @precision high + * @problem.severity warning + * @tags external/cert/id/arr36-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.donotuserelationaloperatorswithdifferingarrays.DoNotUseRelationalOperatorsWithDifferingArrays + +class DoNotRelatePointersThatDoNotReferToTheSameArrayQuery extends DoNotUseRelationalOperatorsWithDifferingArraysSharedQuery { + DoNotRelatePointersThatDoNotReferToTheSameArrayQuery() { + this = Memory2Package::doNotRelatePointersThatDoNotReferToTheSameArrayQuery() + } +} diff --git a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md new file mode 100644 index 0000000000..90d073c18f --- /dev/null +++ b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md @@ -0,0 +1,106 @@ +# ARR36-C: Do not subtract two pointers that do not refer to the same array + +This query implements the CERT-C rule ARR36-C: + +> Do not subtract or compare two pointers that do not refer to the same array + + + +## Description + +When two pointers are subtracted, both must point to elements of the same array object or just one past the last element of the array object (C Standard, 6.5.6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]); the result is the difference of the subscripts of the two array elements. Otherwise, the operation is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48).) + +Similarly, comparing pointers using the relational operators `<`, `<=`, `>=`, and `>` gives the positions of the pointers relative to each other. Subtracting or comparing pointers that do not refer to the same array is undefined behavior. (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48) and [undefined behavior 53](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_53).) + +Comparing pointers using the equality operators `==` and `!=` has well-defined semantics regardless of whether or not either of the pointers is null, points into the same object, or points one past the last element of an array object or function. + +## Noncompliant Code Example + +In this noncompliant code example, pointer subtraction is used to determine how many free elements are left in the `nums` array: + +```cpp +#include + +enum { SIZE = 32 }; + +void func(void) { + int nums[SIZE]; + int end; + int *next_num_ptr = nums; + size_t free_elements; + + /* Increment next_num_ptr as array fills */ + + free_elements = &end - next_num_ptr; +} +``` +This program incorrectly assumes that the `nums` array is adjacent to the `end` variable in memory. A compiler is permitted to insert padding bits between these two variables or even reorder them in memory. + +## Compliant Solution + +In this compliant solution, the number of free elements is computed by subtracting `next_num_ptr` from the address of the pointer past the `nums` array. While this pointer may not be dereferenced, it may be used in pointer arithmetic. + +```cpp +#include +enum { SIZE = 32 }; + +void func(void) { + int nums[SIZE]; + int *next_num_ptr = nums; + size_t free_elements; + + /* Increment next_num_ptr as array fills */ + + free_elements = &(nums[SIZE]) - next_num_ptr; +} +``` + +## Exceptions + +**ARR36-C-EX1:**Comparing two pointers to distinct members of the same `struct` object is allowed. Pointers to structure members declared later in the structure compare greater-than pointers to members declared earlier in the structure. + +## Risk Assessment + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR36-C Medium Probable Medium P8 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 pointer-subtraction Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-ARR36 Can detect operations on pointers that are unrelated
CodeSonar 7.2p0 LANG.STRUCT.CUP LANG.STRUCT.SUP Comparison of Unrelated Pointers Subtraction of Unrelated Pointers
Coverity 2017.07 MISRA C 2004 17.2 MISRA C 2004 17.3 MISRA C 2012 18.2 MISRA C 2012 18.3 Implemented
Helix QAC 2022.4 C0487, C0513 DF2668, DF2669, DF2761, DF2762, DF2763, DF2766, DF2767, DF2768, DF2771, DF2772, DF2773
Klocwork 2022.4 MISRA.PTR.ARITH
LDRA tool suite 9.7.1 437 S, 438 S Fully implemented
Parasoft C/C++test 2022.2 CERT_C-ARR36-aCERT_C-ARR36-b Do not subtract two pointers that do not address elements of the same array Do not compare two unrelated pointers
Polyspace Bug Finder R2023a CERT C: Rule ARR36-C Checks for subtraction or comparison between pointers to different arrays (rule partially covered)
PRQA QA-C 9.7 0487, 0513, 2668, 2669, 2761, 2762, 2763, 2766, 2767, 2768, 2771, 2772, 2773 Fully implemented
PVS-Studio 7.23 V736 , V782
RuleChecker 22.04 pointer-subtraction Partially checked
TrustInSoft Analyzer 1.38 differing_blocks Exhaustively verified (see the compliant and the non-compliant example ).
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+ARR36-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C CTR54-CPP. Do not subtract iterators that do not refer to the same container Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961 Subtracting or comparing two pointers that do not refer to the same array \[ptrobj\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-469 , Use of Pointer Subtraction to Determine Size 2017-07-10: CERT: Exact
CWE 3.11 CWE-469 , Use of Pointer Subtraction to Determine Size 2018-10-18:CERT: CWE subset of rule
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-469 and ARR36-C** + +CWE-469 = Subset(ARR36-C) + +ARR36-C = Union(CWE-469, list) where list = + +* Pointer comparisons using the relational operators `<`, `<=`, `>=`, and `>`, where the pointers do not refer to the same array + +## Bibliography + +
\[ Banahan 2003 \] Section 5.3, "Pointers" Section 5.7, "Expressions Involving Pointers"
\[ ISO/IEC 9899:2011 \] 6.5.6, "Additive Operators"
+ + +## Implementation notes + +None + +## References + +* CERT-C: [ARR36-C: Do not subtract or compare two pointers that do not refer to the same array](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql new file mode 100644 index 0000000000..15e1148b53 --- /dev/null +++ b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql @@ -0,0 +1,22 @@ +/** + * @id c/cert/do-not-subtract-pointers-that-do-not-refer-to-the-same-array + * @name ARR36-C: Do not subtract two pointers that do not refer to the same array + * @description Subtraction between pointers referring to differing arrays results in undefined + * behavior. + * @kind problem + * @precision high + * @problem.severity warning + * @tags external/cert/id/arr36-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.donotsubtractpointersaddressingdifferentarrays.DoNotSubtractPointersAddressingDifferentArrays + +class DoNotSubtractPointersThatDoNotReferToTheSameArrayQuery extends DoNotSubtractPointersAddressingDifferentArraysSharedQuery { + DoNotSubtractPointersThatDoNotReferToTheSameArrayQuery() { + this = Memory2Package::doNotSubtractPointersThatDoNotReferToTheSameArrayQuery() + } +} diff --git a/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md new file mode 100644 index 0000000000..0bfa1e25fc --- /dev/null +++ b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md @@ -0,0 +1,135 @@ +# EXP42-C: Do not compare padding data + +This query implements the CERT-C rule EXP42-C: + +> Do not compare padding data + + + +## Description + +The C Standard, 6.7.2.1 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states + +> There may be unnamed padding within a structure object, but not at its beginning. . . . There may be unnamed padding at the end of a structure or union. + + +Subclause 6.7.9, paragraph 9, states that + +> unnamed members of objects of structure and union type do not participate in initialization. Unnamed members of structure objects have indeterminate value even after initialization. + + +The only exception is that padding bits are set to zero when a static or thread-local object is implicitly initialized (paragraph10): + +> If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static or thread storage duration is not initialized explicitly, then: + + +— if it is an aggregate, every member is initialized (recursively) according to these rules, and any padding is initialized to zero bits; + +— if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits; + +Because these padding values are unspecified, attempting a byte-by-byte comparison between structures can lead to incorrect results \[[Summit 1995](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Summit95)\]. + +## Noncompliant Code Example + +In this noncompliant code example, `memcmp()` is used to compare the contents of two structures, including any padding bytes: + +```cpp +#include + +struct s { + char c; + int i; + char buffer[13]; +}; + +void compare(const struct s *left, const struct s *right) { + if ((left && right) && + (0 == memcmp(left, right, sizeof(struct s)))) { + /* ... */ + } +} +``` + +## Compliant Solution + +In this compliant solution, all of the fields are compared manually to avoid comparing any padding bytes: + +```cpp +#include + +struct s { + char c; + int i; + char buffer[13]; +}; + +void compare(const struct s *left, const struct s *right) { + if ((left && right) && + (left->c == right->c) && + (left->i == right->i) && + (0 == memcmp(left->buffer, right->buffer, 13))) { + /* ... */ + } +} +``` + +## Exceptions + +**EXP42-C-EX1**: A structure can be defined such that the members are aligned properly or the structure is packed using implementation-specific packing instructions. This is true only when the members' data types have no padding bits of their own and when their object representations are the same as their value representations. This frequently is not true for the `_Bool` type or floating-point types and need not be true for pointers. In such cases, the compiler does not insert padding, and use of functions such as `memcmp()` is acceptable. + +This compliant example uses the [\#pragma pack](http://msdn.microsoft.com/en-us/library/2e70t5y1.aspx) compiler extension from Microsoft Visual Studio to ensure the structure members are packed as tightly as possible: + +```cpp +#include + +#pragma pack(push, 1) +struct s { + char c; + int i; + char buffer[13]; +}; +#pragma pack(pop) + +void compare(const struct s *left, const struct s *right) { + if ((left && right) && + (0 == memcmp(left, right, sizeof(struct s)))) { + /* ... */ + } +} +``` + +## Risk Assessment + +Comparing padding bytes, when present, can lead to [unexpected program behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior). + +
Rule Severity Likelihood Remediation Cost Priority Level
EXP42-C Medium Probable Medium P8 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 memcpy-with-padding Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-EXP42
CodeSonar 7.2p0 BADFUNC.MEMCMP Use of memcmp
Helix QAC 2022.4 DF4726, DF4727, DF4728, DF4729
Klocwork 2022.4 MISRA.STDLIB.MEMCMP.PTR_ARG_TYPES
LDRA tool suite 9.7.1 618 S Partially implemented
Cppcheck 1.66 cert.py Detected by the addon cert.py Does not warn about global/static padding data as this is probably initialized to 0
Parasoft C/C++test 2022.2 CERT_C-EXP42-a Don't memcpy or memcmp non-PODs
PC-lint Plus 1.4 958, 959 Assistance provided: reports structures which require padding between members or after the last member
Polyspace Bug Finder R2023a CERT C: Rule EXP42-C Checks for memory comparison of padding data (rule fully covered)
PRQA QA-C 9.7 1488
RuleChecker 22.04 memcpy-with-padding Partially checked
TrustInSoft Analyzer 1.38 comparable_char_blocks Exhaustively verified (see the compliant and the non-compliant example ).
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+EXP42-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
ISO/IEC TS 17961 Comparison of padding data \[padcomp\] Prior to 2018-01-12: CERT: Unspecified Relationship
CERT C EXP62-CPP. Do not access the bits of an object representation that are not part of the object's value representation Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 6.7.2.1, "Structure and Union Specifiers" 6.7.9, "Initialization"
\[ Summit 1995 \] Question 2.8 Question 2.12
+ + +## Implementation notes + +None + +## References + +* CERT-C: [EXP42-C: Do not compare padding data](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.ql b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.ql new file mode 100644 index 0000000000..d2403553aa --- /dev/null +++ b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.ql @@ -0,0 +1,21 @@ +/** + * @id c/cert/do-not-compare-padding-data + * @name EXP42-C: Do not compare padding data + * @description Padding data values are unspecified and should not be included in comparisons. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/exp42-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.memcmpusedtocomparepaddingdata.MemcmpUsedToComparePaddingData + +class DoNotComparePaddingDataQuery extends MemcmpUsedToComparePaddingDataSharedQuery { + DoNotComparePaddingDataQuery() { + this = Memory2Package::doNotComparePaddingDataQuery() + } +} diff --git a/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.md b/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.md index 91654e8ee2..f84163ae4a 100644 --- a/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.md +++ b/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.md @@ -180,7 +180,7 @@ Failing to properly close files may allow an attacker to exhaust system resource This rule is stricter than rule \[fileclose\] in [ISO/IEC TS 17961:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IECTS17961). Analyzers that conform to the technical standard may not detect all violations of this rule. -
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
CodeSonar 7.0p0 ALLOC.LEAK Leak
Compass/ROSE
Coverity 2017.07 RESOURCE_LEAK (partial) Partially implemented
Helix QAC 2022.2 C2701, C2702, C2703 C++2701, C++2702, C++2703
Klocwork 2022.2 RH.LEAK
LDRA tool suite 9.7.1 49 D Partially implemented
Parasoft C/C++test 2022.1 CERT_C-FIO42-a Ensure resources are freed
PC-lint Plus 1.4 429 Partially supported
Polyspace Bug Finder R2022a CERT C: Rule FIO42-C Checks for resource leak (rule partially covered)
PRQA QA-C 9.7 2701, 2702, 2703
PRQA QA-C++ 4.4 2701, 2702, 2703
SonarQube C/C++ Plugin 3.11 S2095
+
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
CodeSonar 7.2p0 ALLOC.LEAK Leak
Compass/ROSE
Coverity 2017.07 RESOURCE_LEAK (partial) Partially implemented
Helix QAC 2022.4 DF2701, DF2702, DF2703
Klocwork 2022.4 RH.LEAK
LDRA tool suite 9.7.1 49 D Partially implemented
Parasoft C/C++test 2022.2 CERT_C-FIO42-a Ensure resources are freed
PC-lint Plus 1.4 429 Partially supported
Polyspace Bug Finder R2023a CERT C: Rule FIO42-C Checks for resource leak (rule partially covered)
PRQA QA-C 9.7 2701, 2702, 2703
PRQA QA-C++ 4.4 2701, 2702, 2703
SonarQube C/C++ Plugin 3.11 S2095
## Related Vulnerabilities diff --git a/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.ql b/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.ql index 2c7959f38e..b7cfd40da5 100644 --- a/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.ql +++ b/c/cert/src/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.ql @@ -14,161 +14,10 @@ import cpp import codingstandards.c.cert -import semmle.code.cpp.controlflow.StackVariableReachability -import codingstandards.cpp.standardlibrary.FileAccess +import codingstandards.cpp.rules.closefilehandlewhennolongerneededshared.CloseFileHandleWhenNoLongerNeededShared -/** - * Extend the NullValue class used by Nullness.qll to include simple -1 as a 'null' value - * (for example 'open' returns -1 if there was an error) - */ -class MinusOne extends NullValue { - MinusOne() { this.(UnaryMinusExpr).getOperand().(Literal).getValue() = "1" } -} - -/** - * 'call' is either a direct call to f, or a possible call to f - * via a function pointer. - */ -predicate mayCallFunction(Expr call, Function f) { - call.(FunctionCall).getTarget() = f or - call.(VariableCall).getVariable().getAnAssignedValue().getAChild*().(FunctionAccess).getTarget() = - f -} - -predicate fopenCallOrIndirect(Expr e) { - // direct fopen call - opened(e) and - // We are only interested in fopen calls that are - // actually closed somehow, as FileNeverClosed - // will catch those that aren't. - fopenCallMayBeClosed(e) - or - exists(ReturnStmt rtn | - // indirect fopen call - mayCallFunction(e, rtn.getEnclosingFunction()) and - ( - // return fopen - fopenCallOrIndirect(rtn.getExpr()) - or - // return variable assigned with fopen - exists(Variable v | - v = rtn.getExpr().(VariableAccess).getTarget() and - fopenCallOrIndirect(v.getAnAssignedValue()) and - not assignedToFieldOrGlobal(v, _) - ) - ) - ) -} - -predicate fcloseCallOrIndirect(FunctionCall fc, Variable v) { - // direct fclose call - fcloseCall(fc, v.getAnAccess()) - or - // indirect fclose call - exists(FunctionCall midcall, Function mid, int arg | - fc.getArgument(arg) = v.getAnAccess() and - mayCallFunction(fc, mid) and - midcall.getEnclosingFunction() = mid and - fcloseCallOrIndirect(midcall, mid.getParameter(arg)) - ) -} - -predicate fopenDefinition(StackVariable v, ControlFlowNode def) { - exists(Expr expr | exprDefinition(v, def, expr) and fopenCallOrIndirect(expr)) -} - -class FOpenVariableReachability extends StackVariableReachabilityWithReassignment { - FOpenVariableReachability() { this = "FOpenVariableReachability" } - - override predicate isSourceActual(ControlFlowNode node, StackVariable v) { - fopenDefinition(v, node) - } - - override predicate isSinkActual(ControlFlowNode node, StackVariable v) { - // node may be used in fopenReaches - exists(node.(AnalysedExpr).getNullSuccessor(v)) or - fcloseCallOrIndirect(node, v) or - assignedToFieldOrGlobal(v, node) or - // node may be used directly in query - v.getFunction() = node.(ReturnStmt).getEnclosingFunction() - } - - override predicate isBarrier(ControlFlowNode node, StackVariable v) { definitionBarrier(v, node) } -} - -/** - * The value from fopen at `def` is still held in Variable `v` upon entering `node`. - */ -predicate fopenVariableReaches(StackVariable v, ControlFlowNode def, ControlFlowNode node) { - exists(FOpenVariableReachability r | - // reachability - r.reachesTo(def, _, node, v) - or - // accept def node itself - r.isSource(def, v) and - node = def - ) -} - -class FOpenReachability extends StackVariableReachabilityExt { - FOpenReachability() { this = "FOpenReachability" } - - override predicate isSource(ControlFlowNode node, StackVariable v) { fopenDefinition(v, node) } - - override predicate isSink(ControlFlowNode node, StackVariable v) { - v.getFunction() = node.(ReturnStmt).getEnclosingFunction() - } - - override predicate isBarrier( - ControlFlowNode source, ControlFlowNode node, ControlFlowNode next, StackVariable v - ) { - isSource(source, v) and - next = node.getASuccessor() and - // the file (stored in any variable `v0`) opened at `source` is closed or - // assigned to a global at node, or NULL checked on the edge node -> next. - exists(StackVariable v0 | fopenVariableReaches(v0, source, node) | - node.(AnalysedExpr).getNullSuccessor(v0) = next or - fcloseCallOrIndirect(node, v0) or - assignedToFieldOrGlobal(v0, node) - ) +class CloseFilesWhenTheyAreNoLongerNeededQuery extends CloseFileHandleWhenNoLongerNeededSharedSharedQuery { + CloseFilesWhenTheyAreNoLongerNeededQuery() { + this = IO1Package::closeFilesWhenTheyAreNoLongerNeededQuery() } } - -/** - * The value returned by fopen `def` has not been closed, confirmed to be null, - * or potentially leaked globally upon reaching `node` (regardless of what variable - * it's still held in, if any). - */ -predicate fopenReaches(ControlFlowNode def, ControlFlowNode node) { - exists(FOpenReachability r | r.reaches(def, _, node)) -} - -predicate assignedToFieldOrGlobal(StackVariable v, Expr e) { - // assigned to anything except a StackVariable - // (typically a field or global, but for example also *ptr = v) - e.(Assignment).getRValue() = v.getAnAccess() and - not e.(Assignment).getLValue().(VariableAccess).getTarget() instanceof StackVariable - or - exists(Expr midExpr, Function mid, int arg | - // indirect assignment - e.(FunctionCall).getArgument(arg) = v.getAnAccess() and - mayCallFunction(e, mid) and - midExpr.getEnclosingFunction() = mid and - assignedToFieldOrGlobal(mid.getParameter(arg), midExpr) - ) - or - // assigned to a field via constructor field initializer - e.(ConstructorFieldInit).getExpr() = v.getAnAccess() -} - -from ControlFlowNode def, Stmt ret -where - not isExcluded(def, IO1Package::closeFilesWhenTheyAreNoLongerNeededQuery()) and - fopenReaches(def, ret) and - not exists(StackVariable v | - fopenVariableReaches(v, def, ret) and - ret.getAChild*() = v.getAnAccess() - ) - or - opened(def) and not fopenCallMayBeClosed(def) and ret = def.getControlFlowScope().getEntryPoint() -select def, "The file opened here may not be closed at $@.", ret, "this location" diff --git a/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md new file mode 100644 index 0000000000..09a6ce7219 --- /dev/null +++ b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md @@ -0,0 +1,122 @@ +# MEM31-C: Free dynamically allocated memory when no longer needed + +This query implements the CERT-C rule MEM31-C: + +> Free dynamically allocated memory when no longer needed + + + +## Description + +Before the lifetime of the last pointer that stores the return value of a call to a standard memory allocation function has ended, it must be matched by a call to `free()` with that pointer value. + +## Noncompliant Code Example + +In this noncompliant example, the object allocated by the call to `malloc()` is not freed before the end of the lifetime of the last pointer `text_buffer` referring to the object: + +```cpp +#include + +enum { BUFFER_SIZE = 32 }; + +int f(void) { + char *text_buffer = (char *)malloc(BUFFER_SIZE); + if (text_buffer == NULL) { + return -1; + } + return 0; +} +``` + +## Compliant Solution + +In this compliant solution, the pointer is deallocated with a call to `free()`: + +```cpp +#include + +enum { BUFFER_SIZE = 32 }; + +int f(void) { + char *text_buffer = (char *)malloc(BUFFER_SIZE); + if (text_buffer == NULL) { + return -1; + } + + free(text_buffer); + return 0; +} + +``` + +## Exceptions + +**MEM31-C-EX1**: Allocated memory does not need to be freed if it is assigned to a pointer whose lifetime includes program termination. The following code example illustrates a pointer that stores the return value from `malloc()` in a `static` variable: + +```cpp +#include + +enum { BUFFER_SIZE = 32 }; + +int f(void) { + static char *text_buffer = NULL; + if (text_buffer == NULL) { + text_buffer = (char *)malloc(BUFFER_SIZE); + if (text_buffer == NULL) { + return -1; + } + } + return 0; +} + +``` + +## Risk Assessment + +Failing to free memory can result in the exhaustion of system memory resources, which can lead to a [denial-of-service attack](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-denial-of-service). + +
Rule Severity Likelihood Remediation Cost Priority Level
MEM31-C Medium Probable Medium P8 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
Axivion Bauhaus Suite 7.2.0 CertC-MEM31 Can detect dynamically allocated resources that are not freed
CodeSonar 7.2p0 ALLOC.LEAK Leak
Compass/ROSE
Coverity 2017.07 RESOURCE_LEAK ALLOC_FREE_MISMATCH Finds resource leaks from variables that go out of scope while owning a resource
Cppcheck 1.66 leakReturnValNotUsed Doesn't use return value of memory allocation function
Helix QAC 2022.4 DF2706, DF2707, DF2708 C++3337, C++3338
Klocwork 2022.4 CL.FFM.ASSIGN CL.FFM.COPY CL.SHALLOW.ASSIGN CL.SHALLOW.COPY FMM.MIGHT FMM.MUST
LDRA tool suite 9.7.1 50 D Partially implemented
Parasoft C/C++test 2022.2 CERT_C-MEM31-a Ensure resources are freed
Parasoft Insure++ Runtime analysis
PC-lint Plus 1.4 429 Fully supported
Polyspace Bug Finder R2023a CERT C: Rule MEM31-C Checks for memory leak (rule fully covered)
PRQA QA-C 9.7 2706, 2707, 2708
PRQA QA-C++ 4.4 2706, 2707, 2708, 3337, 3338
PVS-Studio 7.23 V773
SonarQube C/C++ Plugin 3.11 S3584
Splint 3.1.1
TrustInSoft Analyzer 1.38 malloc Exhaustively verified.
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MEM31-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
ISO/IEC TR 24772:2013 Memory Leak \[XYL\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961 Failing to close files or free dynamic memory when they are no longer needed \[fileclose\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-401 , Improper Release of Memory Before Removing Last Reference ("Memory Leak") 2017-07-05: CERT: Exact
CWE 2.11 CWE-404 2017-07-06: CERT: Rule subset of CWE
CWE 2.11 CWE-459 2017-07-06: CERT: Rule subset of CWE
CWE 2.11 CWE-771 2017-07-06: CERT: Rule subset of CWE
CWE 2.11 CWE-772 2017-07-06: CERT: Rule subset of CWE
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-404/CWE-459/CWE-771/CWE-772 and FIO42-C/MEM31-C** + +Intersection( FIO42-C, MEM31-C) = Ø + +CWE-404 = CWE-459 = CWE-771 = CWE-772 + +CWE-404 = Union( FIO42-C, MEM31-C list) where list = + +* Failure to free resources besides files or memory chunks, such as mutexes) + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] Subclause 7.22.3, "Memory Management Functions"
+ + +## Implementation notes + +The rule is enforced in the context of a single function. + +## References + +* CERT-C: [MEM31-C: Free dynamically allocated memory when no longer needed](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.ql b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.ql new file mode 100644 index 0000000000..d4c81748a2 --- /dev/null +++ b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.ql @@ -0,0 +1,23 @@ +/** + * @id c/cert/free-memory-when-no-longer-needed-cert + * @name MEM31-C: Free dynamically allocated memory when no longer needed + * @description Failing to free memory that is no longer needed can lead to a memory leak and + * resource exhaustion. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/mem31-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.freememorywhennolongerneededshared.FreeMemoryWhenNoLongerNeededShared + +class FreeMemoryWhenNoLongerNeededCertQuery extends FreeMemoryWhenNoLongerNeededSharedSharedQuery { + FreeMemoryWhenNoLongerNeededCertQuery() { + this = Memory2Package::freeMemoryWhenNoLongerNeededCertQuery() + } +} diff --git a/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.md b/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.md new file mode 100644 index 0000000000..6a726c1701 --- /dev/null +++ b/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.md @@ -0,0 +1,249 @@ +# MEM33-C: Allocate structures containing a flexible array member dynamically + +This query implements the CERT-C rule MEM33-C: + +> Allocate and copy structures containing a flexible array member dynamically + + +## Description + +The C Standard, 6.7.2.1, paragraph 18 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], says + +> As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a *flexible array member*. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply. + + +The following is an example of a structure that contains a flexible array member: + +```cpp +struct flex_array_struct { + int num; + int data[]; +}; + +``` +This definition means that when computing the size of such a structure, only the first member, `num`, is considered. Unless the appropriate size of the flexible array member has been explicitly added when allocating storage for an object of the `struct`, the result of accessing the member `data` of a variable of nonpointer type `struct flex_array_struct` is [undefined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). [DCL38-C. Use the correct syntax when declaring a flexible array member](https://wiki.sei.cmu.edu/confluence/display/c/DCL38-C.+Use+the+correct+syntax+when+declaring+a+flexible+array+member) describes the correct way to declare a `struct` with a flexible array member. + +To avoid the potential for undefined behavior, structures that contain a flexible array member should always be allocated dynamically. Flexible array structures must + +* Have dynamic storage duration (be allocated via `malloc()` or another dynamic allocation function) +* Be dynamically copied using `memcpy()` or a similar function and not by assignment +* When used as an argument to a function, be passed by pointer and not copied by value + +## Noncompliant Code Example (Storage Duration) + +This noncompliant code example uses automatic storage for a structure containing a flexible array member: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(void) { + struct flex_array_struct flex_struct; + size_t array_size = 4; + + /* Initialize structure */ + flex_struct.num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + flex_struct.data[i] = 0; + } +} +``` +Because the memory for `flex_struct` is reserved on the stack, no space is reserved for the `data` member. Accessing the `data` member is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +## Compliant Solution (Storage Duration) + +This compliant solution dynamically allocates storage for `flex_array_struct`: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(void) { + struct flex_array_struct *flex_struct; + size_t array_size = 4; + + /* Dynamically allocate memory for the struct */ + flex_struct = (struct flex_array_struct *)malloc( + sizeof(struct flex_array_struct) + + sizeof(int) * array_size); + if (flex_struct == NULL) { + /* Handle error */ + } + + /* Initialize structure */ + flex_struct->num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + flex_struct->data[i] = 0; + } +} +``` + +## Noncompliant Code Example (Copying) + +This noncompliant code example attempts to copy an instance of a structure containing a flexible array member (`struct `flex_array_struct``) by assignment: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(struct flex_array_struct *struct_a, + struct flex_array_struct *struct_b) { + *struct_b = *struct_a; +} +``` +When the structure is copied, the size of the flexible array member is not considered, and only the first member of the structure, `num`, is copied, leaving the array contents untouched. + +## Compliant Solution (Copying) + +This compliant solution uses `memcpy()` to properly copy the content of `struct_a` into `struct_b`: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(struct flex_array_struct *struct_a, + struct flex_array_struct *struct_b) { + if (struct_a->num > struct_b->num) { + /* Insufficient space; handle error */ + return; + } + memcpy(struct_b, struct_a, + sizeof(struct flex_array_struct) + (sizeof(int) + * struct_a->num)); +} +``` + +## Noncompliant Code Example (Function Arguments) + +In this noncompliant code example, the flexible array structure is passed by value to a function that prints the array elements: + +```cpp +#include +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void print_array(struct flex_array_struct struct_p) { + puts("Array is: "); + for (size_t i = 0; i < struct_p.num; ++i) { + printf("%d ", struct_p.data[i]); + } + putchar('\n'); +} + +void func(void) { + struct flex_array_struct *struct_p; + size_t array_size = 4; + + /* Space is allocated for the struct */ + struct_p = (struct flex_array_struct *)malloc( + sizeof(struct flex_array_struct) + + sizeof(int) * array_size); + if (struct_p == NULL) { + /* Handle error */ + } + struct_p->num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + struct_p->data[i] = i; + } + print_array(*struct_p); +} +``` +Because the argument is passed by value, the size of the flexible array member is not considered when the structure is copied, and only the first member of the structure, `num`, is copied. + +## Compliant Solution (Function Arguments) + +In this compliant solution, the structure is passed by reference and not by value: + +```cpp +#include +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void print_array(struct flex_array_struct *struct_p) { + puts("Array is: "); + for (size_t i = 0; i < struct_p->num; ++i) { + printf("%d ", struct_p->data[i]); + } + putchar('\n'); +} + +void func(void) { + struct flex_array_struct *struct_p; + size_t array_size = 4; + + /* Space is allocated for the struct and initialized... */ + + print_array(struct_p); +} +``` + +## Risk Assessment + +Failure to use structures with flexible array members correctly can result in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +
Rule Severity Likelihood Remediation Cost Priority Level
MEM33-C Low Unlikely Low P3 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 flexible-array-member-assignment flexible-array-member-declaration Fully checked
Axivion Bauhaus Suite 7.2.0 CertC-MEM33 Fully implemented
CodeSonar 7.2p0 LANG.STRUCT.DECL.FAM Declaration of Flexible Array Member
Compass/ROSE Can detect all of these
Helix QAC 2022.4 C1061, C1062, C1063, C1064
Klocwork 2022.4 MISRA.INCOMPLETE.STRUCT MISRA.MEMB.FLEX_ARRAY.2012
LDRA tool suite 9.7.1 649 S, 650 S Fully implemented
Parasoft C/C++test 2022.2 CERT_C-MEM33-a CERT_C-MEM33-b Allocate structures containing a flexible array member dynamically Do not copy instances of structures containing a flexible array member
Polyspace Bug Finder R2023a CERT C: Rule MEM33-C Checks for misuse of structure with flexible array member (rule fully covered)
PRQA QA-C 9.7 1061, 1062, 1063, 1064
RuleChecker 22.04 flexible-array-member-assignment flexible-array-member-declaration Fully checked
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MEM33-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C Secure Coding Standard DCL38-C. Use the correct syntax when declaring a flexible array member Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-401 and MEM33-CPP** + +There is no longer a C++ rule for MEM33-CPP. (In fact, all C++ rules from 30-50 are gone, because we changed the numbering system to be 50-99 for C++ rules.) + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] Subclause 6.7.2.1, "Structure and Union Specifiers"
\[ JTC1/SC22/WG14 N791 \] Solving the Struct Hack Problem
+ + +## Implementation notes + +None + +## References + +* CERT-C: [MEM33-C: Allocate and copy structures containing a flexible array member dynamically](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql b/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql new file mode 100644 index 0000000000..620c4486a9 --- /dev/null +++ b/c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql @@ -0,0 +1,80 @@ +/** + * @id c/cert/alloc-structs-with-a-flexible-array-member-dynamically + * @name MEM33-C: Allocate structures containing a flexible array member dynamically + * @description A structure containing a flexible array member must be allocated dynamically in + * order for subsequent accesses to the flexible array to point to valid memory. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/mem33-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.c.Variable +import semmle.code.cpp.models.interfaces.Allocation +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis + +abstract class FlexibleArrayAlloc extends Element { + /** + * Returns the `Variable` being allocated. + */ + abstract Variable getVariable(); +} + +/** + * A `FunctionCall` to an `AllocationFunction` that allocates memory + * which is assigned to a `Variable` of type `FlexibleArrayStructType`. + */ +class FlexibleArrayStructDynamicAlloc extends FlexibleArrayAlloc, FunctionCall { + Variable v; + + FlexibleArrayStructDynamicAlloc() { + this.getTarget() instanceof AllocationFunction and + v.getAnAssignedValue() = this and + v.getUnderlyingType().(PointerType).getBaseType().getUnspecifiedType() instanceof + FlexibleArrayStructType + } + + /** + * Holds if the size argument of the allocation function is insufficient to + * allocate at least one byte for the flexible array member. + */ + predicate hasInsufficientAllocationSize() { + upperBound(this.getArgument(this.getTarget().(AllocationFunction).getSizeArg())) <= + max(v.getUnderlyingType() + .(PointerType) + .getBaseType() + .getUnspecifiedType() + .(FlexibleArrayStructType) + .getSize() + ) + } + + override Variable getVariable() { result = v } +} + +/** + * A `Variable` of type `FlexibleArrayStructType` that is not allocated dynamically. + */ +class FlexibleArrayNonDynamicAlloc extends FlexibleArrayAlloc, Variable { + FlexibleArrayNonDynamicAlloc() { + this.getUnspecifiedType().getUnspecifiedType() instanceof FlexibleArrayStructType + } + + override Variable getVariable() { result = this } +} + +from FlexibleArrayAlloc alloc, string message +where + not isExcluded(alloc, Memory2Package::allocStructsWithAFlexibleArrayMemberDynamicallyQuery()) and + ( + alloc.(FlexibleArrayStructDynamicAlloc).hasInsufficientAllocationSize() and + message = "$@ allocated with insufficient memory for its flexible array member." + or + alloc instanceof FlexibleArrayNonDynamicAlloc and + message = "$@ contains a flexible array member but is not dynamically allocated." + ) +select alloc, message, alloc.getVariable(), alloc.getVariable().getName() diff --git a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md new file mode 100644 index 0000000000..34d1aa6287 --- /dev/null +++ b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md @@ -0,0 +1,249 @@ +# MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function. + +This query implements the CERT-C rule MEM33-C: + +> Allocate and copy structures containing a flexible array member dynamically + + +## Description + +The C Standard, 6.7.2.1, paragraph 18 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], says + +> As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a *flexible array member*. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply. + + +The following is an example of a structure that contains a flexible array member: + +```cpp +struct flex_array_struct { + int num; + int data[]; +}; + +``` +This definition means that when computing the size of such a structure, only the first member, `num`, is considered. Unless the appropriate size of the flexible array member has been explicitly added when allocating storage for an object of the `struct`, the result of accessing the member `data` of a variable of nonpointer type `struct flex_array_struct` is [undefined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). [DCL38-C. Use the correct syntax when declaring a flexible array member](https://wiki.sei.cmu.edu/confluence/display/c/DCL38-C.+Use+the+correct+syntax+when+declaring+a+flexible+array+member) describes the correct way to declare a `struct` with a flexible array member. + +To avoid the potential for undefined behavior, structures that contain a flexible array member should always be allocated dynamically. Flexible array structures must + +* Have dynamic storage duration (be allocated via `malloc()` or another dynamic allocation function) +* Be dynamically copied using `memcpy()` or a similar function and not by assignment +* When used as an argument to a function, be passed by pointer and not copied by value + +## Noncompliant Code Example (Storage Duration) + +This noncompliant code example uses automatic storage for a structure containing a flexible array member: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(void) { + struct flex_array_struct flex_struct; + size_t array_size = 4; + + /* Initialize structure */ + flex_struct.num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + flex_struct.data[i] = 0; + } +} +``` +Because the memory for `flex_struct` is reserved on the stack, no space is reserved for the `data` member. Accessing the `data` member is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +## Compliant Solution (Storage Duration) + +This compliant solution dynamically allocates storage for `flex_array_struct`: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(void) { + struct flex_array_struct *flex_struct; + size_t array_size = 4; + + /* Dynamically allocate memory for the struct */ + flex_struct = (struct flex_array_struct *)malloc( + sizeof(struct flex_array_struct) + + sizeof(int) * array_size); + if (flex_struct == NULL) { + /* Handle error */ + } + + /* Initialize structure */ + flex_struct->num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + flex_struct->data[i] = 0; + } +} +``` + +## Noncompliant Code Example (Copying) + +This noncompliant code example attempts to copy an instance of a structure containing a flexible array member (`struct `flex_array_struct``) by assignment: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(struct flex_array_struct *struct_a, + struct flex_array_struct *struct_b) { + *struct_b = *struct_a; +} +``` +When the structure is copied, the size of the flexible array member is not considered, and only the first member of the structure, `num`, is copied, leaving the array contents untouched. + +## Compliant Solution (Copying) + +This compliant solution uses `memcpy()` to properly copy the content of `struct_a` into `struct_b`: + +```cpp +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void func(struct flex_array_struct *struct_a, + struct flex_array_struct *struct_b) { + if (struct_a->num > struct_b->num) { + /* Insufficient space; handle error */ + return; + } + memcpy(struct_b, struct_a, + sizeof(struct flex_array_struct) + (sizeof(int) + * struct_a->num)); +} +``` + +## Noncompliant Code Example (Function Arguments) + +In this noncompliant code example, the flexible array structure is passed by value to a function that prints the array elements: + +```cpp +#include +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void print_array(struct flex_array_struct struct_p) { + puts("Array is: "); + for (size_t i = 0; i < struct_p.num; ++i) { + printf("%d ", struct_p.data[i]); + } + putchar('\n'); +} + +void func(void) { + struct flex_array_struct *struct_p; + size_t array_size = 4; + + /* Space is allocated for the struct */ + struct_p = (struct flex_array_struct *)malloc( + sizeof(struct flex_array_struct) + + sizeof(int) * array_size); + if (struct_p == NULL) { + /* Handle error */ + } + struct_p->num = array_size; + + for (size_t i = 0; i < array_size; ++i) { + struct_p->data[i] = i; + } + print_array(*struct_p); +} +``` +Because the argument is passed by value, the size of the flexible array member is not considered when the structure is copied, and only the first member of the structure, `num`, is copied. + +## Compliant Solution (Function Arguments) + +In this compliant solution, the structure is passed by reference and not by value: + +```cpp +#include +#include + +struct flex_array_struct { + size_t num; + int data[]; +}; + +void print_array(struct flex_array_struct *struct_p) { + puts("Array is: "); + for (size_t i = 0; i < struct_p->num; ++i) { + printf("%d ", struct_p->data[i]); + } + putchar('\n'); +} + +void func(void) { + struct flex_array_struct *struct_p; + size_t array_size = 4; + + /* Space is allocated for the struct and initialized... */ + + print_array(struct_p); +} +``` + +## Risk Assessment + +Failure to use structures with flexible array members correctly can result in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +
Rule Severity Likelihood Remediation Cost Priority Level
MEM33-C Low Unlikely Low P3 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 flexible-array-member-assignment flexible-array-member-declaration Fully checked
Axivion Bauhaus Suite 7.2.0 CertC-MEM33 Fully implemented
CodeSonar 7.2p0 LANG.STRUCT.DECL.FAM Declaration of Flexible Array Member
Compass/ROSE Can detect all of these
Helix QAC 2022.4 C1061, C1062, C1063, C1064
Klocwork 2022.4 MISRA.INCOMPLETE.STRUCT MISRA.MEMB.FLEX_ARRAY.2012
LDRA tool suite 9.7.1 649 S, 650 S Fully implemented
Parasoft C/C++test 2022.2 CERT_C-MEM33-a CERT_C-MEM33-b Allocate structures containing a flexible array member dynamically Do not copy instances of structures containing a flexible array member
Polyspace Bug Finder R2023a CERT C: Rule MEM33-C Checks for misuse of structure with flexible array member (rule fully covered)
PRQA QA-C 9.7 1061, 1062, 1063, 1064
RuleChecker 22.04 flexible-array-member-assignment flexible-array-member-declaration Fully checked
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MEM33-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C Secure Coding Standard DCL38-C. Use the correct syntax when declaring a flexible array member Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-401 and MEM33-CPP** + +There is no longer a C++ rule for MEM33-CPP. (In fact, all C++ rules from 30-50 are gone, because we changed the numbering system to be 50-99 for C++ rules.) + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] Subclause 6.7.2.1, "Structure and Union Specifiers"
\[ JTC1/SC22/WG14 N791 \] Solving the Struct Hack Problem
+ + +## Implementation notes + +None + +## References + +* CERT-C: [MEM33-C: Allocate and copy structures containing a flexible array member dynamically](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql new file mode 100644 index 0000000000..69f6f9feb9 --- /dev/null +++ b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql @@ -0,0 +1,114 @@ +/** + * @id c/cert/copy-structs-with-a-flexible-array-member-dynamically + * @name MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function. + * @description Copying a structure containing a flexbile array member by assignment ignores the + * flexible array member data. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/cert/id/mem33-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.c.Variable +import semmle.code.cpp.security.BufferAccess + +/** + * An expanded variant of the CodeQL standard library `MemcpyBA` + * class that additionally models the `__builtin___memcpy_chk` function. + */ +class MemcpyBAExpanded extends BufferAccess { + MemcpyBAExpanded() { + this.(FunctionCall).getTarget().getName() = + ["memcmp", "wmemcmp", "_memicmp", "_memicmp_l", "__builtin___memcpy_chk"] + } + + override string getName() { result = this.(FunctionCall).getTarget().getName() } + + override Expr getBuffer(string bufferDesc, int accessType) { + result = this.(FunctionCall).getArgument(0) and + bufferDesc = "destination buffer" and + accessType = 2 + or + result = this.(FunctionCall).getArgument(1) and + bufferDesc = "source buffer" and + accessType = 2 + } + + override int getSize() { + result = + this.(FunctionCall).getArgument(2).getValue().toInt() * + getPointedSize(this.(FunctionCall).getTarget().getParameter(0).getType()) + } +} + +/** + * A class representing an `Expr` that copies a flexible array struct. + */ +abstract class FlexibleArrayCopyExpr extends Expr { } + +/** + * A simple assignment of a flexible array struct to another flexible array struct. + */ +class FlexibleArraySimpleCopyExpr extends FlexibleArrayCopyExpr { + FlexibleArraySimpleCopyExpr() { + exists(Variable v | + this.getUnspecifiedType() instanceof FlexibleArrayStructType and + ( + exists(Initializer init | + init.getDeclaration() = v and + init.getExpr() = this + ) + or + exists(AssignExpr assign | + assign.getLValue().getUnspecifiedType() instanceof FlexibleArrayStructType and + assign.getRValue() = this + ) + ) + ) + } +} + +/** + * A call to a function that copies a flexible array struct. + */ +class FlexibleArrayMemcpyCallExpr extends FlexibleArrayCopyExpr, MemcpyBAExpanded { + FlexibleArrayMemcpyCallExpr() { + not exists(Expr e | + e = this.getBuffer(_, _) and + not e.getType().stripType() instanceof FlexibleArrayStructType + ) + } + + /** + * Holds if the size copied does not account for the flexible array member. + */ + predicate isFlexibleArrayCopiedWithInsufficientSize() { + this.getSize() <= + max(this.getBuffer(_, _) + .getUnderlyingType() + .(DerivedType) + .getBaseType() + .getUnspecifiedType() + .getSize() + ) + } +} + +from FlexibleArrayCopyExpr faCopy, string message +where + not isExcluded(faCopy, Memory2Package::copyStructsWithAFlexibleArrayMemberDynamicallyQuery()) and + ( + // case 1: simple assignment + faCopy instanceof FlexibleArraySimpleCopyExpr and + message = "Struct containing a flexible array member copied by assignment." + or + // case 2: call to memcpy + faCopy.(FlexibleArrayMemcpyCallExpr).isFlexibleArrayCopiedWithInsufficientSize() and + message = + "Struct containing a flexible array member copied by call to memcpy with insufficient size." + ) +select faCopy, message diff --git a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md new file mode 100644 index 0000000000..c5772adb4b --- /dev/null +++ b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md @@ -0,0 +1,168 @@ +# MEM34-C: Only free memory allocated dynamically + +This query implements the CERT-C rule MEM34-C: + +> Only free memory allocated dynamically + + + +## Description + +The C Standard, Annex J \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states that the behavior of a program is [undefined ](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior) when + +> The pointer argument to the `free` or `realloc` function does not match a pointer earlier returned by a memory management function, or the space has been deallocated by a call to `free` or `realloc`. + + +See also [undefined behavior 179](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_179). + +Freeing memory that is not allocated dynamically can result in heap corruption and other serious errors. Do not call `free()` on a pointer other than one returned by a standard memory allocation function, such as `malloc()`, `calloc()`, `realloc()`, or `aligned_alloc()`. + +A similar situation arises when `realloc()` is supplied a pointer to non-dynamically allocated memory. The `realloc()` function is used to resize a block of dynamic memory. If `realloc()` is supplied a pointer to memory not allocated by a standard memory allocation function, the behavior is [undefined](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). One consequence is that the program may [terminate abnormally](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-abnormaltermination). + +This rule does not apply to null pointers. The C Standard guarantees that if `free()` is passed a null pointer, no action occurs. + +## Noncompliant Code Example + +This noncompliant code example sets `c_str` to reference either dynamically allocated memory or a statically allocated string literal depending on the value of `argc`. In either case, `c_str` is passed as an argument to `free()`. If anything other than dynamically allocated memory is referenced by `c_str`, the call to `free(c_str)` is erroneous. + +```cpp +#include +#include +#include + +enum { MAX_ALLOCATION = 1000 }; + +int main(int argc, const char *argv[]) { + char *c_str = NULL; + size_t len; + + if (argc == 2) { + len = strlen(argv[1]) + 1; + if (len > MAX_ALLOCATION) { + /* Handle error */ + } + c_str = (char *)malloc(len); + if (c_str == NULL) { + /* Handle error */ + } + strcpy(c_str, argv[1]); + } else { + c_str = "usage: $>a.exe [string]"; + printf("%s\n", c_str); + } + free(c_str); + return 0; +} + +``` + +## Compliant Solution + +This compliant solution eliminates the possibility of `c_str` referencing memory that is not allocated dynamically when passed to `free()`: + +```cpp +#include +#include +#include + +enum { MAX_ALLOCATION = 1000 }; + +int main(int argc, const char *argv[]) { + char *c_str = NULL; + size_t len; + + if (argc == 2) { + len = strlen(argv[1]) + 1; + if (len > MAX_ALLOCATION) { + /* Handle error */ + } + c_str = (char *)malloc(len); + if (c_str == NULL) { + /* Handle error */ + } + strcpy(c_str, argv[1]); + } else { + printf("%s\n", "usage: $>a.exe [string]"); + return EXIT_FAILURE; + } + free(c_str); + return 0; +} + +``` + +## Noncompliant Code Example (realloc()) + +In this noncompliant example, the pointer parameter to `realloc()`, `buf`, does not refer to dynamically allocated memory: + +```cpp +#include + +enum { BUFSIZE = 256 }; + +void f(void) { + char buf[BUFSIZE]; + char *p = (char *)realloc(buf, 2 * BUFSIZE); + if (p == NULL) { + /* Handle error */ + } +} + +``` + +## Compliant Solution (realloc()) + +In this compliant solution, `buf` refers to dynamically allocated memory: + +```cpp +#include + +enum { BUFSIZE = 256 }; + +void f(void) { + char *buf = (char *)malloc(BUFSIZE * sizeof(char)); + char *p = (char *)realloc(buf, 2 * BUFSIZE); + if (p == NULL) { + /* Handle error */ + } +} +``` +Note that `realloc()` will behave properly even if `malloc()` failed, because when given a null pointer, `realloc()` behaves like a call to `malloc()`. + +## Risk Assessment + +The consequences of this error depend on the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation), but they range from nothing to arbitrary code execution if that memory is reused by `malloc()`. + +
Rule Severity Likelihood Remediation Cost Priority Level
MEM34-C High Likely Medium P18 L1
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 invalid-free Fully checked
Axivion Bauhaus Suite 7.2.0 CertC-MEM34 Can detect memory deallocations for stack objects
Clang 3.9 clang-analyzer-unix.Malloc Checked by clang-tidy ; can detect some instances of this rule, but does not detect all
CodeSonar 7.2p0 ALLOC.TM Type Mismatch
Compass/ROSE Can detect some violations of this rule
Coverity 2017.07 BAD_FREE Identifies calls to free() where the argument is a pointer to a function or an array. It also detects the cases where free() is used on an address-of expression, which can never be heap allocated. Coverity Prevent cannot discover all violations of this rule, so further verification is necessary
Helix QAC 2022.4 DF2721, DF2722, DF2723
Klocwork 2022.4 FNH.MIGHT FNH.MUST
LDRA tool suite 9.7.1 407 S, 483 S, 644 S, 645 S, 125 D Partially implemented
Parasoft C/C++test 2022.2 CERT_C-MEM34-a Do not free resources using invalid pointers
Parasoft Insure++ Runtime analysis
PC-lint Plus 1.4 424, 673 Fully supported
Polyspace Bug Finder R2023a CERT C: Rule MEM34-C Checks for: Invalid free of pointernvalid free of pointer, invalid reallocation of pointernvalid reallocation of pointer. Rule fully covered.
PRQA QA-C 9.7 2721, 2722, 2723
PRQA QA-C++ 4.4 2721 , 2722, 2723
PVS-Studio 7.23 V585 , V726
RuleChecker 22.04 invalid-free Partially checked
TrustInSoft Analyzer 1.38 unclassified ("free expects a free-able address") Exhaustively verified (see one compliant and one non-compliant example ).
+ + +## Related Vulnerabilities + +[CVE-2015-0240](https://securityblog.redhat.com/2015/02/23/samba-vulnerability-cve-2015-0240/) describes a [vulnerability](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) in which an uninitialized pointer is passed to `TALLOC_FREE()`, which is a Samba-specific memory deallocation macro that wraps the `talloc_free()` function. The implementation of `talloc_free()` would access the uninitialized pointer, resulting in a remote [exploit](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-exploit). + +Search for vulnerabilities resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MEM34-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
CERT C Secure Coding Standard MEM31-C. Free dynamically allocated memory when no longer needed Prior to 2018-01-12: CERT: Unspecified Relationship
CERT C MEM51-CPP. Properly deallocate dynamically allocated resources Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961 Reallocating or freeing memory that was not dynamically allocated \[xfree\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-590 , Free of Memory Not on the Heap 2017-07-10: CERT: Exact
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] Subclause J.2, "Undefined Behavior"
\[ Seacord 2013b \] Chapter 4, "Dynamic Memory Management"
+ + +## Implementation notes + +None + +## References + +* CERT-C: [MEM34-C: Only free memory allocated dynamically](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql new file mode 100644 index 0000000000..3ff7564fc9 --- /dev/null +++ b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql @@ -0,0 +1,23 @@ +/** + * @id c/cert/only-free-memory-allocated-dynamically-cert + * @name MEM34-C: Only free memory allocated dynamically + * @description Freeing memory that is not allocated dynamically can lead to heap corruption and + * undefined behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/mem34-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.rules.onlyfreememoryallocateddynamicallyshared.OnlyFreeMemoryAllocatedDynamicallyShared + +class OnlyFreeMemoryAllocatedDynamicallyCertQuery extends OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery { + OnlyFreeMemoryAllocatedDynamicallyCertQuery() { + this = Memory2Package::onlyFreeMemoryAllocatedDynamicallyCertQuery() + } +} diff --git a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md new file mode 100644 index 0000000000..aca1b78530 --- /dev/null +++ b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md @@ -0,0 +1,173 @@ +# MEM36-C: Do not modify the alignment of objects by calling realloc + +This query implements the CERT-C rule MEM36-C: + +> Do not modify the alignment of objects by calling realloc + + + +## Description + +Do not invoke `realloc()` to modify the size of allocated objects that have stricter alignment requirements than those guaranteed by `malloc()`. Storage allocated by a call to the standard `aligned_alloc()` function, for example, can have stricter than normal alignment requirements. The C standard requires only that a pointer returned by `realloc()` be suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement. + +## Noncompliant Code Example + +This noncompliant code example returns a pointer to allocated memory that has been aligned to a 4096-byte boundary. If the `resize` argument to the `realloc()` function is larger than the object referenced by `ptr`, then `realloc()` will allocate new memory that is suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement but may not preserve the stricter alignment of the original object. + +```cpp +#include + +void func(void) { + size_t resize = 1024; + size_t alignment = 1 << 12; + int *ptr; + int *ptr1; + + if (NULL == (ptr = (int *)aligned_alloc(alignment, sizeof(int)))) { + /* Handle error */ + } + + if (NULL == (ptr1 = (int *)realloc(ptr, resize))) { + /* Handle error */ + } +} +``` +**Implementation Details** + +When compiled with GCC 4.1.2 and run on the x86_64 Red Hat Linux platform, the following code produces the following output: + +**CODE** + +```cpp +#include +#include + +int main(void) { + size_t size = 16; + size_t resize = 1024; + size_t align = 1 << 12; + int *ptr; + int *ptr1; + + if (posix_memalign((void **)&ptr, align , size) != 0) { + exit(EXIT_FAILURE); + } + + printf("memory aligned to %zu bytes\n", align); + printf("ptr = %p\n\n", ptr); + + if ((ptr1 = (int*) realloc((int *)ptr, resize)) == NULL) { + exit(EXIT_FAILURE); + } + + puts("After realloc(): \n"); + printf("ptr1 = %p\n", ptr1); + + free(ptr1); + return 0; +} + + +``` +**OUTPUT** + +```cpp +memory aligned to 4096 bytes +ptr = 0x1621b000 + +After realloc(): +ptr1 = 0x1621a010 + +``` +`ptr1` is no longer aligned to 4096 bytes. + +## Compliant Solution + +This compliant solution allocates `resize` bytes of new memory with the same alignment as the old memory, copies the original memory content, and then frees the old memory. This solution has [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation-definedbehavior) because it depends on whether extended alignments in excess of `_Alignof (max_align_t)` are supported and the contexts in which they are supported. If not supported, the behavior of this compliant solution is undefined. + +```cpp +#include +#include + +void func(void) { + size_t resize = 1024; + size_t alignment = 1 << 12; + int *ptr; + int *ptr1; + + if (NULL == (ptr = (int *)aligned_alloc(alignment, + sizeof(int)))) { + /* Handle error */ + } + + if (NULL == (ptr1 = (int *)aligned_alloc(alignment, + resize))) { + /* Handle error */ + } + + if (NULL == memcpy(ptr1, ptr, sizeof(int))) { + /* Handle error */ + } + + free(ptr); +} +``` + +## Compliant Solution (Windows) + +Windows defines the `_aligned_malloc()` function to allocate memory on a specified alignment boundary. The `_aligned_realloc()` \[[MSDN](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-MSDN)\] can be used to change the size of this memory. This compliant solution demonstrates one such usage: + +```cpp +#include + +void func(void) { + size_t alignment = 1 << 12; + int *ptr; + int *ptr1; + + /* Original allocation */ + if (NULL == (ptr = (int *)_aligned_malloc(sizeof(int), + alignment))) { + /* Handle error */ +} + + /* Reallocation */ + if (NULL == (ptr1 = (int *)_aligned_realloc(ptr, 1024, + alignment))) { + _aligned_free(ptr); + /* Handle error */ + } + + _aligned_free(ptr1); +} +``` +The `size` and `alignment` arguments for `_aligned_malloc()` are provided in reverse order of the C Standard `aligned_alloc()` function. + +## Risk Assessment + +Improper alignment can lead to arbitrary memory locations being accessed and written to. + +
Recommendation Severity Likelihood Remediation Cost Priority Level
MEM36-C Low Probable High P2 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported, but no explicit checker
Axivion Bauhaus Suite 7.2.0 CertC-MEM36 Fully implemented
CodeSonar 7.2p0 BADFUNC.REALLOC Use of realloc
Helix QAC 2022.4 C5027 C++5034
Klocwork 2022.4 AUTOSAR.STDLIB.MEMORY
LDRA tool suite 9.7.1 44 S Enhanced enforcement
Parasoft C/C++test 2022.2 CERT_C-MEM36-a Do not modify the alignment of objects by calling realloc()
Polyspace Bug Finder R2023a CERT C: Rule MEM36-C Checks for alignment change after memory allocation (rule fully covered)
PRQA QA-C 9.7 5027
PRQA QA-C++ 4.4 5034
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+MEM36-C). + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 7.22.3.1, "The aligned_alloc Function"
\[ MSDN \] aligned_malloc()
+ + +## Implementation notes + +None + +## References + +* CERT-C: [MEM36-C: Do not modify the alignment of objects by calling realloc](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql new file mode 100644 index 0000000000..79a337f036 --- /dev/null +++ b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql @@ -0,0 +1,58 @@ +/** + * @id c/cert/do-not-modify-alignment-of-memory-with-realloc + * @name MEM36-C: Do not modify the alignment of objects by calling realloc + * @description Realloc does not preserve the alignment of memory allocated with aligned_alloc and + * can result in undefined behavior if reallocating more strictly aligned memory. + * @kind path-problem + * @precision high + * @problem.severity error + * @tags external/cert/id/mem36-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Allocations +import semmle.code.cpp.dataflow.DataFlow +import DataFlow::PathGraph + +int getStatedValue(Expr e) { + // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful + // result in this case we pick the minimum value obtainable from dataflow and range analysis. + result = + upperBound(e) + .minimum(min(Expr source | DataFlow::localExprFlow(source, e) | source.getValue().toInt())) +} + +class NonDefaultAlignedAllocCall extends FunctionCall { + NonDefaultAlignedAllocCall() { + this.getTarget().hasName("aligned_alloc") and + not getStatedValue(this.getArgument(0)) = getGlobalMaxAlignT() + } +} + +class ReallocCall extends FunctionCall { + ReallocCall() { this.getTarget().hasName("realloc") } +} + +class AlignedAllocToReallocConfig extends DataFlow::Configuration { + AlignedAllocToReallocConfig() { this = "AlignedAllocToReallocConfig" } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof NonDefaultAlignedAllocCall + } + + override predicate isSink(DataFlow::Node sink) { + exists(ReallocCall realloc | sink.asExpr() = realloc.getArgument(0)) + } +} + +from AlignedAllocToReallocConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where + not isExcluded(sink.getNode().asExpr(), + Memory2Package::doNotModifyAlignmentOfMemoryWithReallocQuery()) and + cfg.hasFlowPath(source, sink) +select sink, source, sink, "Memory allocated with $@ but reallocated with realloc.", + source.getNode().asExpr(), "aligned_alloc" diff --git a/c/cert/test/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.testref b/c/cert/test/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.testref new file mode 100644 index 0000000000..067c1c5965 --- /dev/null +++ b/c/cert/test/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.testref @@ -0,0 +1 @@ +c/common/test/rules/donotuserelationaloperatorswithdifferingarrays/DoNotUseRelationalOperatorsWithDifferingArrays.ql \ No newline at end of file diff --git a/c/cert/test/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.testref b/c/cert/test/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.testref new file mode 100644 index 0000000000..bbe3b3db8a --- /dev/null +++ b/c/cert/test/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.testref @@ -0,0 +1 @@ +c/common/test/rules/donotsubtractpointersaddressingdifferentarrays/DoNotSubtractPointersAddressingDifferentArrays.ql \ No newline at end of file diff --git a/c/cert/test/rules/EXP42-C/DoNotComparePaddingData.testref b/c/cert/test/rules/EXP42-C/DoNotComparePaddingData.testref new file mode 100644 index 0000000000..fb0d5d283b --- /dev/null +++ b/c/cert/test/rules/EXP42-C/DoNotComparePaddingData.testref @@ -0,0 +1 @@ +c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql \ No newline at end of file diff --git a/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.qlref b/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.qlref deleted file mode 100644 index 53d28c862c..0000000000 --- a/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.ql \ No newline at end of file diff --git a/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.testref b/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.testref new file mode 100644 index 0000000000..960e2354ae --- /dev/null +++ b/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.testref @@ -0,0 +1 @@ +c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.testref b/c/cert/test/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.testref new file mode 100644 index 0000000000..c3215c5533 --- /dev/null +++ b/c/cert/test/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.testref @@ -0,0 +1 @@ +c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.expected b/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.expected new file mode 100644 index 0000000000..0df24e1dcc --- /dev/null +++ b/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.expected @@ -0,0 +1,7 @@ +| test.c:15:13:15:14 | v1 | $@ contains a flexible array member but is not dynamically allocated. | test.c:15:13:15:14 | v1 | v1 | +| test.c:17:8:17:13 | call to malloc | $@ allocated with insufficient memory for its flexible array member. | test.c:16:14:16:15 | v2 | v2 | +| test.c:20:7:20:12 | call to malloc | $@ allocated with insufficient memory for its flexible array member. | test.c:19:14:19:15 | v3 | v3 | +| test.c:22:19:22:24 | call to malloc | $@ allocated with insufficient memory for its flexible array member. | test.c:22:14:22:15 | v4 | v4 | +| test.c:31:30:31:31 | p1 | $@ contains a flexible array member but is not dynamically allocated. | test.c:31:30:31:31 | p1 | p1 | +| test.c:40:13:40:14 | v1 | $@ contains a flexible array member but is not dynamically allocated. | test.c:40:13:40:14 | v1 | v1 | +| test.c:48:13:48:14 | v1 | $@ contains a flexible array member but is not dynamically allocated. | test.c:48:13:48:14 | v1 | v1 | diff --git a/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.qlref b/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.qlref new file mode 100644 index 0000000000..963e5e0175 --- /dev/null +++ b/c/cert/test/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.qlref @@ -0,0 +1 @@ +rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.expected b/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.expected new file mode 100644 index 0000000000..4b9c0bbd65 --- /dev/null +++ b/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.expected @@ -0,0 +1,2 @@ +| test.c:48:18:48:20 | * ... | Struct containing a flexible array member copied by assignment. | +| test.c:49:9:49:11 | * ... | Struct containing a flexible array member copied by assignment. | diff --git a/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.qlref b/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.qlref new file mode 100644 index 0000000000..37493b023f --- /dev/null +++ b/c/cert/test/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.qlref @@ -0,0 +1 @@ +rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM33-C/test.c b/c/cert/test/rules/MEM33-C/test.c new file mode 100644 index 0000000000..364e7b761e --- /dev/null +++ b/c/cert/test/rules/MEM33-C/test.c @@ -0,0 +1,54 @@ +#include +#include + +struct s1 { + int num; + char b[]; +}; + +struct s2 { + int a; + char b[1]; +}; + +void test_alloc(void) { + struct s1 v1; // NON_COMPLIANT + struct s1 *v2; // COMPLIANT + v2 = malloc(sizeof(struct s1)); // NON_COMPLIANT - size does not include space + // for the flexible array + struct s1 *v3 = + malloc(sizeof(struct s1)); // NON_COMPLIANT - size does not include space + // for the flexible array + struct s1 *v4 = malloc( + sizeof(struct s1) - + 1); // NON_COMPLIANT - size does not include space for the flexible array + struct s1 *v5 = malloc(sizeof(struct s1) + 1); // COMPLIANT + struct s2 v6; // COMPLIANT - no flex array + struct s2 *v7 = malloc(sizeof(struct s1)); // COMPLIANT - no flex array +} + +// calls to this function are never compliant +void test_fa_param(struct s1 p1) {} // NON_COMPLIANT + +// calls to this function are always compliant +void test_pfa_param(struct s1 *p1) {} // COMPLIANT + +// calls to this function are always compliant +void test_s_param(struct s2 p1) {} // COMPLIANT + +void test_fa_params_call(void) { + struct s1 v1; // NON_COMPLIANT + struct s1 *v2 = malloc(sizeof(struct s1) + 1); + test_fa_param(v1); // NON_COMPLIANT + test_pfa_param(&v1); // COMPLIANT + test_pfa_param(v2); // COMPLIANT +} + +void test_copy(struct s1 *p1, struct s1 *p2) { + struct s1 v1 = *p2; // NON_COMPLIANT + *p1 = *p2; // NON_COMPLIANT + memcpy(p1, p2, + sizeof(struct s1)); // NON_COMPLIANT - not copying size of array + memcpy(p1, p2, sizeof(struct s1) + 1); // COMPLIANT + memcpy(p1, p2, sizeof(struct s1) + p2->num); // COMPLIANT +} \ No newline at end of file diff --git a/c/cert/test/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.testref b/c/cert/test/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.testref new file mode 100644 index 0000000000..edf7c5cc3b --- /dev/null +++ b/c/cert/test/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.testref @@ -0,0 +1 @@ +c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.expected b/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.expected new file mode 100644 index 0000000000..0592cb038d --- /dev/null +++ b/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.expected @@ -0,0 +1,18 @@ +edges +| test.c:5:10:5:22 | call to aligned_alloc | test.c:15:8:15:28 | call to aligned_alloc_wrapper | +| test.c:8:29:8:31 | ptr | test.c:8:64:8:66 | ptr | +| test.c:15:8:15:28 | call to aligned_alloc_wrapper | test.c:16:24:16:25 | v1 | +| test.c:16:24:16:25 | v1 | test.c:8:29:8:31 | ptr | +| test.c:22:8:22:20 | call to aligned_alloc | test.c:23:16:23:17 | v3 | +nodes +| test.c:5:10:5:22 | call to aligned_alloc | semmle.label | call to aligned_alloc | +| test.c:8:29:8:31 | ptr | semmle.label | ptr | +| test.c:8:64:8:66 | ptr | semmle.label | ptr | +| test.c:15:8:15:28 | call to aligned_alloc_wrapper | semmle.label | call to aligned_alloc_wrapper | +| test.c:16:24:16:25 | v1 | semmle.label | v1 | +| test.c:22:8:22:20 | call to aligned_alloc | semmle.label | call to aligned_alloc | +| test.c:23:16:23:17 | v3 | semmle.label | v3 | +subpaths +#select +| test.c:8:64:8:66 | ptr | test.c:5:10:5:22 | call to aligned_alloc | test.c:8:64:8:66 | ptr | Memory allocated with $@ but reallocated with realloc. | test.c:5:10:5:22 | call to aligned_alloc | aligned_alloc | +| test.c:23:16:23:17 | v3 | test.c:22:8:22:20 | call to aligned_alloc | test.c:23:16:23:17 | v3 | Memory allocated with $@ but reallocated with realloc. | test.c:22:8:22:20 | call to aligned_alloc | aligned_alloc | diff --git a/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.qlref b/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.qlref new file mode 100644 index 0000000000..60d530bf5f --- /dev/null +++ b/c/cert/test/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.qlref @@ -0,0 +1 @@ +rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM36-C/test.c b/c/cert/test/rules/MEM36-C/test.c new file mode 100644 index 0000000000..1f2d159e09 --- /dev/null +++ b/c/cert/test/rules/MEM36-C/test.c @@ -0,0 +1,24 @@ +#include +#include + +void *aligned_alloc_wrapper(size_t alignment, size_t size) { + return aligned_alloc(alignment, size); +} + +void *realloc_wrapper(void *ptr, size_t size) { return realloc(ptr, size); } + +void test_aligned_alloc_to_realloc(void) { + void *v1; + void *v2; + void *v3; + + v1 = aligned_alloc_wrapper(32, 32); + v1 = realloc_wrapper(v1, 64); // NON_COMPLIANT - result reported in wrapper + v1 = realloc(v1, 64); // COMPLIANT + + v2 = aligned_alloc(16, 16); + v2 = realloc(v2, 32); // COMPLIANT - alignment unchanged + + v3 = aligned_alloc(32, 16); + v3 = realloc(v3, 32); // NON_COMPLIANT +} \ No newline at end of file diff --git a/c/common/src/codingstandards/c/Variable.qll b/c/common/src/codingstandards/c/Variable.qll index 4231243be2..6cb18dfb85 100644 --- a/c/common/src/codingstandards/c/Variable.qll +++ b/c/common/src/codingstandards/c/Variable.qll @@ -26,15 +26,15 @@ class FlexibleArrayMember extends FlexibleArrayMemberCandidate { * includes any sized array (either specified or not) */ class FlexibleArrayMemberCandidate extends MemberVariable { - Struct s; - FlexibleArrayMemberCandidate() { this.getType() instanceof ArrayType and - this.getDeclaringType() = s and - not exists(int i, int j | - s.getAMember(i) = this and - exists(s.getAMember(j)) and - j > i + exists(Struct s | + this.getDeclaringType() = s and + not exists(int i, int j | + s.getAMember(i) = this and + exists(s.getAMember(j)) and + j > i + ) ) } } @@ -52,3 +52,17 @@ Variable getAddressOfExprTargetBase(AddressOfExpr expr) { or result = expr.getOperand().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() } + + +/** + * A struct that contains a flexible array member + */ +class FlexibleArrayStructType extends Struct { + FlexibleArrayMember member; + + FlexibleArrayStructType() { + this = member.getDeclaringType() + } + + FlexibleArrayMember getFlexibleArrayMember() { result = member } +} diff --git a/c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.expected b/c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.expected similarity index 100% rename from c/cert/test/rules/FIO42-C/CloseFilesWhenTheyAreNoLongerNeeded.expected rename to c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.expected diff --git a/c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql b/c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql new file mode 100644 index 0000000000..1769b6862e --- /dev/null +++ b/c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.closefilehandlewhennolongerneededshared.CloseFileHandleWhenNoLongerNeededShared diff --git a/c/cert/test/rules/FIO42-C/test.c b/c/common/test/rules/closefilehandlewhennolongerneededshared/test.c similarity index 100% rename from c/cert/test/rules/FIO42-C/test.c rename to c/common/test/rules/closefilehandlewhennolongerneededshared/test.c diff --git a/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.expected b/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.expected new file mode 100644 index 0000000000..0844b14417 --- /dev/null +++ b/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.expected @@ -0,0 +1,4 @@ +| test.c:5:13:5:18 | call to malloc | The memory allocated here may not be freed at $@. | test.c:4:15:11:1 | { ... } | this location | +| test.c:14:13:14:19 | call to realloc | The memory allocated here may not be freed at $@. | test.c:13:25:20:1 | { ... } | this location | +| test.c:23:13:23:19 | call to realloc | The memory allocated here may not be freed at $@. | test.c:22:26:29:1 | { ... } | this location | +| test.c:42:13:42:18 | call to malloc | The memory allocated here may not be freed at $@. | test.c:51:3:51:11 | return ... | this location | diff --git a/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql b/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql new file mode 100644 index 0000000000..6656768011 --- /dev/null +++ b/c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.freememorywhennolongerneededshared.FreeMemoryWhenNoLongerNeededShared diff --git a/c/common/test/rules/freememorywhennolongerneededshared/test.c b/c/common/test/rules/freememorywhennolongerneededshared/test.c new file mode 100644 index 0000000000..85a86594ba --- /dev/null +++ b/c/common/test/rules/freememorywhennolongerneededshared/test.c @@ -0,0 +1,82 @@ +#include +#include + +int f1a(void) { + void *p = malloc(10); // NON_COMPLIANT + if (NULL == p) { + return -1; + } + /* ... */ + return 0; +} + +int f1b(const void *in) { + void *p = realloc(in, 10); // NON_COMPLIANT + if (NULL == p) { + return -1; + } + /* ... */ + return 0; +} + +void f1c(const void *in) { + void *p = realloc(in, 10); // NON_COMPLIANT + if (NULL == p) { + return; + } + /* ... */ + // pointer out of scope not freed +} + +int f2a(void) { + void *p = malloc(10); // COMPLIANT + if (NULL == p) { + return -1; + } + /* ... */ + free(p); + return 0; +} + +int f2b(int test) { + void *p = malloc(10); // NON_COMPLIANT + if (NULL == p) { + return -1; + } + if (test == 1) { + free(p); + return -1; + } + // memory not freed on this path + return 0; +} + +// scope prolonged +int f2c(void) { + void *q; + { + void *p = malloc(10); // COMPLIANT + if (NULL == p) { + return -1; + } + /* ... */ + q = p; + // p out of scope + } + free(q); + return 0; +} + +// interprocedural +int free_helper(void *g) { + free(g); + return 0; +} + +int f2inter(void) { + void *p = malloc(10); // COMPLIANT + if (NULL == p) { + return -1; + } + return free_helper(p); +} \ No newline at end of file diff --git a/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected b/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected new file mode 100644 index 0000000000..01b4649fa8 --- /dev/null +++ b/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected @@ -0,0 +1 @@ +| test.c:13:8:13:13 | call to memcmp | memcmp accesses bits which are not part of the object's value representation. | diff --git a/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql b/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql new file mode 100644 index 0000000000..f924c33f1d --- /dev/null +++ b/c/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.memcmpusedtocomparepaddingdata.MemcmpUsedToComparePaddingData diff --git a/c/common/test/rules/memcmpusedtocomparepaddingdata/test.c b/c/common/test/rules/memcmpusedtocomparepaddingdata/test.c new file mode 100644 index 0000000000..00cf4c3230 --- /dev/null +++ b/c/common/test/rules/memcmpusedtocomparepaddingdata/test.c @@ -0,0 +1,20 @@ +#include + +struct S1 { + unsigned char buffType; + int size; +}; + +struct S2 { + unsigned char buff[8]; +}; + +void f1(const struct S1 *s1, const struct S1 *s2) { + if (!memcmp(s1, s2, sizeof(struct S1))) { // NON_COMPLIANT + } +} + +void f2(const struct S2 *s1, const struct S2 *s2) { + if (!memcmp(&s1, &s2, sizeof(struct S2))) { // COMPLIANT + } +} \ No newline at end of file diff --git a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected new file mode 100644 index 0000000000..84b0cb0ba3 --- /dev/null +++ b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected @@ -0,0 +1,6 @@ +| test.c:8:8:8:10 | g_p | Free expression frees non-dynamically allocated memory. | test.c:8:8:8:10 | g_p | | +| test.c:10:8:10:10 | g_p | Free expression frees $@ which was not dynamically allocated. | test.c:9:9:9:12 | & ... | memory | +| test.c:15:33:15:35 | g_p | Free expression frees non-dynamically allocated memory. | test.c:15:33:15:35 | g_p | | +| test.c:17:36:17:38 | ptr | Free expression frees $@ which was not dynamically allocated. | test.c:24:7:24:8 | & ... | memory | +| test.c:23:8:23:8 | p | Free expression frees $@ which was not dynamically allocated. | test.c:22:13:22:14 | & ... | memory | +| test.c:42:10:42:10 | p | Free expression frees non-dynamically allocated memory. | test.c:42:10:42:10 | p | | diff --git a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql new file mode 100644 index 0000000000..a678006d69 --- /dev/null +++ b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.onlyfreememoryallocateddynamicallyshared.OnlyFreeMemoryAllocatedDynamicallyShared diff --git a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c new file mode 100644 index 0000000000..20b39454a1 --- /dev/null +++ b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c @@ -0,0 +1,47 @@ +#include +#include + +int g_i = 0; +void *g_p = &g_i; + +void test_global(void) { + free(g_p); // NON_COMPLIANT + g_p = &g_i; + free(g_p); // NON_COMPLIANT + g_p = malloc(10); + free(g_p); // COMPLIANT - but could be written to in different scope +} + +void test_global_b(void) { free(g_p); } // NON_COMPLIANT + +void free_nested(void *ptr) { free(ptr); } // NON_COMPLIANT - some paths + +void test_local(void) { + int i; + int j; + void *p = &i; + free(p); // NON_COMPLIANT + p = &j; + free_nested(p); // NON_COMPLIANT + p = malloc(10); + free(p); // COMPLIANT + p = malloc(10); + free_nested(p); // COMPLIANT +} + +struct S { + int i; + void *p; +}; + +void test_local_field_nested(struct S *s) { free(s->p); } // COMPLIANT + +void test_local_field(void) { + struct S s; + s.p = &s.i; + free(s.p); // NON_COMPLIANT + s.p = malloc(10); + free(s.p); // COMPLIANT + s.p = malloc(10); + test_local_field_nested(&s); +} \ No newline at end of file diff --git a/c/misra/src/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.ql b/c/misra/src/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.ql new file mode 100644 index 0000000000..66f4625584 --- /dev/null +++ b/c/misra/src/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.ql @@ -0,0 +1,23 @@ +/** + * @id c/misra/close-file-handle-when-no-longer-needed-misra + * @name RULE-22-1: File handles acquired with Standard Library functions shall be explicitly closed + * @description File handles acquired with standard library functions should be released to avoid + * resource exhaustion. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-22-1 + * correctness + * security + * external/misra/obligation/required + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.rules.closefilehandlewhennolongerneededshared.CloseFileHandleWhenNoLongerNeededShared + +class CloseFileHandleWhenNoLongerNeededMisraQuery extends CloseFileHandleWhenNoLongerNeededSharedSharedQuery { + CloseFileHandleWhenNoLongerNeededMisraQuery() { + this = Memory2Package::closeFileHandleWhenNoLongerNeededMisraQuery() + } +} diff --git a/c/misra/src/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.ql b/c/misra/src/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.ql new file mode 100644 index 0000000000..1650590559 --- /dev/null +++ b/c/misra/src/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.ql @@ -0,0 +1,23 @@ +/** + * @id c/misra/free-memory-when-no-longer-needed-misra + * @name RULE-22-1: Memory allocated dynamically with Standard Library functions shall be explicitly released + * @description Memory allocated dynamically with standard library functions should be freed to + * avoid memory leaks. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-22-1 + * correctness + * security + * external/misra/obligation/required + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.rules.freememorywhennolongerneededshared.FreeMemoryWhenNoLongerNeededShared + +class FreeMemoryWhenNoLongerNeededMisraQuery extends FreeMemoryWhenNoLongerNeededSharedSharedQuery { + FreeMemoryWhenNoLongerNeededMisraQuery() { + this = Memory2Package::freeMemoryWhenNoLongerNeededMisraQuery() + } +} diff --git a/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql b/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql new file mode 100644 index 0000000000..9293ebe716 --- /dev/null +++ b/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql @@ -0,0 +1,23 @@ +/** + * @id c/misra/only-free-memory-allocated-dynamically-misra + * @name RULE-22-2: A block of memory shall only be freed if it was allocated by means of a Standard Library function + * @description Freeing memory that is not allocated dynamically can lead to heap corruption and + * undefined behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/misra/id/rule-22-2 + * correctness + * security + * external/misra/obligation/mandatory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.cpp.rules.onlyfreememoryallocateddynamicallyshared.OnlyFreeMemoryAllocatedDynamicallyShared + +class OnlyFreeMemoryAllocatedDynamicallyMisraQuery extends OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery { + OnlyFreeMemoryAllocatedDynamicallyMisraQuery() { + this = Memory2Package::onlyFreeMemoryAllocatedDynamicallyMisraQuery() + } +} diff --git a/c/misra/test/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.testref b/c/misra/test/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.testref new file mode 100644 index 0000000000..960e2354ae --- /dev/null +++ b/c/misra/test/rules/RULE-22-1/CloseFileHandleWhenNoLongerNeededMisra.testref @@ -0,0 +1 @@ +c/common/test/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.testref b/c/misra/test/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.testref new file mode 100644 index 0000000000..c3215c5533 --- /dev/null +++ b/c/misra/test/rules/RULE-22-1/FreeMemoryWhenNoLongerNeededMisra.testref @@ -0,0 +1 @@ +c/common/test/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.testref b/c/misra/test/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.testref new file mode 100644 index 0000000000..edf7c5cc3b --- /dev/null +++ b/c/misra/test/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.testref @@ -0,0 +1 @@ +c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.ql \ No newline at end of file diff --git a/change_notes/2023-03-14-fio42-c-fix-logic-error.md b/change_notes/2023-03-14-fio42-c-fix-logic-error.md new file mode 100644 index 0000000000..c4b8019789 --- /dev/null +++ b/change_notes/2023-03-14-fio42-c-fix-logic-error.md @@ -0,0 +1,3 @@ + - `FIO42-C` - `CloseFilesWhenTheyAreNoLongerNeeded.ql`: + - Parentheses have been added to a resolve previously lacking parentheses in the `where` clause, such that the exclusion mechanism only functioned for a certain subset of results. + - The query implementation has been moved to a shared implementation. \ No newline at end of file diff --git a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md index c5ecafae1a..44380a3b49 100644 --- a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md +++ b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md @@ -3,155 +3,9 @@ This query implements the CERT-C++ rule EXP62-CPP: > Do not access the bits of an object representation that are not part of the object's value representation +## CERT - -## Description - -The C++ Standard, \[basic.types\], paragraph 9 \[[ISO/IEC 14882-2014](https://wiki.sei.cmu.edu/confluence/display/cplusplus/AA.+Bibliography#AA.Bibliography-ISO%2FIEC14882-2014)\], states the following: - -> The *object representation* of an object of type `T` is the sequence of *N* `unsigned char` objects taken up by the object of type `T`, where *N* equals `sizeof(T)`. The *value representation* of an object is the set of bits that hold the value of type `T`. - - -The narrow character types (`char`, `signed char`, and `unsigned char`)—as well as some other integral types on specific platforms—have an object representation that consists solely of the bits from the object's value representation. For such types, accessing any of the bits of the value representation is well-defined behavior. This form of object representation allows a programmer to access and modify an object solely based on its bit representation, such as by calling `std::memcmp()` on its object representation. - -Other types, such as classes, may not have an object representation composed solely of the bits from the object's value representation. For instance, classes may have bit-field data members, padding inserted between data members, a [vtable](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vtable) to support virtual method dispatch, or data members declared with different access privileges. For such types, accessing bits of the object representation that are not part of the object's value representation may result in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-undefinedbehavior) depending on how those bits are accessed. - -Do not access the bits of an object representation that are not part of the object's value representation. Even if the bits are accessed in a well-defined manner, such as through an array of `unsigned char` objects, the values represented by those bits are unspecified or implementation-defined, and reliance on any particular value can lead to abnormal program execution. - -## Noncompliant Code Example - -In this noncompliant code example, the complete object representation is accessed when comparing two objects of type `S`. Per the C++ Standard, \[class\], paragraph 13 \[[ISO/IEC 14882-2014](https://wiki.sei.cmu.edu/confluence/display/cplusplus/AA.+Bibliography#AA.Bibliography-ISO%2FIEC14882-2014)\], classes may be padded with data to ensure that they are properly aligned in memory. The contents of the padding and the amount of padding added is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation-definedbehavior). This can lead to incorrect results when comparing the object representation of classes instead of the value representation, as the padding may assume different [unspecified values](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-unspecifiedvalue) for each object instance. - -```cpp -#include - -struct S { - unsigned char buffType; - int size; -}; - -void f(const S &s1, const S &s2) { - if (!std::memcmp(&s1, &s2, sizeof(S))) { - // ... - } -} -``` - -## Compliant Solution - -In this compliant solution, `S` overloads `operator==()` to perform a comparison of the value representation of the object. - -```cpp -struct S { - unsigned char buffType; - int size; - - friend bool operator==(const S &lhs, const S &rhs) { - return lhs.buffType == rhs.buffType && - lhs.size == rhs.size; - } -}; - -void f(const S &s1, const S &s2) { - if (s1 == s2) { - // ... - } -} -``` - -## Noncompliant Code Example - -In this noncompliant code example, `std::memset()` is used to clear the internal state of an object. An [implementation](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation) may store a vtable within the object instance due to the presence of a virtual function, and that vtable is subsequently overwritten by the call to `std::memset()`, leading to [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-undefinedbehavior) when virtual method dispatch is required. - -```cpp -#include - -struct S { - int i, j, k; - - // ... - - virtual void f(); -}; - -void f() { - S *s = new S; - // ... - std::memset(s, 0, sizeof(S)); - // ... - s->f(); // undefined behavior -} -``` - -## Compliant Solution - -In this compliant solution, the data members of `S` are cleared explicitly instead of calling `std::memset().` - -```cpp -struct S { - int i, j, k; - - // ... - - virtual void f(); - void clear() { i = j = k = 0; } -}; - -void f() { - S *s = new S; - // ... - s->clear(); - // ... - s->f(); // ok -} -``` - -## Exceptions - -**EXP62-CPP-EX1:** It is permissible to access the bits of an object representation when that access is otherwise unobservable in well-defined code. Specifically, reading bits that are not part of the value representation is permissible when there is no reliance or assumptions placed on their values, and writing bits that are not part of the value representation is only permissible when those bits are padding bits. This exception does not permit writing to bits that are part of the object representation aside from padding bits, such as overwriting a vtable pointer. - -For instance, it is acceptable to call `std::memcpy()` on an object containing a bit-field, as in the following example, because the read and write of the padding bits cannot be observed. - -```cpp -#include - -struct S { - int i : 10; - int j; -}; - -void f(const S &s1) { - S s2; - std::memcpy(&s2, &s1, sizeof(S)); -} -``` -Code that complies with this exception must still comply with [OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions](https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions). - -## Risk Assessment - -The effects of accessing bits of an object representation that are not part of the object's value representation can range from [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation-definedbehavior) (such as assuming the layout of fields with differing access controls) to code execution [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vulnerability) (such as overwriting the vtable pointer). - -
Rule Severity Likelihood Remediation Cost Priority Level
EXP62-CPP High Probable High P6 L2
- - -## Automated Detection - -
Tool Version Checker Description
Astrée 20.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.0p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.2 C++4726, C++4727, C++4728, C++4729, C++4731, C++4732, C++4733, C++4734
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.1 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
PVS-Studio 7.19 V598 , V780, V1084
- - -## Related Vulnerabilities - -Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vulnerabil) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+EXP62-CPP). - -## Related Guidelines - -
SEI CERT C++ Coding Standard OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions
- - -## Bibliography - -
\[ ISO/IEC 14882-2014 \] Subclause 3.9, "Types" Subclause 3.10, "Lvalues and Rvalues" Clause 9, "Classes"
- +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** ## Implementation notes diff --git a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.ql b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.ql index c57a5f2f9b..4b8b67368f 100644 --- a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.ql +++ b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.ql @@ -13,18 +13,10 @@ import cpp import codingstandards.cpp.cert -import semmle.code.cpp.padding.Padding -import semmle.code.cpp.security.BufferAccess -import VirtualTable +import codingstandards.cpp.rules.memcmpusedtocomparepaddingdata.MemcmpUsedToComparePaddingData -from MemcmpBA cmp -where - not isExcluded(cmp, RepresentationPackage::memcmpUsedToAccessObjectRepresentationQuery()) and - cmp.getBuffer(_, _) - .getUnconverted() - .getUnspecifiedType() - .(PointerType) - .getBaseType() - .getUnspecifiedType() instanceof PaddedType -select cmp, - cmp.getName() + " accesses bits which are not part of the object's value representation." +class MemcmpUsedToAccessObjectRepresentationQuery extends MemcmpUsedToComparePaddingDataSharedQuery { + MemcmpUsedToAccessObjectRepresentationQuery() { + this = RepresentationPackage::memcmpUsedToAccessObjectRepresentationQuery() + } +} diff --git a/cpp/cert/src/rules/MEM57-CPP/UsingDefaultOperatorNewForOverAlignedTypes.ql b/cpp/cert/src/rules/MEM57-CPP/UsingDefaultOperatorNewForOverAlignedTypes.ql index 8fc33f8457..f8a5247ff1 100644 --- a/cpp/cert/src/rules/MEM57-CPP/UsingDefaultOperatorNewForOverAlignedTypes.ql +++ b/cpp/cert/src/rules/MEM57-CPP/UsingDefaultOperatorNewForOverAlignedTypes.ql @@ -15,6 +15,7 @@ import cpp import codingstandards.cpp.cert import codingstandards.cpp.Alignment +import codingstandards.cpp.Allocations from NewOrNewArrayExpr newExpr, Type overAlignedType where diff --git a/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.expected b/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.expected deleted file mode 100644 index bb0dc52e0c..0000000000 --- a/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.expected +++ /dev/null @@ -1 +0,0 @@ -| test.cpp:19:8:19:18 | call to memcmp | memcmp accesses bits which are not part of the object's value representation. | diff --git a/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.qlref b/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.qlref deleted file mode 100644 index 103173391a..0000000000 --- a/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.qlref +++ /dev/null @@ -1 +0,0 @@ -rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.testref b/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.testref new file mode 100644 index 0000000000..aacddd73c6 --- /dev/null +++ b/cpp/cert/test/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.testref @@ -0,0 +1 @@ +cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql \ No newline at end of file diff --git a/cpp/cert/test/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.expected b/cpp/cert/test/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.expected index 23ba8ceb60..b690d25f2b 100644 --- a/cpp/cert/test/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.expected +++ b/cpp/cert/test/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.expected @@ -1 +1 @@ -| test.cpp:32:3:32:13 | call to memcpy | call to memcpy accesses bits which are not part of the object's value representation. | +| test.cpp:12:3:12:13 | call to memcpy | call to memcpy accesses bits which are not part of the object's value representation. | diff --git a/cpp/cert/test/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.expected b/cpp/cert/test/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.expected index 56b93ef68b..1ad5885d8d 100644 --- a/cpp/cert/test/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.expected +++ b/cpp/cert/test/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.expected @@ -1 +1 @@ -| test.cpp:59:3:59:13 | call to memset | call to memset accesses bits which are not part of the object's value representation. | +| test.cpp:39:3:39:13 | call to memset | call to memset accesses bits which are not part of the object's value representation. | diff --git a/cpp/cert/test/rules/EXP62-CPP/test.cpp b/cpp/cert/test/rules/EXP62-CPP/test.cpp index 818686e1ff..1f80dc04c2 100644 --- a/cpp/cert/test/rules/EXP62-CPP/test.cpp +++ b/cpp/cert/test/rules/EXP62-CPP/test.cpp @@ -1,25 +1,5 @@ #include -struct S { - unsigned char buffType; - int size; - - friend bool operator==(const S &lhs, const S &rhs) { - return lhs.buffType == rhs.buffType && lhs.size == rhs.size; - } -}; - -void f(const S &s1, const S &s2) { - if (s1 == s2) { - // COMPLIANT S overloads operator==() to perform a comparison of the value - // representation of the object - } -} -void f1(const S &s1, const S &s2) { - if (!std::memcmp(&s1, &s2, sizeof(S))) { // NON_COMPLIANT - } -} - struct S1 { int i, j, k; diff --git a/cpp/common/src/codingstandards/cpp/Allocations.qll b/cpp/common/src/codingstandards/cpp/Allocations.qll index db47b0b028..f0523d2d0b 100644 --- a/cpp/common/src/codingstandards/cpp/Allocations.qll +++ b/cpp/common/src/codingstandards/cpp/Allocations.qll @@ -168,3 +168,21 @@ predicate freeExprOrIndirect(Expr free, Expr freed, string kind) { free.(FunctionCall).getArgument(arg) = freed ) } + +class MaxAlignT extends TypedefType { + MaxAlignT() { getName() = "max_align_t" } +} + +/** + * Gets the alignment for `max_align_t`, assuming there is a single consistent alignment for the + * database. + * + * In theory, each compilation of each file can have a different `max_align_t` value (for example, + * if the same file is compiled under different compilers in the same database). We don't have the + * fine-grained data to determine which compilation each operator new call is from, so only hold in + * cases where there's a single clear alignment for the whole database. + */ +int getGlobalMaxAlignT() { + count(MaxAlignT m | | m.getAlignment()) = 1 and + result = any(MaxAlignT t).getAlignment() +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Memory2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Memory2.qll new file mode 100644 index 0000000000..4f537020fa --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Memory2.qll @@ -0,0 +1,197 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Memory2Query = + TDoNotSubtractPointersThatDoNotReferToTheSameArrayQuery() or + TDoNotRelatePointersThatDoNotReferToTheSameArrayQuery() or + TDoNotComparePaddingDataQuery() or + TFreeMemoryWhenNoLongerNeededCertQuery() or + TAllocStructsWithAFlexibleArrayMemberDynamicallyQuery() or + TCopyStructsWithAFlexibleArrayMemberDynamicallyQuery() or + TOnlyFreeMemoryAllocatedDynamicallyCertQuery() or + TDoNotModifyAlignmentOfMemoryWithReallocQuery() or + TFreeMemoryWhenNoLongerNeededMisraQuery() or + TCloseFileHandleWhenNoLongerNeededMisraQuery() or + TOnlyFreeMemoryAllocatedDynamicallyMisraQuery() + +predicate isMemory2QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `doNotSubtractPointersThatDoNotReferToTheSameArray` query + Memory2Package::doNotSubtractPointersThatDoNotReferToTheSameArrayQuery() and + queryId = + // `@id` for the `doNotSubtractPointersThatDoNotReferToTheSameArray` query + "c/cert/do-not-subtract-pointers-that-do-not-refer-to-the-same-array" and + ruleId = "ARR36-C" and + category = "rule" + or + query = + // `Query` instance for the `doNotRelatePointersThatDoNotReferToTheSameArray` query + Memory2Package::doNotRelatePointersThatDoNotReferToTheSameArrayQuery() and + queryId = + // `@id` for the `doNotRelatePointersThatDoNotReferToTheSameArray` query + "c/cert/do-not-relate-pointers-that-do-not-refer-to-the-same-array" and + ruleId = "ARR36-C" and + category = "rule" + or + query = + // `Query` instance for the `doNotComparePaddingData` query + Memory2Package::doNotComparePaddingDataQuery() and + queryId = + // `@id` for the `doNotComparePaddingData` query + "c/cert/do-not-compare-padding-data" and + ruleId = "EXP42-C" and + category = "rule" + or + query = + // `Query` instance for the `freeMemoryWhenNoLongerNeededCert` query + Memory2Package::freeMemoryWhenNoLongerNeededCertQuery() and + queryId = + // `@id` for the `freeMemoryWhenNoLongerNeededCert` query + "c/cert/free-memory-when-no-longer-needed-cert" and + ruleId = "MEM31-C" and + category = "rule" + or + query = + // `Query` instance for the `allocStructsWithAFlexibleArrayMemberDynamically` query + Memory2Package::allocStructsWithAFlexibleArrayMemberDynamicallyQuery() and + queryId = + // `@id` for the `allocStructsWithAFlexibleArrayMemberDynamically` query + "c/cert/alloc-structs-with-a-flexible-array-member-dynamically" and + ruleId = "MEM33-C" and + category = "rule" + or + query = + // `Query` instance for the `copyStructsWithAFlexibleArrayMemberDynamically` query + Memory2Package::copyStructsWithAFlexibleArrayMemberDynamicallyQuery() and + queryId = + // `@id` for the `copyStructsWithAFlexibleArrayMemberDynamically` query + "c/cert/copy-structs-with-a-flexible-array-member-dynamically" and + ruleId = "MEM33-C" and + category = "rule" + or + query = + // `Query` instance for the `onlyFreeMemoryAllocatedDynamicallyCert` query + Memory2Package::onlyFreeMemoryAllocatedDynamicallyCertQuery() and + queryId = + // `@id` for the `onlyFreeMemoryAllocatedDynamicallyCert` query + "c/cert/only-free-memory-allocated-dynamically-cert" and + ruleId = "MEM34-C" and + category = "rule" + or + query = + // `Query` instance for the `doNotModifyAlignmentOfMemoryWithRealloc` query + Memory2Package::doNotModifyAlignmentOfMemoryWithReallocQuery() and + queryId = + // `@id` for the `doNotModifyAlignmentOfMemoryWithRealloc` query + "c/cert/do-not-modify-alignment-of-memory-with-realloc" and + ruleId = "MEM36-C" and + category = "rule" + or + query = + // `Query` instance for the `freeMemoryWhenNoLongerNeededMisra` query + Memory2Package::freeMemoryWhenNoLongerNeededMisraQuery() and + queryId = + // `@id` for the `freeMemoryWhenNoLongerNeededMisra` query + "c/misra/free-memory-when-no-longer-needed-misra" and + ruleId = "RULE-22-1" and + category = "required" + or + query = + // `Query` instance for the `closeFileHandleWhenNoLongerNeededMisra` query + Memory2Package::closeFileHandleWhenNoLongerNeededMisraQuery() and + queryId = + // `@id` for the `closeFileHandleWhenNoLongerNeededMisra` query + "c/misra/close-file-handle-when-no-longer-needed-misra" and + ruleId = "RULE-22-1" and + category = "required" + or + query = + // `Query` instance for the `onlyFreeMemoryAllocatedDynamicallyMisra` query + Memory2Package::onlyFreeMemoryAllocatedDynamicallyMisraQuery() and + queryId = + // `@id` for the `onlyFreeMemoryAllocatedDynamicallyMisra` query + "c/misra/only-free-memory-allocated-dynamically-misra" and + ruleId = "RULE-22-2" and + category = "mandatory" +} + +module Memory2Package { + Query doNotSubtractPointersThatDoNotReferToTheSameArrayQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotSubtractPointersThatDoNotReferToTheSameArray` query + TQueryC(TMemory2PackageQuery(TDoNotSubtractPointersThatDoNotReferToTheSameArrayQuery())) + } + + Query doNotRelatePointersThatDoNotReferToTheSameArrayQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotRelatePointersThatDoNotReferToTheSameArray` query + TQueryC(TMemory2PackageQuery(TDoNotRelatePointersThatDoNotReferToTheSameArrayQuery())) + } + + Query doNotComparePaddingDataQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotComparePaddingData` query + TQueryC(TMemory2PackageQuery(TDoNotComparePaddingDataQuery())) + } + + Query freeMemoryWhenNoLongerNeededCertQuery() { + //autogenerate `Query` type + result = + // `Query` type for `freeMemoryWhenNoLongerNeededCert` query + TQueryC(TMemory2PackageQuery(TFreeMemoryWhenNoLongerNeededCertQuery())) + } + + Query allocStructsWithAFlexibleArrayMemberDynamicallyQuery() { + //autogenerate `Query` type + result = + // `Query` type for `allocStructsWithAFlexibleArrayMemberDynamically` query + TQueryC(TMemory2PackageQuery(TAllocStructsWithAFlexibleArrayMemberDynamicallyQuery())) + } + + Query copyStructsWithAFlexibleArrayMemberDynamicallyQuery() { + //autogenerate `Query` type + result = + // `Query` type for `copyStructsWithAFlexibleArrayMemberDynamically` query + TQueryC(TMemory2PackageQuery(TCopyStructsWithAFlexibleArrayMemberDynamicallyQuery())) + } + + Query onlyFreeMemoryAllocatedDynamicallyCertQuery() { + //autogenerate `Query` type + result = + // `Query` type for `onlyFreeMemoryAllocatedDynamicallyCert` query + TQueryC(TMemory2PackageQuery(TOnlyFreeMemoryAllocatedDynamicallyCertQuery())) + } + + Query doNotModifyAlignmentOfMemoryWithReallocQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotModifyAlignmentOfMemoryWithRealloc` query + TQueryC(TMemory2PackageQuery(TDoNotModifyAlignmentOfMemoryWithReallocQuery())) + } + + Query freeMemoryWhenNoLongerNeededMisraQuery() { + //autogenerate `Query` type + result = + // `Query` type for `freeMemoryWhenNoLongerNeededMisra` query + TQueryC(TMemory2PackageQuery(TFreeMemoryWhenNoLongerNeededMisraQuery())) + } + + Query closeFileHandleWhenNoLongerNeededMisraQuery() { + //autogenerate `Query` type + result = + // `Query` type for `closeFileHandleWhenNoLongerNeededMisra` query + TQueryC(TMemory2PackageQuery(TCloseFileHandleWhenNoLongerNeededMisraQuery())) + } + + Query onlyFreeMemoryAllocatedDynamicallyMisraQuery() { + //autogenerate `Query` type + result = + // `Query` type for `onlyFreeMemoryAllocatedDynamicallyMisra` query + TQueryC(TMemory2PackageQuery(TOnlyFreeMemoryAllocatedDynamicallyMisraQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index e427fdef5b..8310ec0164 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -32,6 +32,7 @@ import IO4 import InvalidMemory1 import Language1 import Language2 +import Memory2 import Misc import Pointers1 import Pointers2 @@ -81,6 +82,7 @@ newtype TCQuery = TInvalidMemory1PackageQuery(InvalidMemory1Query q) or TLanguage1PackageQuery(Language1Query q) or TLanguage2PackageQuery(Language2Query q) or + TMemory2PackageQuery(Memory2Query q) or TMiscPackageQuery(MiscQuery q) or TPointers1PackageQuery(Pointers1Query q) or TPointers2PackageQuery(Pointers2Query q) or @@ -130,6 +132,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isInvalidMemory1QueryMetadata(query, queryId, ruleId, category) or isLanguage1QueryMetadata(query, queryId, ruleId, category) or isLanguage2QueryMetadata(query, queryId, ruleId, category) or + isMemory2QueryMetadata(query, queryId, ruleId, category) or isMiscQueryMetadata(query, queryId, ruleId, category) or isPointers1QueryMetadata(query, queryId, ruleId, category) or isPointers2QueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll b/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll new file mode 100644 index 0000000000..a1df75fe11 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll @@ -0,0 +1,168 @@ +/** + * Provides a library which includes a `problems` predicate for reporting + * file handles which are open but not closed before they go out of scope. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import semmle.code.cpp.controlflow.StackVariableReachability +import codingstandards.cpp.standardlibrary.FileAccess +import codingstandards.cpp.Allocations + +/** + * 'call' is either a direct call to f, or a possible call to f + * via a function pointer. + */ +predicate mayCallFunction(Expr call, Function f) { + call.(FunctionCall).getTarget() = f or + call.(VariableCall).getVariable().getAnAssignedValue().getAChild*().(FunctionAccess).getTarget() = + f +} + +predicate fopenCallOrIndirect(Expr e) { + // direct allocation call + allocExpr(e, _) and + // We are only interested in allocation calls that are + // actually freed somehow, as MemoryNeverFreed + // will catch those that aren't. + fopenCallMayBeClosed(e) + or + exists(ReturnStmt rtn | + // indirect fopen call + mayCallFunction(e, rtn.getEnclosingFunction()) and + ( + // return fopen + fopenCallOrIndirect(rtn.getExpr()) + or + // return variable assigned with fopen + exists(Variable v | + v = rtn.getExpr().(VariableAccess).getTarget() and + fopenCallOrIndirect(v.getAnAssignedValue()) and + not assignedToFieldOrGlobal(v, _) + ) + ) + ) +} + +predicate fcloseCallOrIndirect(FunctionCall fc, Variable v) { + // direct fclose call + fcloseCall(fc, v.getAnAccess()) + or + // indirect fclose call + exists(FunctionCall midcall, Function mid, int arg | + fc.getArgument(arg) = v.getAnAccess() and + mayCallFunction(fc, mid) and + midcall.getEnclosingFunction() = mid and + fcloseCallOrIndirect(midcall, mid.getParameter(arg)) + ) +} + +predicate fopenDefinition(StackVariable v, ControlFlowNode def) { + exists(Expr expr | exprDefinition(v, def, expr) and fopenCallOrIndirect(expr)) +} + +class FOpenVariableReachability extends StackVariableReachabilityWithReassignment { + FOpenVariableReachability() { this = "FOpenVariableReachability" } + + override predicate isSourceActual(ControlFlowNode node, StackVariable v) { + fopenDefinition(v, node) + } + + override predicate isSinkActual(ControlFlowNode node, StackVariable v) { + // node may be used in fopenReaches + exists(node.(AnalysedExpr).getNullSuccessor(v)) or + fcloseCallOrIndirect(node, v) or + assignedToFieldOrGlobal(v, node) or + // node may be used directly in query + v.getFunction() = node.(ReturnStmt).getEnclosingFunction() + } + + override predicate isBarrier(ControlFlowNode node, StackVariable v) { definitionBarrier(v, node) } +} + +/** + * The value from fopen at `def` is still held in Variable `v` upon entering `node`. + */ +predicate fopenVariableReaches(StackVariable v, ControlFlowNode def, ControlFlowNode node) { + exists(FOpenVariableReachability r | + // reachability + r.reachesTo(def, _, node, v) + or + // accept def node itself + r.isSource(def, v) and + node = def + ) +} + +class FOpenReachability extends StackVariableReachabilityExt { + FOpenReachability() { this = "FOpenReachability" } + + override predicate isSource(ControlFlowNode node, StackVariable v) { fopenDefinition(v, node) } + + override predicate isSink(ControlFlowNode node, StackVariable v) { + v.getFunction() = node.(ReturnStmt).getEnclosingFunction() + } + + override predicate isBarrier( + ControlFlowNode source, ControlFlowNode node, ControlFlowNode next, StackVariable v + ) { + isSource(source, v) and + next = node.getASuccessor() and + // the file (stored in any variable `v0`) opened at `source` is closed or + // assigned to a global at node, or NULL checked on the edge node -> next. + exists(StackVariable v0 | fopenVariableReaches(v0, source, node) | + node.(AnalysedExpr).getNullSuccessor(v0) = next or + fcloseCallOrIndirect(node, v0) or + assignedToFieldOrGlobal(v0, node) + ) + } +} + +/** + * The value returned by fopen `def` has not been closed, confirmed to be null, + * or potentially leaked globally upon reaching `node` (regardless of what variable + * it's still held in, if any). + */ +predicate fopenReaches(ControlFlowNode def, ControlFlowNode node) { + exists(FOpenReachability r | r.reaches(def, _, node)) +} + +predicate assignedToFieldOrGlobal(StackVariable v, Expr e) { + // assigned to anything except a StackVariable + // (typically a field or global, but for example also *ptr = v) + e.(Assignment).getRValue() = v.getAnAccess() and + not e.(Assignment).getLValue().(VariableAccess).getTarget() instanceof StackVariable + or + exists(Expr midExpr, Function mid, int arg | + // indirect assignment + e.(FunctionCall).getArgument(arg) = v.getAnAccess() and + mayCallFunction(e, mid) and + midExpr.getEnclosingFunction() = mid and + assignedToFieldOrGlobal(mid.getParameter(arg), midExpr) + ) + or + // assigned to a field via constructor field initializer + e.(ConstructorFieldInit).getExpr() = v.getAnAccess() +} + +abstract class CloseFileHandleWhenNoLongerNeededSharedSharedQuery extends Query { } + +Query getQuery() { result instanceof CloseFileHandleWhenNoLongerNeededSharedSharedQuery } + +query predicate problems(ControlFlowNode def, string message, Stmt ret, string retMsg) { + not isExcluded(def, getQuery()) and + message = "The file opened here may not be closed at $@." and + retMsg = "this location" and + ( + fopenReaches(def, ret) and + not exists(StackVariable v | + fopenVariableReaches(v, def, ret) and + ret.getAChild*() = v.getAnAccess() + ) + or + opened(def) and + not fopenCallMayBeClosed(def) and + ret = def.getControlFlowScope().getEntryPoint() + ) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll new file mode 100644 index 0000000000..de4fb70610 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll @@ -0,0 +1,198 @@ +/** + * Provides a library which includes a `problems` predicate for reporting + * memory allocations which are allocated but not freed before they go out of scope. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import semmle.code.cpp.controlflow.StackVariableReachability +import codingstandards.cpp.Allocations +import semmle.code.cpp.pointsto.PointsTo + +predicate allocated(FunctionCall fc) { allocExpr(fc, _) } + +/** Holds if there exists a call to a function that might free the allocation specified by `e`. */ +predicate freed(Expr e) { + freeExpr(_, e, _) or + exists(ExprCall c | + // cautiously assume that any ExprCall could be a call to free. + c.getAnArgument() = e + ) +} + +/** An expression for which there exists a function call that might free it. */ +class FreedExpr extends PointsToExpr { + FreedExpr() { freed(this) } + + override predicate interesting() { freed(this) } +} + +/** + * Holds if `fc` is a call to a function that allocates memory that might be freed. + */ +predicate mallocCallMayBeFreed(FunctionCall fc) { allocated(fc) and anythingPointsTo(fc) } + +/** + * 'call' is either a direct call to f, or a possible call to f + * via a function pointer. + */ +predicate mayCallFunction(Expr call, Function f) { + call.(FunctionCall).getTarget() = f or + call.(VariableCall).getVariable().getAnAssignedValue().getAChild*().(FunctionAccess).getTarget() = + f +} + +predicate allocCallOrIndirect(Expr e) { + // direct memory allocation call + allocated(e) and + // We are only interested in memory allocation calls that are + // actually freed somehow, as MemoryNeverFreed + // will catch those that aren't. + mallocCallMayBeFreed(e) + or + exists(ReturnStmt rtn | + // indirect memory allocation call + mayCallFunction(e, rtn.getEnclosingFunction()) and + ( + // return memory allocation + allocCallOrIndirect(rtn.getExpr()) + or + // return variable assigned with allocated memory + exists(Variable v | + v = rtn.getExpr().(VariableAccess).getTarget() and + allocCallOrIndirect(v.getAnAssignedValue()) and + not assignedToFieldOrGlobal(v, _) + ) + ) + ) +} + +predicate freeCallOrIndirect(FunctionCall fc, Variable v) { + // direct free call + v.getAnAccess() = fc.(DeallocationExpr).getFreedExpr() + or + // indirect free call + exists(FunctionCall midcall, Function mid, int arg | + fc.getArgument(arg) = v.getAnAccess() and + mayCallFunction(fc, mid) and + midcall.getEnclosingFunction() = mid and + freeCallOrIndirect(midcall, mid.getParameter(arg)) + ) +} + +predicate allocDefinition(StackVariable v, ControlFlowNode def) { + exists(Expr expr | exprDefinition(v, def, expr) and allocCallOrIndirect(expr)) +} + +class MallocVariableReachability extends StackVariableReachabilityWithReassignment { + MallocVariableReachability() { this = "MallocVariableReachability" } + + override predicate isSourceActual(ControlFlowNode node, StackVariable v) { + allocDefinition(v, node) + } + + override predicate isSinkActual(ControlFlowNode node, StackVariable v) { + // node may be used in allocReaches + exists(node.(AnalysedExpr).getNullSuccessor(v)) or + freeCallOrIndirect(node, v) or + assignedToFieldOrGlobal(v, node) or + // node may be used directly in query + v.getFunction() = node.(ReturnStmt).getEnclosingFunction() + } + + override predicate isBarrier(ControlFlowNode node, StackVariable v) { definitionBarrier(v, node) } +} + +/** + * The value from malloc at `def` is still held in Variable `v` upon entering `node`. + */ +predicate mallocVariableReaches(StackVariable v, ControlFlowNode def, ControlFlowNode node) { + exists(MallocVariableReachability r | + // reachability + r.reachesTo(def, _, node, v) + or + // accept def node itself + r.isSource(def, v) and + node = def + ) +} + +class MallocReachability extends StackVariableReachabilityExt { + MallocReachability() { this = "MallocReachability" } + + override predicate isSource(ControlFlowNode node, StackVariable v) { allocDefinition(v, node) } + + override predicate isSink(ControlFlowNode node, StackVariable v) { + v.getFunction() = node.(ReturnStmt).getEnclosingFunction() and + // exclude return statements that call a function and pass the pointer as an argument + not exists(Expr arg | + arg = node.(ReturnStmt).getExpr().(FunctionCall).getAnArgument() and + arg = v.getAnAccess() and + not dereferenced(arg) + ) + } + + override predicate isBarrier( + ControlFlowNode source, ControlFlowNode node, ControlFlowNode next, StackVariable v + ) { + isSource(source, v) and + next = node.getASuccessor() and + // the memory (stored in any variable `v0`) allocated at `source` is freed or + // assigned to a global at node, or NULL checked on the edge node -> next. + exists(StackVariable v0 | mallocVariableReaches(v0, source, node) | + node.(AnalysedExpr).getNullSuccessor(v0) = next or + freeCallOrIndirect(node, v0) or + assignedToFieldOrGlobal(v0, node) + ) + } +} + +/** + * The value returned by alloc `def` has not been freed, confirmed to be null, + * or potentially leaked globally upon reaching `node` (regardless of what variable + * it's still held in, if any). + */ +predicate mallocReaches(ControlFlowNode def, ControlFlowNode node) { + exists(MallocReachability r | r.reaches(def, _, node)) +} + +predicate assignedToFieldOrGlobal(StackVariable v, Expr e) { + // assigned to anything except a StackVariable + // (typically a field or global, but for example also *ptr = v) + e.(Assignment).getRValue() = v.getAnAccess() and + not e.(Assignment).getLValue().(VariableAccess).getTarget() instanceof StackVariable + or + exists(Expr midExpr, Function mid, int arg | + // indirect assignment + e.(FunctionCall).getArgument(arg) = v.getAnAccess() and + mayCallFunction(e, mid) and + midExpr.getEnclosingFunction() = mid and + assignedToFieldOrGlobal(mid.getParameter(arg), midExpr) + ) + or + // assigned to a field via constructor field initializer + e.(ConstructorFieldInit).getExpr() = v.getAnAccess() +} + +abstract class FreeMemoryWhenNoLongerNeededSharedSharedQuery extends Query { } + +Query getQuery() { result instanceof FreeMemoryWhenNoLongerNeededSharedSharedQuery } + +// note: this query is based on CloseFileHandleWhenNoLongerNeededShared.qll +query predicate problems(ControlFlowNode def, string message, Stmt ret, string retMsg) { + not isExcluded(def, getQuery()) and + message = "The memory allocated here may not be freed at $@." and + retMsg = "this location" and + ( + mallocReaches(def, ret) and + not exists(StackVariable v | + mallocVariableReaches(v, def, ret) and + ret.getAChild*() = v.getAnAccess() + ) + or + allocated(def) and + not mallocCallMayBeFreed(def) and + ret = def.getControlFlowScope().getEntryPoint() + ) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.qll b/cpp/common/src/codingstandards/cpp/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.qll new file mode 100644 index 0000000000..8017308fda --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.qll @@ -0,0 +1,27 @@ +/** + * Provides a library which includes a `problems` predicate for reporting + * instances of memcmp being used to access bits of an object representation + * that are not part of the object's value representation. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import semmle.code.cpp.padding.Padding +import semmle.code.cpp.security.BufferAccess + +abstract class MemcmpUsedToComparePaddingDataSharedQuery extends Query { } + +Query getQuery() { result instanceof MemcmpUsedToComparePaddingDataSharedQuery } + +query predicate problems(MemcmpBA cmp, string message) { + not isExcluded(cmp, getQuery()) and + cmp.getBuffer(_, _) + .getUnconverted() + .getUnspecifiedType() + .(PointerType) + .getBaseType() + .getUnspecifiedType() instanceof PaddedType and + message = + cmp.getName() + " accesses bits which are not part of the object's value representation." +} diff --git a/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll b/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll new file mode 100644 index 0000000000..44c8d8147f --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll @@ -0,0 +1,127 @@ +/** + * Provides a library which includes a `problems` predicate for reporting memory + * that is not allocated dynamically being subsequently freed via a call to `free`. + */ + +import cpp +import codingstandards.cpp.Customizations +import codingstandards.cpp.Exclusions +import codingstandards.cpp.Allocations +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.dataflow.DataFlow2 + +/** + * A pointer to potentially dynamically allocated memory + */ +class AllocExprSource extends DataFlow::Node { + AllocExprSource() { + allocExprOrIndirect(this.asExpr(), _) + or + // additionally include calls to library functions or output parameters + // to heuristically reduce false-positives from library functions that + // might provide pointers to dynamically allocated memory + exists(FunctionCall fc | + not exists(fc.getTarget().getBlock()) and + ( + this.asExpr() = fc or + this.asDefiningArgument() = fc.getAnArgument() + ) + ) + } +} + +/** + * An argument to a call to `free` or `realloc`. + */ +class FreeExprSink extends DataFlow::Node { + FreeExprSink() { freeExpr(_, this.asExpr(), "free") } +} + +/** + * A data-flow configuration that tracks flow from an `AllocExprSource` to + * the value assigned to a variable. + */ +class AllocExprSourceToAssignedValueConfig extends DataFlow2::Configuration { + AllocExprSourceToAssignedValueConfig() { this = "AllocExprSourceToAssignedValueConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource } + + override predicate isSink(DataFlow::Node sink) { + sink.asExpr() = any(Variable v).getAnAssignedValue() + } +} + +/** + * An assignment of a value that is not a dynamically allocated pointer to a variable. + */ +class NonDynamicallyAllocatedVariableAssignment extends DataFlow::Node { + NonDynamicallyAllocatedVariableAssignment() { + exists(Variable v | + this.asExpr() = v.getAnAssignedValue() and + not this.asExpr() instanceof NullValue and + not any(AllocExprSourceToAssignedValueConfig cfg).hasFlowTo(this) + ) + } +} + +/** + * A data-flow configuration that tracks flow from an `AllocExprSource` to a `FreeExprSink`. + */ +class DynamicMemoryAllocationToFreeConfig extends DataFlow::Configuration { + DynamicMemoryAllocationToFreeConfig() { this = "DynamicMemoryAllocationToFreeConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink } +} + +/** + * A data-flow configuration that tracks flow from a + * `NonDynamicallyAllocatedVariableAssignment` to a `FreeExprSink`. + */ +class NonDynamicPointerToFreeConfig extends DataFlow::Configuration { + NonDynamicPointerToFreeConfig() { this = "NonDynamicPointerToFreeConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof NonDynamicallyAllocatedVariableAssignment + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink } + + override predicate isBarrierOut(DataFlow::Node node) { + // the default interprocedural data-flow model flows through any field or array assignment + // expressionsto the qualifier (array base, pointer dereferenced, or qualifier) instead of the + // individual element or field that the assignment modifies. this default behaviour causes + // false positives for future frees of the object base, so we remove the edges + // between those assignments from the graph with `isBarrierOut`. + exists(AssignExpr a | + node.asExpr() = a.getRValue() and + ( + a.getLValue() instanceof ArrayExpr or + a.getLValue() instanceof PointerDereferenceExpr or + a.getLValue() instanceof FieldAccess + ) + ) + } +} + +abstract class OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery extends Query { } + +Query getQuery() { result instanceof OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery } + +query predicate problems( + FreeExprSink free, string message, DataFlow::Node source, string sourceDescription +) { + not isExcluded(free.asExpr(), getQuery()) and + ( + not any(DynamicMemoryAllocationToFreeConfig cfg).hasFlowTo(free) and + not any(NonDynamicPointerToFreeConfig cfg).hasFlowTo(free) and + message = "Free expression frees non-dynamically allocated memory." and + source = free and + sourceDescription = "" + or + any(NonDynamicPointerToFreeConfig cfg).hasFlow(source, free) and + message = "Free expression frees $@ which was not dynamically allocated." and + sourceDescription = "memory" + ) +} diff --git a/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected new file mode 100644 index 0000000000..b6f56a20a5 --- /dev/null +++ b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.expected @@ -0,0 +1 @@ +| test.cpp:23:8:23:18 | call to memcmp | memcmp accesses bits which are not part of the object's value representation. | diff --git a/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql new file mode 100644 index 0000000000..f924c33f1d --- /dev/null +++ b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/MemcmpUsedToComparePaddingData.ql @@ -0,0 +1,2 @@ +// GENERATED FILE - DO NOT MODIFY +import codingstandards.cpp.rules.memcmpusedtocomparepaddingdata.MemcmpUsedToComparePaddingData diff --git a/cpp/common/test/rules/memcmpusedtocomparepaddingdata/test.cpp b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/test.cpp new file mode 100644 index 0000000000..9f0ba8cab7 --- /dev/null +++ b/cpp/common/test/rules/memcmpusedtocomparepaddingdata/test.cpp @@ -0,0 +1,30 @@ +#include + +struct S1 { + unsigned char buffType; + int size; + + friend bool operator==(const S1 &lhs, const S1 &rhs) { + return lhs.buffType == rhs.buffType && lhs.size == rhs.size; + } +}; + +struct S2 { + unsigned char buff[16]; +}; + +void f(const S1 &s1, const S1 &s2) { + if (s1 == s2) { + // COMPLIANT S overloads operator==() to perform a comparison of the value + // representation of the object + } +} +void f1(const S1 &s1, const S1 &s2) { + if (!std::memcmp(&s1, &s2, sizeof(S1))) { // NON_COMPLIANT + } +} + +void f2(const S2 &s1, const S2 &s2) { + if (!std::memcmp(&s1.buff, &s2.buff, sizeof(S2::buff))) { // COMPLIANT + } +} \ No newline at end of file diff --git a/rule_packages/c/IO1.json b/rule_packages/c/IO1.json index 7d7ae66645..1d90c6f28f 100644 --- a/rule_packages/c/IO1.json +++ b/rule_packages/c/IO1.json @@ -94,6 +94,7 @@ "precision": "very-high", "severity": "error", "short_name": "CloseFilesWhenTheyAreNoLongerNeeded", + "shared_implementation_short_name": "CloseFileHandleWhenNoLongerNeededShared", "tags": [ "correctness", "security" diff --git a/rule_packages/c/Memory2.json b/rule_packages/c/Memory2.json new file mode 100644 index 0000000000..23935197c6 --- /dev/null +++ b/rule_packages/c/Memory2.json @@ -0,0 +1,214 @@ +{ + "CERT-C": { + "ARR36-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Subtraction between pointers referring to differing arrays results in undefined behavior.", + "kind": "problem", + "name": "Do not subtract two pointers that do not refer to the same array", + "precision": "high", + "severity": "warning", + "short_name": "DoNotSubtractPointersThatDoNotReferToTheSameArray", + "shared_implementation_short_name": "DoNotSubtractPointersAddressingDifferentArrays", + "tags": [ + "correctness" + ] + }, + { + "description": "Comparison using the >, >=, <, and <= operators between pointers referring to differing arrays results in undefined behavior.", + "kind": "problem", + "name": "Do not subtract two pointers that do not refer to the same array", + "precision": "high", + "severity": "warning", + "short_name": "DoNotRelatePointersThatDoNotReferToTheSameArray", + "shared_implementation_short_name": "DoNotUseRelationalOperatorsWithDifferingArrays", + "tags": [ + "correctness" + ] + } + ], + "title": "Do not subtract or compare two pointers that do not refer to the same array" + }, + "EXP42-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Padding data values are unspecified and should not be included in comparisons.", + "kind": "problem", + "name": "Do not compare padding data", + "precision": "very-high", + "severity": "error", + "short_name": "DoNotComparePaddingData", + "shared_implementation_short_name": "MemcmpUsedToComparePaddingData", + "tags": [ + "correctness" + ] + } + ], + "title": "Do not compare padding data" + }, + "MEM31-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Failing to free memory that is no longer needed can lead to a memory leak and resource exhaustion.", + "kind": "problem", + "name": "Free dynamically allocated memory when no longer needed", + "precision": "very-high", + "severity": "error", + "short_name": "FreeMemoryWhenNoLongerNeededCert", + "shared_implementation_short_name": "FreeMemoryWhenNoLongerNeededShared", + "tags": [ + "correctness", + "security" + ], + "implementation_scope": { + "description": "The rule is enforced in the context of a single function." + } + } + ], + "title": "Free dynamically allocated memory when no longer needed" + }, + "MEM33-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "A structure containing a flexible array member must be allocated dynamically in order for subsequent accesses to the flexible array to point to valid memory.", + "kind": "problem", + "name": "Allocate structures containing a flexible array member dynamically", + "precision": "very-high", + "severity": "error", + "short_name": "AllocStructsWithAFlexibleArrayMemberDynamically", + "tags": [ + "correctness" + ] + }, + { + "description": "Copying a structure containing a flexbile array member by assignment ignores the flexible array member data.", + "kind": "problem", + "name": "Copy structures containing a flexible array member using memcpy or a similar function.", + "precision": "very-high", + "severity": "error", + "short_name": "CopyStructsWithAFlexibleArrayMemberDynamically", + "tags": [ + "correctness" + ] + } + ], + "title": "Allocate and copy structures containing a flexible array member dynamically" + }, + "MEM34-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.", + "kind": "problem", + "name": "Only free memory allocated dynamically", + "precision": "high", + "severity": "error", + "short_name": "OnlyFreeMemoryAllocatedDynamicallyCert", + "shared_implementation_short_name": "OnlyFreeMemoryAllocatedDynamicallyShared", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Only free memory allocated dynamically" + }, + "MEM36-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Realloc does not preserve the alignment of memory allocated with aligned_alloc and can result in undefined behavior if reallocating more strictly aligned memory.", + "kind": "path-problem", + "name": "Do not modify the alignment of objects by calling realloc", + "precision": "high", + "severity": "error", + "short_name": "DoNotModifyAlignmentOfMemoryWithRealloc", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Do not modify the alignment of objects by calling realloc" + } + }, + "MISRA-C-2012": { + "RULE-22-1": { + "properties": { + "obligation": "required" + }, + "queries": [ + { + "description": "Memory allocated dynamically with standard library functions should be freed to avoid memory leaks.", + "kind": "problem", + "name": "Memory allocated dynamically with Standard Library functions shall be explicitly released", + "precision": "very-high", + "severity": "error", + "short_name": "FreeMemoryWhenNoLongerNeededMisra", + "shared_implementation_short_name": "FreeMemoryWhenNoLongerNeededShared", + "tags": [ + "correctness", + "security" + ], + "implementation_scope": { + "description": "The rule is enforced in the context of a single function." + } + }, + { + "description": "File handles acquired with standard library functions should be released to avoid resource exhaustion.", + "kind": "problem", + "name": "File handles acquired with Standard Library functions shall be explicitly closed", + "precision": "very-high", + "severity": "error", + "short_name": "CloseFileHandleWhenNoLongerNeededMisra", + "shared_implementation_short_name": "CloseFileHandleWhenNoLongerNeededShared", + "tags": [ + "correctness", + "security" + ], + "implementation_scope": { + "description": "The rule is enforced in the context of a single function." + } + } + ], + "title": "All resources obtained dynamically by means of Standard Library functions shall be explicitly released" + }, + "RULE-22-2": { + "properties": { + "obligation": "mandatory" + }, + "queries": [ + { + "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.", + "kind": "problem", + "name": "A block of memory shall only be freed if it was allocated by means of a Standard Library function", + "precision": "high", + "severity": "error", + "short_name": "OnlyFreeMemoryAllocatedDynamicallyMisra", + "shared_implementation_short_name": "OnlyFreeMemoryAllocatedDynamicallyShared", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "A block of memory shall only be freed if it was allocated by means of a Standard Library function" + } + } +} \ No newline at end of file diff --git a/rule_packages/cpp/Representation.json b/rule_packages/cpp/Representation.json index 4428966e87..96674eef0e 100644 --- a/rule_packages/cpp/Representation.json +++ b/rule_packages/cpp/Representation.json @@ -128,6 +128,7 @@ "precision": "very-high", "severity": "error", "short_name": "MemcmpUsedToAccessObjectRepresentation", + "shared_implementation_short_name": "MemcmpUsedToComparePaddingData", "tags": [ "correctness" ] diff --git a/rules.csv b/rules.csv index d9a3863928..c3767997e0 100644 --- a/rules.csv +++ b/rules.csv @@ -559,7 +559,7 @@ c,CERT-C,MEM30-C,Yes,Rule,,,Do not access freed memory,MEM50-CPP,InvalidMemory1, c,CERT-C,MEM31-C,Yes,Rule,,,Free dynamically allocated memory when no longer needed,,Memory2,Very Hard, c,CERT-C,MEM33-C,Yes,Rule,,,Allocate and copy structures containing a flexible array member dynamically,,Memory2,Very Hard, c,CERT-C,MEM34-C,Yes,Rule,,,Only free memory allocated dynamically,,Memory2,Hard, -c,CERT-C,MEM35-C,Yes,Rule,,,Allocate sufficient memory for an object,,Memory2,Very Hard, +c,CERT-C,MEM35-C,Yes,Rule,,,Allocate sufficient memory for an object,,Memory3,Very Hard, c,CERT-C,MEM36-C,Yes,Rule,,,Do not modify the alignment of objects by calling realloc(),,Memory2,Medium, c,CERT-C,MSC30-C,Yes,Rule,,,Do not use the rand() function for generating pseudorandom numbers,MSC50-CPP,Misc,Easy, c,CERT-C,MSC32-C,Yes,Rule,,,Properly seed pseudorandom number generators,MSC51-CPP,Misc,Easy, From fc0f72bf287821ce9a3589c7aec6f861e2fd5082 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 02:51:19 +0100 Subject: [PATCH 02/13] Update RuleMetadata.qll --- .../cpp/exclusions/c/RuleMetadata.qll | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index fc8412b662..f600cd2ead 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -33,11 +33,8 @@ import IO4 import InvalidMemory1 import Language1 import Language2 -<<<<<<< HEAD -import Memory2 -======= import Memory1 ->>>>>>> upstream/main +import Memory2 import Misc import Pointers1 import Pointers2 @@ -88,11 +85,8 @@ newtype TCQuery = TInvalidMemory1PackageQuery(InvalidMemory1Query q) or TLanguage1PackageQuery(Language1Query q) or TLanguage2PackageQuery(Language2Query q) or -<<<<<<< HEAD - TMemory2PackageQuery(Memory2Query q) or -======= TMemory1PackageQuery(Memory1Query q) or ->>>>>>> upstream/main + TMemory2PackageQuery(Memory2Query q) or TMiscPackageQuery(MiscQuery q) or TPointers1PackageQuery(Pointers1Query q) or TPointers2PackageQuery(Pointers2Query q) or @@ -143,11 +137,8 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isInvalidMemory1QueryMetadata(query, queryId, ruleId, category) or isLanguage1QueryMetadata(query, queryId, ruleId, category) or isLanguage2QueryMetadata(query, queryId, ruleId, category) or -<<<<<<< HEAD - isMemory2QueryMetadata(query, queryId, ruleId, category) or -======= isMemory1QueryMetadata(query, queryId, ruleId, category) or ->>>>>>> upstream/main + isMemory2QueryMetadata(query, queryId, ruleId, category) or isMiscQueryMetadata(query, queryId, ruleId, category) or isPointers1QueryMetadata(query, queryId, ruleId, category) or isPointers2QueryMetadata(query, queryId, ruleId, category) or From 91170b4ae95743ec2055a80ca14aa732b1b0efd8 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:02:29 +0100 Subject: [PATCH 03/13] MEM33-C: Remove illegal character from name --- .../MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql | 2 +- rule_packages/c/Memory2.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql index 69f6f9feb9..b4993e2cae 100644 --- a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql +++ b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.ql @@ -1,6 +1,6 @@ /** * @id c/cert/copy-structs-with-a-flexible-array-member-dynamically - * @name MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function. + * @name MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function * @description Copying a structure containing a flexbile array member by assignment ignores the * flexible array member data. * @kind problem diff --git a/rule_packages/c/Memory2.json b/rule_packages/c/Memory2.json index 23935197c6..02d962ac06 100644 --- a/rule_packages/c/Memory2.json +++ b/rule_packages/c/Memory2.json @@ -95,7 +95,7 @@ { "description": "Copying a structure containing a flexbile array member by assignment ignores the flexible array member data.", "kind": "problem", - "name": "Copy structures containing a flexible array member using memcpy or a similar function.", + "name": "Copy structures containing a flexible array member using memcpy or a similar function", "precision": "very-high", "severity": "error", "short_name": "CopyStructsWithAFlexibleArrayMemberDynamically", From d1cb2a1391c3bcaf38efb1686d89b3c8ed37c026 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:04:22 +0100 Subject: [PATCH 04/13] Format Variable.qll --- c/common/src/codingstandards/c/Variable.qll | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/c/common/src/codingstandards/c/Variable.qll b/c/common/src/codingstandards/c/Variable.qll index 6cb18dfb85..adf2f08ad9 100644 --- a/c/common/src/codingstandards/c/Variable.qll +++ b/c/common/src/codingstandards/c/Variable.qll @@ -52,17 +52,14 @@ Variable getAddressOfExprTargetBase(AddressOfExpr expr) { or result = expr.getOperand().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() } - - + /** * A struct that contains a flexible array member */ class FlexibleArrayStructType extends Struct { FlexibleArrayMember member; - FlexibleArrayStructType() { - this = member.getDeclaringType() - } + FlexibleArrayStructType() { this = member.getDeclaringType() } FlexibleArrayMember getFlexibleArrayMember() { result = member } } From ae4ed14bbb110aaa93a4b58d679a201dfa548bf9 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:08:05 +0100 Subject: [PATCH 05/13] Update rules.csv --- rules.csv | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules.csv b/rules.csv index 5d8d816604..68a4ced7d4 100644 --- a/rules.csv +++ b/rules.csv @@ -664,7 +664,7 @@ c,MISRA-C-2012,RULE-9-1,Yes,Mandatory,,,The value of an object with automatic st c,MISRA-C-2012,RULE-9-2,Yes,Required,,,The initializer for an aggregate or union shall be enclosed in braces,,Memory1,Easy, c,MISRA-C-2012,RULE-9-3,Yes,Required,,,Arrays shall not be partially initialized,,Memory1,Medium, c,MISRA-C-2012,RULE-9-4,Yes,Required,,,An element of an object shall not be initialized more than once,,Memory1,Medium, -c,MISRA-C-2012,RULE-9-5,Yes,Required,,,Where designated initializers are used to initialize an array object the size of the array shall be specified explicitly,,Memory2,Medium, +c,MISRA-C-2012,RULE-9-5,No,Required,,,Where designated initializers are used to initialize an array object the size of the array shall be specified explicitly,,Memory2,Medium, c,MISRA-C-2012,RULE-10-1,Yes,Required,,,Operands shall not be of an inappropriate essential type,,EssentialTypes,Hard, c,MISRA-C-2012,RULE-10-2,Yes,Required,,,Expressions of essentially character type shall not be used inappropriately in addition and subtraction operations,,EssentialTypes,Medium, c,MISRA-C-2012,RULE-10-3,Yes,Required,,,The value of an expression shall not be assigned to an object with a narrower essential type or of a different essential type category,,EssentialTypes,Hard, @@ -759,7 +759,7 @@ c,MISRA-C-2012,RULE-21-13,Yes,Mandatory,,,Any value passed to a function in shall not result in accesses beyond the bounds of the objects referenced by their pointer parameters,,Memory2,Hard, +c,MISRA-C-2012,RULE-21-17,Yes,Mandatory,,,Use of the string handling functions from shall not result in accesses beyond the bounds of the objects referenced by their pointer parameters,,OutOfBounds,Hard, c,MISRA-C-2012,RULE-21-18,Yes,Mandatory,,,The size_t argument passed to any function in shall have an appropriate value,,OutOfBounds,Hard, c,MISRA-C-2012,RULE-21-19,Yes,Mandatory,,,"The pointers returned by the Standard Library functions localeconv, getenv, setlocale or, strerror shall only be used as if they have pointer to const-qualified type",ENV30-C,Contracts2,Medium, c,MISRA-C-2012,RULE-21-20,Yes,Mandatory,,,"The pointer returned by the Standard Library functions asctime, ctime, gmtime, localtime, localeconv, getenv, setlocale or strerror shall not be used following a subsequent call to the same function","ENV34-C",Contracts2,Import, From 0c2a7061900df7086bd8b551ad6f1649508f789b Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:08:20 +0100 Subject: [PATCH 06/13] EXP62-CPP: Generate help files --- .../MemcmpUsedToAccessObjectRepresentation.md | 150 +++++++++++++++++- .../MemcpyUsedToAccessObjectRepresentation.md | 2 +- .../MemsetUsedToAccessObjectRepresentation.md | 2 +- 3 files changed, 150 insertions(+), 4 deletions(-) diff --git a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md index 44380a3b49..165436a126 100644 --- a/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md +++ b/cpp/cert/src/rules/EXP62-CPP/MemcmpUsedToAccessObjectRepresentation.md @@ -3,9 +3,155 @@ This query implements the CERT-C++ rule EXP62-CPP: > Do not access the bits of an object representation that are not part of the object's value representation -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Description + +The C++ Standard, \[basic.types\], paragraph 9 \[[ISO/IEC 14882-2014](https://wiki.sei.cmu.edu/confluence/display/cplusplus/AA.+Bibliography#AA.Bibliography-ISO%2FIEC14882-2014)\], states the following: + +> The *object representation* of an object of type `T` is the sequence of *N* `unsigned char` objects taken up by the object of type `T`, where *N* equals `sizeof(T)`. The *value representation* of an object is the set of bits that hold the value of type `T`. + + +The narrow character types (`char`, `signed char`, and `unsigned char`)—as well as some other integral types on specific platforms—have an object representation that consists solely of the bits from the object's value representation. For such types, accessing any of the bits of the value representation is well-defined behavior. This form of object representation allows a programmer to access and modify an object solely based on its bit representation, such as by calling `std::memcmp()` on its object representation. + +Other types, such as classes, may not have an object representation composed solely of the bits from the object's value representation. For instance, classes may have bit-field data members, padding inserted between data members, a [vtable](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vtable) to support virtual method dispatch, or data members declared with different access privileges. For such types, accessing bits of the object representation that are not part of the object's value representation may result in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-undefinedbehavior) depending on how those bits are accessed. + +Do not access the bits of an object representation that are not part of the object's value representation. Even if the bits are accessed in a well-defined manner, such as through an array of `unsigned char` objects, the values represented by those bits are unspecified or implementation-defined, and reliance on any particular value can lead to abnormal program execution. + +## Noncompliant Code Example + +In this noncompliant code example, the complete object representation is accessed when comparing two objects of type `S`. Per the C++ Standard, \[class\], paragraph 13 \[[ISO/IEC 14882-2014](https://wiki.sei.cmu.edu/confluence/display/cplusplus/AA.+Bibliography#AA.Bibliography-ISO%2FIEC14882-2014)\], classes may be padded with data to ensure that they are properly aligned in memory. The contents of the padding and the amount of padding added is [implementation-defined](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation-definedbehavior). This can lead to incorrect results when comparing the object representation of classes instead of the value representation, as the padding may assume different [unspecified values](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-unspecifiedvalue) for each object instance. + +```cpp +#include + +struct S { + unsigned char buffType; + int size; +}; + +void f(const S &s1, const S &s2) { + if (!std::memcmp(&s1, &s2, sizeof(S))) { + // ... + } +} +``` + +## Compliant Solution + +In this compliant solution, `S` overloads `operator==()` to perform a comparison of the value representation of the object. + +```cpp +struct S { + unsigned char buffType; + int size; + + friend bool operator==(const S &lhs, const S &rhs) { + return lhs.buffType == rhs.buffType && + lhs.size == rhs.size; + } +}; + +void f(const S &s1, const S &s2) { + if (s1 == s2) { + // ... + } +} +``` + +## Noncompliant Code Example + +In this noncompliant code example, `std::memset()` is used to clear the internal state of an object. An [implementation](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation) may store a vtable within the object instance due to the presence of a virtual function, and that vtable is subsequently overwritten by the call to `std::memset()`, leading to [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-undefinedbehavior) when virtual method dispatch is required. + +```cpp +#include + +struct S { + int i, j, k; + + // ... + + virtual void f(); +}; + +void f() { + S *s = new S; + // ... + std::memset(s, 0, sizeof(S)); + // ... + s->f(); // undefined behavior +} +``` + +## Compliant Solution + +In this compliant solution, the data members of `S` are cleared explicitly instead of calling `std::memset().` + +```cpp +struct S { + int i, j, k; + + // ... + + virtual void f(); + void clear() { i = j = k = 0; } +}; + +void f() { + S *s = new S; + // ... + s->clear(); + // ... + s->f(); // ok +} +``` + +## Exceptions + +**EXP62-CPP-EX1:** It is permissible to access the bits of an object representation when that access is otherwise unobservable in well-defined code. Specifically, reading bits that are not part of the value representation is permissible when there is no reliance or assumptions placed on their values, and writing bits that are not part of the value representation is only permissible when those bits are padding bits. This exception does not permit writing to bits that are part of the object representation aside from padding bits, such as overwriting a vtable pointer. + +For instance, it is acceptable to call `std::memcpy()` on an object containing a bit-field, as in the following example, because the read and write of the padding bits cannot be observed. + +```cpp +#include + +struct S { + int i : 10; + int j; +}; + +void f(const S &s1) { + S s2; + std::memcpy(&s2, &s1, sizeof(S)); +} +``` +Code that complies with this exception must still comply with [OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions](https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions). + +## Risk Assessment + +The effects of accessing bits of an object representation that are not part of the object's value representation can range from [implementation-defined behavior](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-implementation-definedbehavior) (such as assuming the layout of fields with differing access controls) to code execution [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vulnerability) (such as overwriting the vtable pointer). + +
Rule Severity Likelihood Remediation Cost Priority Level
EXP62-CPP High Probable High P6 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.2p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.4 DF4726, DF4727, DF4728, DF4729, DF4731, DF4732, DF4733, DF4734
Klocwork 2022.4 CERT.MEMCMP.PADDED_DATA CWARN.MEM.NONPOD
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.2 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
Polyspace Bug Finder R2023a CERT C++: EXP62-CPP Checks for access attempts on padding and vtable bits (rule fully covered).
PVS-Studio 7.23 V598 , V780, V1084
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/cplusplus/BB.+Definitions#BB.Definitions-vulnerabil) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+EXP62-CPP). + +## Related Guidelines + +
SEI CERT C++ Coding Standard OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions
+ + +## Bibliography + +
\[ ISO/IEC 14882-2014 \] Subclause 3.9, "Types" Subclause 3.10, "Lvalues and Rvalues" Clause 9, "Classes"
+ ## Implementation notes diff --git a/cpp/cert/src/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.md b/cpp/cert/src/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.md index 1dd80f651c..3301a7eacb 100644 --- a/cpp/cert/src/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.md +++ b/cpp/cert/src/rules/EXP62-CPP/MemcpyUsedToAccessObjectRepresentation.md @@ -136,7 +136,7 @@ The effects of accessing bits of an object representation that are not part of t ## Automated Detection -
Tool Version Checker Description
Astrée 20.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.0p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.2 C++4726, C++4727, C++4728, C++4729, C++4731, C++4732, C++4733, C++4734
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.1 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
PVS-Studio 7.19 V598 , V780, V1084
+
Tool Version Checker Description
Astrée 22.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.2p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.4 DF4726, DF4727, DF4728, DF4729, DF4731, DF4732, DF4733, DF4734
Klocwork 2022.4 CERT.MEMCMP.PADDED_DATA CWARN.MEM.NONPOD
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.2 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
Polyspace Bug Finder R2023a CERT C++: EXP62-CPP Checks for access attempts on padding and vtable bits (rule fully covered).
PVS-Studio 7.23 V598 , V780, V1084
## Related Vulnerabilities diff --git a/cpp/cert/src/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.md b/cpp/cert/src/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.md index 5f37a5f449..77874d3110 100644 --- a/cpp/cert/src/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.md +++ b/cpp/cert/src/rules/EXP62-CPP/MemsetUsedToAccessObjectRepresentation.md @@ -136,7 +136,7 @@ The effects of accessing bits of an object representation that are not part of t ## Automated Detection -
Tool Version Checker Description
Astrée 20.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.0p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.2 C++4726, C++4727, C++4728, C++4729, C++4731, C++4732, C++4733, C++4734
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.1 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
PVS-Studio 7.19 V598 , V780, V1084
+
Tool Version Checker Description
Astrée 22.10 invalid_pointer_dereferenceuninitialized_variable_use
CodeSonar 7.2p0 BADFUNC.MEMCMP BADFUNC.MEMSET Use of memcmp Use of memset
Helix QAC 2022.4 DF4726, DF4727, DF4728, DF4729, DF4731, DF4732, DF4733, DF4734
Klocwork 2022.4 CERT.MEMCMP.PADDED_DATA CWARN.MEM.NONPOD
LDRA tool suite 618 S Partially implemented
Parasoft C/C++test 2022.2 CERT_CPP-EXP62-a Do not compare objects of a class that may contain padding bits with C standard library functions
Polyspace Bug Finder R2023a CERT C++: EXP62-CPP Checks for access attempts on padding and vtable bits (rule fully covered).
PVS-Studio 7.23 V598 , V780, V1084
## Related Vulnerabilities From bfa08639c05da631d689c331507f63d409a33cde Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:14:39 +0100 Subject: [PATCH 07/13] Remove RULE-9-5 from Memory2 package --- rules.csv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules.csv b/rules.csv index 68a4ced7d4..7947930f4b 100644 --- a/rules.csv +++ b/rules.csv @@ -664,7 +664,7 @@ c,MISRA-C-2012,RULE-9-1,Yes,Mandatory,,,The value of an object with automatic st c,MISRA-C-2012,RULE-9-2,Yes,Required,,,The initializer for an aggregate or union shall be enclosed in braces,,Memory1,Easy, c,MISRA-C-2012,RULE-9-3,Yes,Required,,,Arrays shall not be partially initialized,,Memory1,Medium, c,MISRA-C-2012,RULE-9-4,Yes,Required,,,An element of an object shall not be initialized more than once,,Memory1,Medium, -c,MISRA-C-2012,RULE-9-5,No,Required,,,Where designated initializers are used to initialize an array object the size of the array shall be specified explicitly,,Memory2,Medium, +c,MISRA-C-2012,RULE-9-5,No,Required,,,Where designated initializers are used to initialize an array object the size of the array shall be specified explicitly,,,Medium, c,MISRA-C-2012,RULE-10-1,Yes,Required,,,Operands shall not be of an inappropriate essential type,,EssentialTypes,Hard, c,MISRA-C-2012,RULE-10-2,Yes,Required,,,Expressions of essentially character type shall not be used inappropriately in addition and subtraction operations,,EssentialTypes,Medium, c,MISRA-C-2012,RULE-10-3,Yes,Required,,,The value of an expression shall not be assigned to an object with a narrower essential type or of a different essential type category,,EssentialTypes,Hard, From 2b1ebfec78521892a1f3137fbdec669f6ef10062 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 03:14:53 +0100 Subject: [PATCH 08/13] Remove duplicate MaxAlignT code --- .../DoNotModifyAlignmentOfMemoryWithRealloc.ql | 2 +- .../src/codingstandards/cpp/Allocations.qll | 18 ------------------ 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql index 79a337f036..48279993d5 100644 --- a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql +++ b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.ql @@ -14,7 +14,7 @@ import cpp import codingstandards.c.cert -import codingstandards.cpp.Allocations +import codingstandards.cpp.Alignment import semmle.code.cpp.dataflow.DataFlow import DataFlow::PathGraph diff --git a/cpp/common/src/codingstandards/cpp/Allocations.qll b/cpp/common/src/codingstandards/cpp/Allocations.qll index f0523d2d0b..db47b0b028 100644 --- a/cpp/common/src/codingstandards/cpp/Allocations.qll +++ b/cpp/common/src/codingstandards/cpp/Allocations.qll @@ -168,21 +168,3 @@ predicate freeExprOrIndirect(Expr free, Expr freed, string kind) { free.(FunctionCall).getArgument(arg) = freed ) } - -class MaxAlignT extends TypedefType { - MaxAlignT() { getName() = "max_align_t" } -} - -/** - * Gets the alignment for `max_align_t`, assuming there is a single consistent alignment for the - * database. - * - * In theory, each compilation of each file can have a different `max_align_t` value (for example, - * if the same file is compiled under different compilers in the same database). We don't have the - * fine-grained data to determine which compilation each operator new call is from, so only hold in - * cases where there's a single clear alignment for the whole database. - */ -int getGlobalMaxAlignT() { - count(MaxAlignT m | | m.getAlignment()) = 1 and - result = any(MaxAlignT t).getAlignment() -} From 922d0e514bd805717010448e83ad837e587ba63a Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 11:55:07 +0100 Subject: [PATCH 09/13] Revert CloseFileHandleWhenNoLongerNeededShared.qll change --- .../CloseFileHandleWhenNoLongerNeededShared.qll | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll b/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll index a1df75fe11..3d65ea3662 100644 --- a/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/closefilehandlewhennolongerneededshared/CloseFileHandleWhenNoLongerNeededShared.qll @@ -10,6 +10,14 @@ import semmle.code.cpp.controlflow.StackVariableReachability import codingstandards.cpp.standardlibrary.FileAccess import codingstandards.cpp.Allocations +/** + * Extend the NullValue class used by Nullness.qll to include simple -1 as a 'null' value + * (for example 'open' returns -1 if there was an error) + */ +class MinusOne extends NullValue { + MinusOne() { this.(UnaryMinusExpr).getOperand().(Literal).getValue() = "1" } +} + /** * 'call' is either a direct call to f, or a possible call to f * via a function pointer. @@ -22,7 +30,7 @@ predicate mayCallFunction(Expr call, Function f) { predicate fopenCallOrIndirect(Expr e) { // direct allocation call - allocExpr(e, _) and + opened(e) and // We are only interested in allocation calls that are // actually freed somehow, as MemoryNeverFreed // will catch those that aren't. From db92ee6bf1a9ded74cfe361e45f210c19d419c65 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Thu, 23 Mar 2023 11:59:09 +0100 Subject: [PATCH 10/13] Regenerate help files --- .../ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md | 1 - .../DoNotSubtractPointersThatDoNotReferToTheSameArray.md | 1 - c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md | 1 - c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md | 1 - .../MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md | 2 +- .../src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md | 1 - .../rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md | 1 - 7 files changed, 1 insertion(+), 7 deletions(-) diff --git a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md index 90d073c18f..320eaa0c05 100644 --- a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md +++ b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule ARR36-C: > Do not subtract or compare two pointers that do not refer to the same array - ## Description When two pointers are subtracted, both must point to elements of the same array object or just one past the last element of the array object (C Standard, 6.5.6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]); the result is the difference of the subscripts of the two array elements. Otherwise, the operation is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48).) diff --git a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md index 90d073c18f..320eaa0c05 100644 --- a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md +++ b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule ARR36-C: > Do not subtract or compare two pointers that do not refer to the same array - ## Description When two pointers are subtracted, both must point to elements of the same array object or just one past the last element of the array object (C Standard, 6.5.6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\]); the result is the difference of the subscripts of the two array elements. Otherwise, the operation is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 48](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_48).) diff --git a/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md index 0bfa1e25fc..297b718852 100644 --- a/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md +++ b/c/cert/src/rules/EXP42-C/DoNotComparePaddingData.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule EXP42-C: > Do not compare padding data - ## Description The C Standard, 6.7.2.1 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states diff --git a/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md index 09a6ce7219..d3c849331a 100644 --- a/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md +++ b/c/cert/src/rules/MEM31-C/FreeMemoryWhenNoLongerNeededCert.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule MEM31-C: > Free dynamically allocated memory when no longer needed - ## Description Before the lifetime of the last pointer that stores the return value of a call to a standard memory allocation function has ended, it must be matched by a call to `free()` with that pointer value. diff --git a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md index 34d1aa6287..3fe0840a96 100644 --- a/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md +++ b/c/cert/src/rules/MEM33-C/CopyStructsWithAFlexibleArrayMemberDynamically.md @@ -1,4 +1,4 @@ -# MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function. +# MEM33-C: Copy structures containing a flexible array member using memcpy or a similar function This query implements the CERT-C rule MEM33-C: diff --git a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md index c5772adb4b..c6fa7eb298 100644 --- a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md +++ b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule MEM34-C: > Only free memory allocated dynamically - ## Description The C Standard, Annex J \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states that the behavior of a program is [undefined ](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior) when diff --git a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md index aca1b78530..51cf1b2179 100644 --- a/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md +++ b/c/cert/src/rules/MEM36-C/DoNotModifyAlignmentOfMemoryWithRealloc.md @@ -5,7 +5,6 @@ This query implements the CERT-C rule MEM36-C: > Do not modify the alignment of objects by calling realloc - ## Description Do not invoke `realloc()` to modify the size of allocated objects that have stricter alignment requirements than those guaranteed by `malloc()`. Storage allocated by a call to the standard `aligned_alloc()` function, for example, can have stricter than normal alignment requirements. The C standard requires only that a pointer returned by `realloc()` be suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement. From 47eaf7cf816ae9ad7cb85e29fc1e4804b43ecb60 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Tue, 28 Mar 2023 19:52:29 +0200 Subject: [PATCH 11/13] ARR36-C: Change kind to path-problem --- .../DoNotRelatePointersThatDoNotReferToTheSameArray.ql | 2 +- .../DoNotSubtractPointersThatDoNotReferToTheSameArray.ql | 2 +- rule_packages/c/Memory2.json | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql index 5b346c02dd..b0cd3200f1 100644 --- a/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql +++ b/c/cert/src/rules/ARR36-C/DoNotRelatePointersThatDoNotReferToTheSameArray.ql @@ -3,7 +3,7 @@ * @name ARR36-C: Do not subtract two pointers that do not refer to the same array * @description Comparison using the >, >=, <, and <= operators between pointers referring to * differing arrays results in undefined behavior. - * @kind problem + * @kind path-problem * @precision high * @problem.severity warning * @tags external/cert/id/arr36-c diff --git a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql index 15e1148b53..d62c3eda5a 100644 --- a/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql +++ b/c/cert/src/rules/ARR36-C/DoNotSubtractPointersThatDoNotReferToTheSameArray.ql @@ -3,7 +3,7 @@ * @name ARR36-C: Do not subtract two pointers that do not refer to the same array * @description Subtraction between pointers referring to differing arrays results in undefined * behavior. - * @kind problem + * @kind path-problem * @precision high * @problem.severity warning * @tags external/cert/id/arr36-c diff --git a/rule_packages/c/Memory2.json b/rule_packages/c/Memory2.json index 02d962ac06..ac77720028 100644 --- a/rule_packages/c/Memory2.json +++ b/rule_packages/c/Memory2.json @@ -7,7 +7,7 @@ "queries": [ { "description": "Subtraction between pointers referring to differing arrays results in undefined behavior.", - "kind": "problem", + "kind": "path-problem", "name": "Do not subtract two pointers that do not refer to the same array", "precision": "high", "severity": "warning", @@ -19,7 +19,7 @@ }, { "description": "Comparison using the >, >=, <, and <= operators between pointers referring to differing arrays results in undefined behavior.", - "kind": "problem", + "kind": "path-problem", "name": "Do not subtract two pointers that do not refer to the same array", "precision": "high", "severity": "warning", From e073f4ab4b527158bd9ea81c750eab3ac4781b03 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Tue, 28 Mar 2023 19:54:23 +0200 Subject: [PATCH 12/13] MEM34-C and RULE-22-2: Reduce FPs The query implementation is now a path-problem and only outputs results for flow from address-of expressions and global variable accesses that do not have allocation expressions assigned to them. --- .../OnlyFreeMemoryAllocatedDynamicallyCert.ql | 2 +- ...eMemoryAllocatedDynamicallyShared.expected | 30 ++++-- .../test.c | 11 ++- ...OnlyFreeMemoryAllocatedDynamicallyMisra.ql | 2 +- .../FreeMemoryWhenNoLongerNeededShared.qll | 13 +-- ...lyFreeMemoryAllocatedDynamicallyShared.qll | 91 +++++++++++-------- rule_packages/c/Memory2.json | 4 +- 7 files changed, 89 insertions(+), 64 deletions(-) diff --git a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql index 3ff7564fc9..a51effec5a 100644 --- a/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql +++ b/c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql @@ -3,7 +3,7 @@ * @name MEM34-C: Only free memory allocated dynamically * @description Freeing memory that is not allocated dynamically can lead to heap corruption and * undefined behavior. - * @kind problem + * @kind path-problem * @precision high * @problem.severity error * @tags external/cert/id/mem34-c diff --git a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected index 84b0cb0ba3..5881d5e78f 100644 --- a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected +++ b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected @@ -1,6 +1,24 @@ -| test.c:8:8:8:10 | g_p | Free expression frees non-dynamically allocated memory. | test.c:8:8:8:10 | g_p | | -| test.c:10:8:10:10 | g_p | Free expression frees $@ which was not dynamically allocated. | test.c:9:9:9:12 | & ... | memory | -| test.c:15:33:15:35 | g_p | Free expression frees non-dynamically allocated memory. | test.c:15:33:15:35 | g_p | | -| test.c:17:36:17:38 | ptr | Free expression frees $@ which was not dynamically allocated. | test.c:24:7:24:8 | & ... | memory | -| test.c:23:8:23:8 | p | Free expression frees $@ which was not dynamically allocated. | test.c:22:13:22:14 | & ... | memory | -| test.c:42:10:42:10 | p | Free expression frees non-dynamically allocated memory. | test.c:42:10:42:10 | p | | +problems +| test.c:8:8:8:10 | g_p | test.c:8:8:8:10 | g_p | test.c:8:8:8:10 | g_p | Free expression frees memory which was not dynamically allocated. | +| test.c:10:8:10:10 | g_p | test.c:10:8:10:10 | g_p | test.c:10:8:10:10 | g_p | Free expression frees memory which was not dynamically allocated. | +| test.c:12:8:12:10 | g_p | test.c:12:8:12:10 | g_p | test.c:12:8:12:10 | g_p | Free expression frees memory which was not dynamically allocated. | +| test.c:16:33:16:35 | g_p | test.c:16:33:16:35 | g_p | test.c:16:33:16:35 | g_p | Free expression frees memory which was not dynamically allocated. | +| test.c:18:36:18:38 | ptr | test.c:27:7:27:8 | & ... | test.c:18:36:18:38 | ptr | Free expression frees memory which was not dynamically allocated. | +| test.c:26:8:26:8 | p | test.c:25:13:25:14 | & ... | test.c:26:8:26:8 | p | Free expression frees memory which was not dynamically allocated. | +edges +| test.c:18:24:18:26 | ptr | test.c:18:36:18:38 | ptr | +| test.c:25:13:25:14 | & ... | test.c:26:8:26:8 | p | +| test.c:27:7:27:8 | & ... | test.c:28:15:28:15 | p | +| test.c:28:15:28:15 | p | test.c:18:24:18:26 | ptr | +nodes +| test.c:8:8:8:10 | g_p | semmle.label | g_p | +| test.c:10:8:10:10 | g_p | semmle.label | g_p | +| test.c:12:8:12:10 | g_p | semmle.label | g_p | +| test.c:16:33:16:35 | g_p | semmle.label | g_p | +| test.c:18:24:18:26 | ptr | semmle.label | ptr | +| test.c:18:36:18:38 | ptr | semmle.label | ptr | +| test.c:25:13:25:14 | & ... | semmle.label | & ... | +| test.c:26:8:26:8 | p | semmle.label | p | +| test.c:27:7:27:8 | & ... | semmle.label | & ... | +| test.c:28:15:28:15 | p | semmle.label | p | +subpaths diff --git a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c index 20b39454a1..bfb8899f71 100644 --- a/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c +++ b/c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c @@ -9,24 +9,29 @@ void test_global(void) { g_p = &g_i; free(g_p); // NON_COMPLIANT g_p = malloc(10); - free(g_p); // COMPLIANT - but could be written to in different scope + free(g_p); // COMPLIANT[FALSE_POSITIVE] - but could be written to in different + // scope } void test_global_b(void) { free(g_p); } // NON_COMPLIANT void free_nested(void *ptr) { free(ptr); } // NON_COMPLIANT - some paths +void get_allocated_memory(void **p) { *p = malloc(10); } + void test_local(void) { int i; int j; void *p = &i; free(p); // NON_COMPLIANT p = &j; - free_nested(p); // NON_COMPLIANT + free_nested(p); // NON_COMPLIANT - reported on line 18 p = malloc(10); free(p); // COMPLIANT p = malloc(10); free_nested(p); // COMPLIANT + get_allocated_memory(&p); + free(p); // COMPLIANT } struct S { @@ -39,7 +44,7 @@ void test_local_field_nested(struct S *s) { free(s->p); } // COMPLIANT void test_local_field(void) { struct S s; s.p = &s.i; - free(s.p); // NON_COMPLIANT + free(s.p); // NON_COMPLIANT[FALSE_NEGATIVE] s.p = malloc(10); free(s.p); // COMPLIANT s.p = malloc(10); diff --git a/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql b/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql index 9293ebe716..ee14d443d2 100644 --- a/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql +++ b/c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql @@ -3,7 +3,7 @@ * @name RULE-22-2: A block of memory shall only be freed if it was allocated by means of a Standard Library function * @description Freeing memory that is not allocated dynamically can lead to heap corruption and * undefined behavior. - * @kind problem + * @kind path-problem * @precision high * @problem.severity error * @tags external/misra/id/rule-22-2 diff --git a/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll index de4fb70610..78ef0e228a 100644 --- a/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll @@ -12,20 +12,11 @@ import semmle.code.cpp.pointsto.PointsTo predicate allocated(FunctionCall fc) { allocExpr(fc, _) } -/** Holds if there exists a call to a function that might free the allocation specified by `e`. */ -predicate freed(Expr e) { - freeExpr(_, e, _) or - exists(ExprCall c | - // cautiously assume that any ExprCall could be a call to free. - c.getAnArgument() = e - ) -} - /** An expression for which there exists a function call that might free it. */ class FreedExpr extends PointsToExpr { - FreedExpr() { freed(this) } + FreedExpr() { freeExprOrIndirect(this, _, _) } - override predicate interesting() { freed(this) } + override predicate interesting() { freeExprOrIndirect(this, _, _) } } /** diff --git a/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll b/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll index 44c8d8147f..9cd3810827 100644 --- a/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll @@ -9,6 +9,7 @@ import codingstandards.cpp.Exclusions import codingstandards.cpp.Allocations import semmle.code.cpp.dataflow.DataFlow import semmle.code.cpp.dataflow.DataFlow2 +import DataFlow::PathGraph /** * A pointer to potentially dynamically allocated memory @@ -38,41 +39,54 @@ class FreeExprSink extends DataFlow::Node { } /** - * A data-flow configuration that tracks flow from an `AllocExprSource` to - * the value assigned to a variable. + * An `Expr` that is an `AddressOfExpr` of a `Variable`. + * + * `Field`s of `PointerType` are not included in order to reduce false-positives, + * as the data-flow library sometimes equates pointers to their underlying data. */ -class AllocExprSourceToAssignedValueConfig extends DataFlow2::Configuration { - AllocExprSourceToAssignedValueConfig() { this = "AllocExprSourceToAssignedValueConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource } - - override predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(Variable v).getAnAssignedValue() - } -} - -/** - * An assignment of a value that is not a dynamically allocated pointer to a variable. - */ -class NonDynamicallyAllocatedVariableAssignment extends DataFlow::Node { - NonDynamicallyAllocatedVariableAssignment() { - exists(Variable v | - this.asExpr() = v.getAnAssignedValue() and - not this.asExpr() instanceof NullValue and - not any(AllocExprSourceToAssignedValueConfig cfg).hasFlowTo(this) - ) +class AddressOfExprSourceNode extends Expr { + AddressOfExprSourceNode() { + exists(VariableAccess va | + this.(AddressOfExpr).getOperand() = va and + ( + va.getTarget() instanceof StackVariable or + va.getTarget() instanceof GlobalVariable or + // allow address-of field, but only if that field is not a pointer type, + // as there may be nested allocations assigned to fields of pointer types. + va.(FieldAccess).getTarget().getUnderlyingType() instanceof ArithmeticType + ) + or + this = va and + exists(GlobalVariable gv | + gv = va.getTarget() and + ( + gv.getUnderlyingType() instanceof ArithmeticType or + not exists(gv.getAnAssignedValue()) or + exists(AddressOfExprSourceNode other | + DataFlow::localExprFlow(other, gv.getAnAssignedValue()) + ) + ) + ) + ) and + // exclude alloc(&allocated_ptr) cases + not any(DynamicMemoryAllocationToAddressOfDefiningArgConfig cfg) + .hasFlowTo(DataFlow::definitionByReferenceNodeFromArgument(this)) } } /** * A data-flow configuration that tracks flow from an `AllocExprSource` to a `FreeExprSink`. */ -class DynamicMemoryAllocationToFreeConfig extends DataFlow::Configuration { - DynamicMemoryAllocationToFreeConfig() { this = "DynamicMemoryAllocationToFreeConfig" } +class DynamicMemoryAllocationToAddressOfDefiningArgConfig extends DataFlow2::Configuration { + DynamicMemoryAllocationToAddressOfDefiningArgConfig() { + this = "DynamicMemoryAllocationToAddressOfDefiningArgConfig" + } override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink } + override predicate isSink(DataFlow::Node sink) { + sink.asDefiningArgument() instanceof AddressOfExpr + } } /** @@ -83,14 +97,14 @@ class NonDynamicPointerToFreeConfig extends DataFlow::Configuration { NonDynamicPointerToFreeConfig() { this = "NonDynamicPointerToFreeConfig" } override predicate isSource(DataFlow::Node source) { - source instanceof NonDynamicallyAllocatedVariableAssignment + source.asExpr() instanceof AddressOfExprSourceNode } override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink } override predicate isBarrierOut(DataFlow::Node node) { // the default interprocedural data-flow model flows through any field or array assignment - // expressionsto the qualifier (array base, pointer dereferenced, or qualifier) instead of the + // expressions to the qualifier (array base, pointer dereferenced, or qualifier) instead of the // individual element or field that the assignment modifies. this default behaviour causes // false positives for future frees of the object base, so we remove the edges // between those assignments from the graph with `isBarrierOut`. @@ -103,6 +117,11 @@ class NonDynamicPointerToFreeConfig extends DataFlow::Configuration { ) ) } + + override predicate isBarrierIn(DataFlow::Node node) { + // only the last source expression is relevant + isSource(node) + } } abstract class OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery extends Query { } @@ -110,18 +129,10 @@ abstract class OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery extends Query Query getQuery() { result instanceof OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery } query predicate problems( - FreeExprSink free, string message, DataFlow::Node source, string sourceDescription + DataFlow::PathNode element, DataFlow::PathNode source, DataFlow::PathNode sink, string message ) { - not isExcluded(free.asExpr(), getQuery()) and - ( - not any(DynamicMemoryAllocationToFreeConfig cfg).hasFlowTo(free) and - not any(NonDynamicPointerToFreeConfig cfg).hasFlowTo(free) and - message = "Free expression frees non-dynamically allocated memory." and - source = free and - sourceDescription = "" - or - any(NonDynamicPointerToFreeConfig cfg).hasFlow(source, free) and - message = "Free expression frees $@ which was not dynamically allocated." and - sourceDescription = "memory" - ) + not isExcluded(element.getNode().asExpr(), getQuery()) and + element = sink and + any(NonDynamicPointerToFreeConfig cfg).hasFlowPath(source, sink) and + message = "Free expression frees memory which was not dynamically allocated." } diff --git a/rule_packages/c/Memory2.json b/rule_packages/c/Memory2.json index ac77720028..677711938a 100644 --- a/rule_packages/c/Memory2.json +++ b/rule_packages/c/Memory2.json @@ -113,7 +113,7 @@ "queries": [ { "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.", - "kind": "problem", + "kind": "path-problem", "name": "Only free memory allocated dynamically", "precision": "high", "severity": "error", @@ -196,7 +196,7 @@ "queries": [ { "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.", - "kind": "problem", + "kind": "path-problem", "name": "A block of memory shall only be freed if it was allocated by means of a Standard Library function", "precision": "high", "severity": "error", From dc22813d2738fb3b2201b95bfd073f7e942d2d86 Mon Sep 17 00:00:00 2001 From: Nikita Kraiouchkine Date: Tue, 28 Mar 2023 20:18:08 +0200 Subject: [PATCH 13/13] Update FreeMemoryWhenNoLongerNeededShared.qll --- .../FreeMemoryWhenNoLongerNeededShared.qll | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll index 78ef0e228a..035c0a1f4a 100644 --- a/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll +++ b/cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll @@ -14,9 +14,9 @@ predicate allocated(FunctionCall fc) { allocExpr(fc, _) } /** An expression for which there exists a function call that might free it. */ class FreedExpr extends PointsToExpr { - FreedExpr() { freeExprOrIndirect(this, _, _) } + FreedExpr() { freeExprOrIndirect(_, this, _) } - override predicate interesting() { freeExprOrIndirect(this, _, _) } + override predicate interesting() { freeExprOrIndirect(_, this, _) } } /** @@ -59,19 +59,6 @@ predicate allocCallOrIndirect(Expr e) { ) } -predicate freeCallOrIndirect(FunctionCall fc, Variable v) { - // direct free call - v.getAnAccess() = fc.(DeallocationExpr).getFreedExpr() - or - // indirect free call - exists(FunctionCall midcall, Function mid, int arg | - fc.getArgument(arg) = v.getAnAccess() and - mayCallFunction(fc, mid) and - midcall.getEnclosingFunction() = mid and - freeCallOrIndirect(midcall, mid.getParameter(arg)) - ) -} - predicate allocDefinition(StackVariable v, ControlFlowNode def) { exists(Expr expr | exprDefinition(v, def, expr) and allocCallOrIndirect(expr)) } @@ -86,7 +73,7 @@ class MallocVariableReachability extends StackVariableReachabilityWithReassignme override predicate isSinkActual(ControlFlowNode node, StackVariable v) { // node may be used in allocReaches exists(node.(AnalysedExpr).getNullSuccessor(v)) or - freeCallOrIndirect(node, v) or + freeExprOrIndirect(node, v.getAnAccess(), _) or assignedToFieldOrGlobal(v, node) or // node may be used directly in query v.getFunction() = node.(ReturnStmt).getEnclosingFunction() @@ -133,7 +120,7 @@ class MallocReachability extends StackVariableReachabilityExt { // assigned to a global at node, or NULL checked on the edge node -> next. exists(StackVariable v0 | mallocVariableReaches(v0, source, node) | node.(AnalysedExpr).getNullSuccessor(v0) = next or - freeCallOrIndirect(node, v0) or + freeExprOrIndirect(node, v0.getAnAccess(), _) or assignedToFieldOrGlobal(v0, node) ) }