Implement C Pointers3 package #139

nicolaswill · 2022-11-16T16:26:33Z

Description

Implements the C Pointers3 package

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:
- EXP32-C (closes github/coding-standards-team#764)
- EXP36-C (closes github/coding-standards-team#765)
- EXP39-C (closes github/coding-standards-team#766)
- EXP43-C (closes github/coding-standards-team#767)
Queries have been modified for the following rules:
- RULE-11-7

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 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!)

Add rule definition and package files Add Pointers3 to package list in tasks.json

The previous message referenced a "pointer to void type" rather than "pointer to object type" as specified by the query.

Additionally updated the `EXP36-C` test-case and refactored `MEM57-CPP` to move certain classes and predicates to the `Alignment.qll` library for re-use in `EXP36-C`.

lcartey

Started my review, will add some more comments tomorrow.

c/cert/src/rules/EXP32-C/DoNotAccessVolatileObjectWithNonVolatileReference.ql

c/cert/src/rules/EXP36-C/DoNotCastPointerToMoreStrictlyAlignedPointerType.ql

lcartey

Added a few comments for early feedback.

c/cert/src/rules/EXP32-C/DoNotAccessVolatileObjectWithNonVolatileReference.ql

nicolaswill · 2023-01-16T21:22:41Z

small note: 8633a30 is the first commit to contain the EXP32-C performance fix and changes to getVolatileDepth

…tandards into pointers3

nicolaswill · 2023-01-27T02:09:52Z

@lcartey EXP43-C - RestrictPointerReferencesOverlappingObject.ql is a bit slow on OpenPilot due to data-flow-related predicates; the execution time was just under 3 minutes on my machine with a cold cache. I am also not sure if the final approach I took for that query is quite right and could use an in-depth review.

knewbury01

just some minor comments!

c/cert/src/rules/EXP43-C/DoNotPassAlisedPointerToRestrictQualifiedParameter.ql

c/cert/test/rules/EXP43-C/test.c

c/cert/src/rules/EXP43-C/RestrictPointerReferencesOverlappingObject.ql

Replace local with global data-flow. Modify the query implementation to detect violations of aliasing at block rather than statement scope as defined in the standard.

lcartey

A few final comments, mostly on refining messages a couple of perf/scope issues. Looking good!

c/cert/src/rules/EXP36-C/DoNotCastPointerToMoreStrictlyAlignedPointerType.ql

c/cert/test/rules/EXP39-C/test.c

c/cert/src/rules/EXP39-C/DoNotAccessVariableViaPointerOfIncompatibleType.ql

c/cert/src/rules/EXP43-C/DoNotPassAliasedPointerToRestrictQualifiedParam.ql

c/cert/src/rules/EXP43-C/RestrictPointerReferencesOverlappingObject.ql

Co-authored-by: Luke Cartey <5377966+lcartey@users.noreply.github.com>

…tandards into pointers3

Remove an accidental `isBarrier` predicate left in from development

nicolaswill · 2023-02-15T21:21:09Z

I've made changes to address all of the above comments.

lcartey and others added 9 commits November 7, 2022 09:57

EXP32-C: Add test case

efb286d

EXP36-C: Add test case

ee22665

EXP43-C: Add test case

94347c4

EXP39-C: Add malloc/realloc test

741ab39

EXP43-C: Add test cases

8462844

Add Pointers3 package

e0e4181

Add rule definition and package files Add Pointers3 to package list in tasks.json

Fix RULE-11-7 query output message

1bfa1b1

The previous message referenced a "pointer to void type" rather than "pointer to object type" as specified by the query.

Implement EXP32-C and EXP36-C

833d834

Additionally updated the `EXP36-C` test-case and refactored `MEM57-CPP` to move certain classes and predicates to the `Alignment.qll` library for re-use in `EXP36-C`.

Merge pull request #127 from lcartey/lcartey/pointers3

3d337ce

nicolaswill requested a review from lcartey November 16, 2022 16:26

nicolaswill self-assigned this Nov 16, 2022

lcartey requested changes Nov 16, 2022

View reviewed changes

lcartey reviewed Nov 17, 2022

View reviewed changes

c/cert/src/rules/EXP36-C/DoNotCastPointerToMoreStrictlyAlignedPointerType.ql Outdated Show resolved Hide resolved

Nikita Kraiouchkine added 7 commits November 17, 2022 14:08

Update EXP32-C query and test

d483c3c

Add comments to EXP32-C query

6733786

Add comments and implementation scope for EXP32-C

a157089

Update EXP36-C query and test

03bac50

Update EXP39-C test and implement query

0c28310

Update Pointers3.json

87d52ae

Update DoNotAccessVariableViaPointerOfIncompatibleType.ql

c0f9767

lcartey reviewed Jan 9, 2023

View reviewed changes

c/cert/src/rules/EXP32-C/DoNotAccessVolatileObjectWithNonVolatileReference.ql Outdated Show resolved Hide resolved

c/cert/src/rules/EXP32-C/DoNotAccessVolatileObjectWithNonVolatileReference.ql Outdated Show resolved Hide resolved

Nikita Kraiouchkine added 2 commits January 16, 2023 20:54

Resolve EXP32-C performance issue

0431c73

Update DoNotAccessVolatileObjectWithNonVolatileReference.ql

8633a30

nicolaswill requested a review from lcartey January 16, 2023 21:20

Implement EXP43-C and add help files

469e42c

nicolaswill marked this pull request as ready for review January 27, 2023 01:43

Nikita Kraiouchkine added 3 commits January 27, 2023 02:44

Merge branch 'main' into pointers3

0332744

Update RuleMetadata.qll

72e9fd6

Update test.c

0e9df1e

Nikita Kraiouchkine added 3 commits January 27, 2023 02:48

Merge branch 'pointers3' of https://github.com/github/codeql-coding-s…

f2659d6

…tandards into pointers3

Regenerate Pointers3 package files

7bb5665

Update test.c

6d25cd6

Correct whitespace in rule help files (regen package)

23961bc

nicolaswill enabled auto-merge January 27, 2023 02:19

Nikita Kraiouchkine added 2 commits January 27, 2023 03:26

Update EXP43-C message and expected output

e2e6590

Revert accidental change to EXP39-C and update .expected results

5f09408

knewbury01 reviewed Jan 27, 2023

View reviewed changes

c/cert/src/rules/EXP43-C/DoNotPassAlisedPointerToRestrictQualifiedParameter.ql Outdated Show resolved Hide resolved

c/cert/test/rules/EXP43-C/test.c Show resolved Hide resolved

c/cert/src/rules/EXP43-C/RestrictPointerReferencesOverlappingObject.ql Outdated Show resolved Hide resolved

Nikita Kraiouchkine added 6 commits January 28, 2023 01:20

EXP43-C: add test-case and update expected results

68b3f39

EXP43-C: remove redundant check

934811d

Move PointerOrArrayType into Pointers.qll to deduplicate usage

de6cd01

Update EXP43-C query and test

a390e54

Replace local with global data-flow. Modify the query implementation to detect violations of aliasing at block rather than statement scope as defined in the standard.

EXP43-C: swap PointsTo with data-flow and fix typo

c239197

EXP43-C: Revert IR dataflow to AST data-flow

2dd045c

lcartey requested changes Feb 15, 2023

View reviewed changes

Nikita Kraiouchkine and others added 6 commits February 15, 2023 16:08

EXP43-C: Replace != with not =

563e084

Co-authored-by: Luke Cartey <5377966+lcartey@users.noreply.github.com>

EXP36-C: Output cast instead of sink node

3a3a6a5

EXP39-C: Remove redundant "TODO" comment from test-case

009fc65

EXP43-C: Refine data-flow and add context to output

6a554ac

Merge branch 'pointers3' of https://github.com/github/codeql-coding-s…

46abe38

…tandards into pointers3

EXP43-C: Refactor and remove test code

2f91e33

Remove an accidental `isBarrier` predicate left in from development

nicolaswill requested a review from lcartey February 15, 2023 21:19

lcartey approved these changes Feb 15, 2023

View reviewed changes

Merge branch 'main' into pointers3

0b564db

nicolaswill merged commit 1821034 into main Feb 15, 2023

nicolaswill deleted the pointers3 branch February 15, 2023 22:52

Implement C Pointers3 package #139

Implement C Pointers3 package #139

Uh oh!

Conversation

nicolaswill commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Change request type

Rules with added or modified queries

Release change checklist

Query development review checklist

Author

Reviewer

Uh oh!

lcartey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcartey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicolaswill commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolaswill commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knewbury01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcartey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolaswill commented Feb 15, 2023

Uh oh!

Uh oh!

nicolaswill commented Nov 16, 2022 •

edited

Loading

nicolaswill commented Jan 16, 2023 •

edited

Loading

nicolaswill commented Jan 27, 2023 •

edited

Loading