CPP: Split PotentiallyDangerousFunction.ql#1315
Conversation
geoffw0
added
C++
depends on internal PR
|
redo failed |
| @@ -2,7 +2,7 @@ | |||
| * @name Use of potentially dangerous function | |||
| * @description Certain standard library functions are dangerous to call. | |||
There was a problem hiding this comment.
I think this name and description has always been a bit exaggerated. Now that we're toning down the severity, let's also tone down the other metadata. Perhaps we can focus this query on non-reentrant functions. Then the name could be "Use of non-reentrant function" or "Use of non-threadsafe function", and the description could be adjusted similarly. Over time we can add other such functions to the query.
I think it makes sense to focus the query on re-entrancy since all re-entrancy issues will share the same alert suppression patterns: if someone wants to suppress results about non-reentrant time functions in their lgtm.yml, they probably also want to suppress results about non-reentrant DNS lookup functions. If we add logic to the query to only produce alerts in multi-threaded code, then that similarly applies to use of non-reentrant functions but not to other "potentially dangerous" functions.
| * @kind problem | ||
| * @problem.severity error | ||
| * @precision very-high | ||
| * @id cpp/potentially-dangerous-function |
There was a problem hiding this comment.
This query needs its own query ID. Even though only gets is matched right now, I suggest making the ID slightly more general so it can also accommodate sprintf("%s", ...) in the future if we want that. Maybe @id cpp/unbounded-read-from-file or something like that. The file name should match whatever we decide on for query id.
There was a problem hiding this comment.
Ah, missed the query ID! Fixed now, and I've changed the name to be a bit more general than just 'gets'.
| @@ -0,0 +1,18 @@ | |||
| /** | |||
| * @name Use of dangerous function 'gets' | |||
| * @description The standard library 'gets' function is dangerous and should not be used. | |||
There was a problem hiding this comment.
This new query is not in any suites, is it? We still use the hand-maintained suites for nightly jobs, and the security suites are used by some customers.
There was a problem hiding this comment.
Whoops, forgot to update suites as well. Updated.
|
redo failed |
| * @id cpp/potentially-dangerous-function | ||
| * @tags reliability | ||
| * security | ||
| * external/cwe/cwe-242 |
There was a problem hiding this comment.
Shouldn't this be 676, not 242?
There was a problem hiding this comment.
CWE-242 ('Use of Inherently Dangerous Function') is probably the more accurate CWE in this case and the one the CWE examples and samate tests associated with this query are under; where as CWE-676 'Use of Potentially Dangerous Function' is a more general CWE that we have several queries under, hence the directory name. According to https://wiki.semmle.com/display/IN/Modelling+CWEs+at+Semmle "The rule we currently follow is to add @cwe tags for all of the most specific (using the parent/child relationship in View 1000) Weakness Bases or Weakness Classes which apply to a query." so I think this should be tagged CWE-242. I'm happy to be corrected on this though.
| * @problem.severity warning | ||
| * @precision high | ||
| * @id cpp/potentially-dangerous-function | ||
| * @tags reliability |
There was a problem hiding this comment.
Is 242 the correct CWE?
676?
There was a problem hiding this comment.
Yeah, this one should be 676 I think. Updated.
| rules for the following CWEs:</p> | ||
| <ul> | ||
| <li>CWE-120 Classic Buffer Overflow | ||
| </li><li>CWE-131 Incorrect Calculation of Buffer Size |
There was a problem hiding this comment.
Make these links to https://cwe.mitre.org/data/definitions/120.html and https://cwe.mitre.org/data/definitions/131.html
|
Added change notes. |
|
Updated. |
This seems to have been missed in #1315.
3 participants
Split PotentiallyDangerousFunction.ql, as discussed on https://discuss.lgtm.com/t/ctime-reported-incorrectly-as-dangerous/2074.