Implement InvalidMemory3, Rule 18-8 amendment. #750

MichaelRFairhurst · 2024-10-16T23:58:20Z

Description

Implements package InvalidMemory3, which detects pointer-to-array conversions and modification of array values with temporary lifetimes, (RULE-18-9) and bans pointers to variably modified types (RULE-18-10).

RULE-18-10 is marked as "split" from RULE-18-8 in the amendments file, so update RULE-18-8 to only detect allocating VLA declarations. In the process, remove potential false positives around incomplete array parameter types, eg, f(int arr[]). To protect against the same false positives from occurring in RULE-18-10, use locations that correspond with VlaDimensionStmt instances.

If it is better to split this PR up, I'm happy to do so!

Change request type

Release or process automation (GitHub workflows, internal scripts)
Internal documentation
External documentation
Query files (.ql, .qll, .qls or unit tests)
External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

No rules added
Queries have been added for the following rules:
- RULE-18-9
- RULE-18-10
Queries have been modified for the following rules:
- RULE-18-8

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

The structure or layout of the release artifacts.
The evaluation performance (memory, execution time) of an existing query.
The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

Yes
No

🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

Confirmed

Query development review checklist

For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:

Author

Have all the relevant rule package description files been checked in?
Have you verified that the metadata properties of each new query is set appropriately?
Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
Are the alert messages properly formatted and consistent with the style guide?
Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Does the query have an appropriate level of in-query comments/documentation?
Have you considered/identified possible edge cases?
Does the query not reinvent features in the standard library?
Can the query be simplified further (not golfed!)

Reviewer

Have all the relevant rule package description files been checked in?
Have you verified that the metadata properties of each new query is set appropriately?
Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
Are the alert messages properly formatted and consistent with the style guide?
Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Does the query have an appropriate level of in-query comments/documentation?
Have you considered/identified possible edge cases?
Does the query not reinvent features in the standard library?
Can the query be simplified further (not golfed!)

…lement-package-invalid-memory3

…memory3

lcartey

Thanks! Some suggestions and comments to follow up on individual files.

lcartey · 2024-11-15T11:00:53Z

c/misra/test/rules/RULE-18-8/test.c

+
+  // A typedef is not a VLA. However, `VlaDeclStmt`s match the typedef.
+  typedef int vlaTypedef[n]; // COMPLIANT[FALSE_POSITIVE]
+  vlaTypedef t1;             // NON_COMPLIANT[FALSE_NEGATIVE]


I think we should remove the false positive and false negative markers here, and accept the existing results as correct.

I justify this in two ways:

It's more developer friendly to highlight the typedef that introduces the VLA, rather than the actual declaration. It's where they would likely need to fix the problem, and it reduces the number of results that need to be managed vs. every use of the typedef.

I think that's consistent with how MISRA intended to report the results. This forum post https://forum.misra.org.uk/archive/index.php?thread-1384.html, while not being 100% clear, I think is indicative that typedefs themselves should be considered a use of variable-length arrays.

I'm happy to do it that way, and I think the first point is a good reason why we would want to do it this way. That said, in my conversations with Robert, our conclusion was that 18-8 is intended to reject allocating VLAs. It's the allocation of a VLA that makes them so dangerous to use. In my head, a VLA is an object in memory, while a VLA type is the static type of that object, and the rule now states that it's VLAs which are banned, not VLA types.

Of course, an unused VLA typedef should be deleted. And most uses of a VLA typedef would be invalid. But in theory, not all. A VLA typedef could be used in a sizeof(), or it could be used in a context where it is adjusted to a pointer.

I don't think these cases are likely to come up, I did want to document them correctly in the tests.

Perhaps I should add this explanation in a comment?

Re-reading the rule, I think your interpretation as it intending to identify allocations is correct. The rule starts by saying:

Variable-length arrays are specified when the size of an array declared in a block or function prototype is not an integer constant expression.

Which would exclude an unused typedef, or a typedef that is not used to declare an array in a block or function prototype. This was a change from the previous interpretation for Rule 18.8 in MISRA C 2012.

I think there's two options for how we handle this:

We continue to flag the typedef, but only report it if we find it used as the type in a variable declaration in a "valid" location - e.g. where there exists a Variable whose type is the typedef (or derived from the typedef?). We could add the Variables as placeholders in this case.

We re-jig the query to report Variables, where the Variable is the getVariable() of the VlaDeclStmt or where the variable type is a use of a the TypeDef associated with a VlaDeclStmt.

If you've used a typedef I think it's likely you're intending to use the type in multiple places, so I would say that's an argument to continue to flag the typedef. However, it may not be that common in practice, so it's possibly a premature optimization in reporting.

c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql

cpp/common/src/codingstandards/cpp/VariablyModifiedTypes.qll

lcartey · 2024-11-15T11:45:58Z

c/misra/src/rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.ql

+      s.getADeclaration() = entry.getDeclaration() and
+      before = s.getLocation() and
+      after = before and
+      before.subsumes(inner)


I think we should consider what happens if the declaration is in a macro. Specifically, what happens if a macro has multiple array declarations, only one of which is a VM. I suspect we'll end up flagging all of them, as they will all have the same location.

Note: that is not a problem for the parameter case, because we require a strict before/after location.

We can exclude that case. In general, it'd be easy to exclude all cases where the declaration entry isGeneratedByMacro.

However, it's worth noting that the false positives are incomplete array types. Variables can't be declared with incomplete array types. So the cases where this would flag multiple things because they all match would be a pretty unusual case where, for instance, multiple declared variables have function types with incomplete array types as parameters.

Could we detect when this issue had occurred by determining if the number of VlaDeclStmt and the number candidates that occur at the same location match? Then avoid reporting in the case where it doesn't?

cpp/common/src/codingstandards/cpp/lifetimes/CLifetimes.qll

c/misra/src/rules/RULE-18-9/ArrayToPointerConversionOfTemporaryObject.ql

…memory3

lcartey · 2024-11-22T11:23:37Z

cpp/common/src/codingstandards/cpp/VariablyModifiedTypes.qll

+/**
+ * A declaration involving a variably-modified type.
+ */
+class VmtDeclarationEntry extends DeclarationEntry {


I think we're only interested in variable declarations, right?

Suggested change

class VmtDeclarationEntry extends DeclarationEntry {

class VmtDeclarationEntry extends VariableDeclarationEntry {

lcartey · 2024-11-22T11:24:17Z

cpp/common/src/codingstandards/cpp/VariablyModifiedTypes.qll

+ * the `Location` of the containing `DeclStmt`.
+ */
+private predicate declarationSubsumes(
+  DeclarationEntry entry, Location inner, Location before, Location after


Same here.

Suggested change

DeclarationEntry entry, Location inner, Location before, Location after

VariableDeclarationEntry entry, Location inner, Location before, Location after

lcartey · 2024-11-22T11:45:21Z

c/misra/src/rules/RULE-18-9/ArrayToPointerConversionOfTemporaryObject.ql

+  fa.getTemporary() = temporary and
+  conversion.getExpr() = fa and
+  isUsedOrStored(temporaryObjectFlowStep*(conversion.getExpr()))
+select conversion, "Array to pointer conversion of array $@ from temporary object $@",


Add a full stop to the alert messages, as per the style guide.

MichaelRFairhurst and others added 4 commits October 16, 2024 16:51

Implement InvalidMemory, Rule 18-8 amendment.

dd475be

Fix formatting

5b5777f

Merge remote-tracking branch 'origin/main' into michaelrfairhurst/imp…

01e51e8

…lement-package-invalid-memory3

Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…

ab4a7bf

…memory3

MichaelRFairhurst requested review from lcartey and data-douser October 17, 2024 04:55

lcartey requested changes Nov 15, 2024

View reviewed changes

Address feedback

06ba26d

MichaelRFairhurst requested a review from lcartey November 18, 2024 08:00

lcartey added 2 commits November 21, 2024 11:52

Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…

7014931

…memory3

Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…

ef76126

…memory3

lcartey reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement InvalidMemory3, Rule 18-8 amendment. #750

Implement InvalidMemory3, Rule 18-8 amendment. #750

MichaelRFairhurst commented Oct 16, 2024 •

edited by lcartey

Loading

lcartey left a comment

lcartey Nov 15, 2024

MichaelRFairhurst Nov 18, 2024

lcartey Nov 22, 2024

lcartey Nov 15, 2024

MichaelRFairhurst Nov 18, 2024

lcartey Nov 22, 2024

lcartey Nov 22, 2024

lcartey Nov 22, 2024

lcartey Nov 22, 2024

	class VmtDeclarationEntry extends DeclarationEntry {
	class VmtDeclarationEntry extends VariableDeclarationEntry {

	DeclarationEntry entry, Location inner, Location before, Location after
	VariableDeclarationEntry entry, Location inner, Location before, Location after

Implement InvalidMemory3, Rule 18-8 amendment. #750

Are you sure you want to change the base?

Implement InvalidMemory3, Rule 18-8 amendment. #750

Conversation

MichaelRFairhurst commented Oct 16, 2024 • edited by lcartey Loading

Description

Change request type

Rules with added or modified queries

Release change checklist

Query development review checklist

Author

Reviewer

lcartey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRFairhurst commented Oct 16, 2024 •

edited by lcartey

Loading