CPP: Deprecate PotentialBufferOverflow.ql#923
Conversation
|
Is PotentialBufferOverflow referenced in a query suite? |
It's only in the security suite, which I've updated in this PR. Surprisingly it was not included in |
| * @deprecated | ||
| * | ||
| * This query is deprecated, use Security/CWE/CWE-120/OverrunWrite.ql and | ||
| * Security/CWE/CWE-120/OverrunWriteFloat.ql instead. |
There was a problem hiding this comment.
Does it work with all our tools to have two line breaks after @deprecated? I used to add one line break after @description, but I had to revert that because one of our tools (the Confluence uploader?) didn't support it.
There was a problem hiding this comment.
I'm not quite sure what you're saying here. Does @deprecated have to be the last thing in the opening comment?
There was a problem hiding this comment.
What I'm suggesting is to format @deprecated like we format @description and write
* @deprecated This query is deprecated. Use
* Security/CWE/CWE-120/OverrunWrite.ql and
* Security/CWE/CWE-120/OverrunWriteFloat.ql instead.
I don't actually know whether this text is picked up by tools now or in the future. I tried to grep for text after @deprecated in our existing queries, and I found only three JS queries that use it. They all have the text on the same line as @deprecated.
There was a problem hiding this comment.
I've never seen this syntax before. Happy to change it, but doing so will establish a precedent that this is the right thing to do, and I'm not sure it is.
There was a problem hiding this comment.
(all I originally intended was to write a comment at the top of the file, I used to use // for this but I don't think the autoformatter liked it)
There was a problem hiding this comment.
I checked in #ql-help on Slack, and the answer was that @deprecated can have a message after it like @description, and this message could be used by our tools even if they might not be using it now.
There is already precedent for this: https://github.com/Semmle/ql/blob/2dea0b4270/javascript/ql/src/LanguageFeatures/HTMLComments.ql#L11-L12
There was a problem hiding this comment.
OK, done, sorry for the delay.
I've also updated our other @deprecated messages to follow the agreed format.
geoffw0
force-pushed
the
potentialbufferoverflow
branch
from
b4562fb to
25a5ff5
Compare
3 participants
Deprecate
PotentialBufferOverflow.ql. Everything it does is done byOverrunWrite.qlandOverrunWriteFloat.qltogether, and the latter support a much wider variety of functions. I can't find any real world results unique toPotentialBufferOverflow.ql(https://lgtm.com/query/1401438515475863559/).In addition, despite appearances, the claim in the qhelp that
PotentialBufferOverflow.qlcovers uses ofvsprintfis false (because they're outside the scope ofFormattingFunctionCall). I may at some point get around to makingOverrunWrite.qlsupport that, but we're not losing anything right now.