Implement Concurrency6 package -- split out Concurrency7 #791
Conversation
| * @problem.severity error | ||
| * @tags external/misra/id/dir-5-3 | ||
| * external/misra/c/2012/amendment4 | ||
| * external/misra/c/audit |
There was a problem hiding this comment.
We don't currently have an audit query suite - could you add one? Thanks!
| import codingstandards.c.misra | ||
| import codingstandards.cpp.Concurrency | ||
|
|
||
| Function callers(Function f) { result = f.getACallToThisFunction().getEnclosingFunction() } |
There was a problem hiding this comment.
There's already a standard library predicate for this - Function.calls(Function).
|
|
||
| Function callers(Function f) { result = f.getACallToThisFunction().getEnclosingFunction() } | ||
|
|
||
| class ThreadReachableFunction extends Function { |
There was a problem hiding this comment.
I think this would be nicer if you rewrote this as a ThreadRoot class, that provides accessors getThreadCreateCall() and getAReachableFunction().
Then the query could be rewritten as:
from ThreadRoot threadRoot, CThreadCreateCall tc
where
not isExcluded(tc, Concurrency6Package::threadCreatedByThreadQuery()) and
threadRoot.getAReachableFunction() = tc.getEnclosingFunction()
select tc, "Thread creation call reachable from threaded function '$@' created by call to $@.", threadRoot,
threadRoot.getName(), threadRoot.getThreadCreateCall() as tcOriginal, tcOriginal.getTarget().getName()Note, I'm also suggesting here tagging in the original CThreadCreateCall, so that it's easier for a user to verify that threadRoot is actually a threaded function.
| not isExcluded(expr, Concurrency6Package::atomicAggregateObjectDirectlyAccessedQuery()) and | ||
| not expr.isUnevaluated() and | ||
| ( | ||
| exists(FieldAccess fa | |
There was a problem hiding this comment.
Are the atomic_.. access functions alway guaranteed to be function calls and not macros? Could we get false positives for this query if a standard library decided to implement a macro which did access a field? In general the C library can be implemented with functions or macros. Or, in practice do we expect all standard libraries to implement this using a function call?
| ( | ||
| exists(FieldAccess fa | | ||
| expr = fa and | ||
| fa.getQualifier().getUnderlyingType().hasSpecifier("atomic") and |
There was a problem hiding this comment.
Technically you don't need the getUnderlyingType here
| or | ||
| // Non-macro case, may occur in a subset of gcc/clang functions: | ||
| prefix = "" and | ||
| suffix = "_explicit" and |
There was a problem hiding this comment.
The .* suffix would also capture _explicit, but not be handled by this case.
|
|
||
| /** | ||
| * If the node is a memory order constant, or shares a value with a memory order constant, then | ||
| * return the name of that cnonstant. Otherwise, simply print the node. |
There was a problem hiding this comment.
| * return the name of that cnonstant. Otherwise, simply print the node. | |
| * return the name of that constant. Otherwise, simply print the node. |
| not value = any(AllowedMemoryOrder e).getName() and | ||
| function.getACallToThisFunction().getAnArgument() = argument | ||
| select argument, source, sink, "Invalid memory order '$@' in call to function '$@'.", value, value, | ||
| function, function.toString() |
There was a problem hiding this comment.
In general prefer to use .getName() for functions, as toString() may be evaluated for all elements in the database.
| function, function.toString() | |
| function, function.getName() |
| ) | ||
| or | ||
| // Everything else: not a memory order constant or an integer valued literal, also exclude | ||
| // variables and functions, things that flow further back. |
There was a problem hiding this comment.
What else would this be? And if we cannot determine the value for it, presumably we can't tell whether it's in violation or not?
| atomic_s1.x; // NON-COMPLIANT | ||
| ptr_atomic_s1->x; // NON-COMPLIANT | ||
| atomic_s1.x = 0; // NON-COMPLIANT | ||
| ptr_atomic_s1->x = 0; // NON-COMPLIANT |
There was a problem hiding this comment.
Can we add some user macro-wrapped cases?
Description
Implement Concurrency6 package -- split out Concurrency7.
I had added support for
pthread_create, and left it in. Should I enable support for pthreads in the control flow graph inConcurrency.qll?Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
DIR-5-2DIR-5-3RULE-12-6RULE-21-25RULE-22-11Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
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.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
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
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.
Reviewer
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.