Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brodes/seh flow phase1 throwing models #18014

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
de05aee
Adding model transition to using Throwing.qll.
bdrodes Nov 18, 2024
4b83a45
Change log
bdrodes Nov 18, 2024
1c7b5ae
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 18, 2024
792231c
Removing SEH default case for function calls as the logic to handle S…
bdrodes Nov 18, 2024
1c874d3
Fixed usage raisesException
bdrodes Nov 19, 2024
5bb765d
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 19, 2024
26d590a
Putting back deleted file, and deprecating instead. Deprecating mayTh…
bdrodes Nov 19, 2024
63ddd81
Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…
bdrodes Nov 19, 2024
0784776
bringing back mayThrowException to make it cleaner/easier for backwar…
bdrodes Nov 19, 2024
ae1ed38
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 19, 2024
a69daa0
Missing change to 'mayThrowException' in StructuredExceptionHandling.qll
bdrodes Nov 19, 2024
23485f1
Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…
bdrodes Nov 19, 2024
4e77756
Changing terminology back to "throws" vs "rasis" for alwaysThrowsExce…
bdrodes Nov 19, 2024
69df07e
Update cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md
bdrodes Nov 20, 2024
6aa7412
Merge branch 'main' into brodes/seh_flow_phase1_throwing_models
bdrodes Nov 20, 2024
9b2590e
Updating PR per review comments. Moving more towards a simplified model.
bdrodes Nov 21, 2024
4412691
Delaying deprecation of ThrowingFunction.
bdrodes Nov 21, 2024
7059fc3
Adding intermediate solution towards deprecating ThrowingFunction
bdrodes Nov 21, 2024
248f1c4
Updating change log
bdrodes Nov 21, 2024
583651b
Missing NonCppThrowingFunction changes in Printf.qll
bdrodes Nov 21, 2024
66cf736
printf formatting.
bdrodes Nov 21, 2024
37365c7
Updating to NonCppThrowingFunction use in IncorrectALlocationErrorHan…
bdrodes Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: deprecated
---
* The `NonThrowing` class (`semmle.code.cpp.models.interfaces.NonThrowing`) has been deprecated. Please use the `NonCppThrowingFunction` class instead.
Original file line number Diff line number Diff line change
Expand Up @@ -364,10 +364,14 @@ class TranslatedFunctionCall extends TranslatedCallExpr, TranslatedDirectCall {

final override predicate mayThrowException() {
expr.getTarget().(ThrowingFunction).mayThrowException(_)
or
expr.getTarget() instanceof AlwaysSehThrowingFunction
}

final override predicate mustThrowException() {
expr.getTarget().(ThrowingFunction).mayThrowException(true)
or
expr.getTarget() instanceof AlwaysSehThrowingFunction
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import semmle.code.cpp.models.interfaces.NonThrowing
* `__builtin___memcpy_chk`.
*/
private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction,
AliasFunction, NonThrowingFunction
AliasFunction, NonCppThrowingFunction
{
MemcpyFunction() {
// memcpy(dest, src, num)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing

private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, AliasFunction,
SideEffectFunction, NonThrowingFunction
SideEffectFunction, NonCppThrowingFunction
{
MemsetFunctionModel() {
this.hasGlobalOrStdOrBslName("memset")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ import semmle.code.cpp.models.interfaces.NonThrowing
*
* Note: The `throw` specifier was deprecated in C++11 and removed in C++17.
*/
class NoexceptFunction extends NonThrowingFunction {
class NoexceptFunction extends NonCppThrowingFunction {
NoexceptFunction() { this.isNoExcept() or this.isNoThrow() }
}
12 changes: 7 additions & 5 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import semmle.code.cpp.models.interfaces.NonThrowing
/**
* The standard functions `printf`, `wprintf` and their glib variants.
*/
private class Printf extends FormattingFunction, AliasFunction, NonThrowingFunction {
private class Printf extends FormattingFunction, AliasFunction, NonCppThrowingFunction {
Printf() {
this instanceof TopLevelFunction and
(
Expand All @@ -37,7 +37,7 @@ private class Printf extends FormattingFunction, AliasFunction, NonThrowingFunct
/**
* The standard functions `fprintf`, `fwprintf` and their glib variants.
*/
private class Fprintf extends FormattingFunction, NonThrowingFunction {
private class Fprintf extends FormattingFunction, NonCppThrowingFunction {
Fprintf() {
this instanceof TopLevelFunction and
(
Expand All @@ -55,7 +55,7 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction {
/**
* The standard function `sprintf` and its Microsoft and glib variants.
*/
private class Sprintf extends FormattingFunction, NonThrowingFunction {
private class Sprintf extends FormattingFunction, NonCppThrowingFunction {
Sprintf() {
this instanceof TopLevelFunction and
(
Expand Down Expand Up @@ -98,7 +98,9 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction {
/**
* Implements `Snprintf`.
*/
private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction, NonThrowingFunction {
private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction,
NonCppThrowingFunction
{
SnprintfImpl() {
this instanceof TopLevelFunction and
(
Expand Down Expand Up @@ -205,7 +207,7 @@ private class StringCchPrintf extends FormattingFunction {
/**
* The standard function `syslog`.
*/
private class Syslog extends FormattingFunction, NonThrowingFunction {
private class Syslog extends FormattingFunction, NonCppThrowingFunction {
Syslog() {
this instanceof TopLevelFunction and
this.hasGlobalName("syslog") and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import semmle.code.cpp.models.interfaces.NonThrowing
* Does not include `strlcat`, which is covered by `StrlcatFunction`
*/
class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, SideEffectFunction,
NonThrowingFunction
NonCppThrowingFunction
{
StrcatFunction() {
this.hasGlobalOrStdOrBslName([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import semmle.code.cpp.models.interfaces.NonThrowing
* The standard function `strcpy` and its wide, sized, and Microsoft variants.
*/
class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction,
NonThrowingFunction
NonCppThrowingFunction
{
StrcpyFunction() {
this.hasGlobalOrStdOrBslName([
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import semmle.code.cpp.models.interfaces.Throwing

class WindowsDriverFunction extends ThrowingFunction {
WindowsDriverFunction() {
class WindowsDriverExceptionAnnotation extends AlwaysSehThrowingFunction {
WindowsDriverExceptionAnnotation() {
this.hasGlobalName(["RaiseException", "ExRaiseAccessViolation", "ExRaiseDatatypeMisalignment"])
}

final override predicate mayThrowException(boolean unconditional) { unconditional = true }
}
10 changes: 9 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/models/interfaces/NonThrowing.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@
import semmle.code.cpp.Function
import semmle.code.cpp.models.Models

/**
* A function that is guaranteed to never throw a C++ exception
* (distinct from a structured exception handling, SEH, exception).
*/
abstract class NonCppThrowingFunction extends Function { }

/**
* A function that is guaranteed to never throw.
*
* DEPRECATED: use `NonCppThrowingFunction` instead.
*/
abstract class NonThrowingFunction extends Function { }
Copy link
Contributor

@jketema jketema Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot just delete files that are not in non-implementation or internal directories. These will need to go through a deprecation period. Could you discuss with @MathiasVP what the correct approach should be here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we have a 1-ish year deprecation period for non-internal QL things. So we need to:

  • Mark this class as deprecated
  • Keep the functionality. i.e., extending NonThrowingFunction should do something equivalent to what it did before

And then it'll be deleted by someone in a PR a year from now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that after I submitted, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just do:

private import Throwing as T

deprecated class NonThrowingFunction = T::NonThrowingFunction;

and write in the change note that (a) the class is deprecated, and (b) requires a new member to be implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is that too dirty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can definitely do that, yes!

Copy link
Contributor Author

@bdrodes bdrodes Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this, but I had avoided changes like this because any existing use of the old NonThrowingFunction would require the member predicate be defined, breaking any existing queries. I thought the point of deprecating vs deleting was to not completely break existing builds. If you are actually ok with that @jketema I'm ok with it.

Copy link
Contributor Author

@bdrodes bdrodes Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose another option is to just make the deprecated version extend the new throwing function mechanic and set the exception type predicate to be any type (matching the prior intended behavior). I can do whatever, just please advise what is more acceptable to github standards.

deprecated class NonThrowingFunction = NonCppThrowingFunction;
11 changes: 10 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,21 @@ import semmle.code.cpp.models.Models
import semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs

/**
* A class that models the exceptional behavior of a function.
* A function that is known to raise an exception.
*/
abstract class ThrowingFunction extends Function {
ThrowingFunction() { any() }

/**
* Holds if this function may throw an exception during evaluation.
* If `unconditional` is `true` the function always throws an exception.
*/
abstract predicate mayThrowException(boolean unconditional);
}

/**
* A function that is known to raise an exception unconditionally.
* The only cases known where this happens is for SEH
* (structured exception handling) exceptions.
*/
abstract class AlwaysSehThrowingFunction extends Function { }
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ predicate deleteMayThrow(DeleteOrDeleteArrayExpr deleteExpr) {
* like it might throw an exception, and the function does not have a `noexcept` or `throw()` specifier.
*/
predicate functionMayThrow(Function f) {
not f instanceof NonThrowingFunction and
not f instanceof NonCppThrowingFunction and
(not exists(f.getBlock()) or stmtMayThrow(f.getBlock()))
}

Expand Down