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 13 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: minorAnalysis
---
* Removed NonThrowing.qll. Throwing meta-data now part of Throwing.qll. Updated models and IR to use the new Throwing library and predicates.
bdrodes marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
* functions. See `semmle.code.cpp.models.Models` for usage information.
*/

import semmle.code.cpp.Function

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.ArrayFunction
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.DataFlow
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Alias
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.SideEffect
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Taint
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.NonThrowing
.
import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard functions `memcpy`, `memmove` and `bcopy`; and the gcc variant
Expand Down Expand Up @@ -106,6 +106,8 @@
not this.hasGlobalName(["bcopy", mempcpy(), "memccpy"]) and
index = this.getParamDest()
}

override TCxxException getExceptionType() { any() }
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this confusing. Together with the NonThrowing this seems to say that:

This function is non-throwing, but only in the case of C++ exceptions. So it may still throw a SEH exception.

Is that the correct reading?

Copy link
Contributor Author

@bdrodes bdrodes Nov 21, 2024

Choose a reason for hiding this comment

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

That's right. That's what it is saying. It is no longer sufficient to say a function doesn't throw, you have to say how it doesn't throw (which kind of exception doesn't it throw). If they want to say it doesn't throw any you can just return the parent exception type.

The issue we got into with making memcpy nonthrowing is that it is true that it doesn't throw a C++ exception, but it absolutely throws a SEH exception. The mechanics in this PR force users to think about what it is they really want when they say a function throws or doesn't throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

I wonder if this is all somewhat overly complicated. In my understanding the following cases are interesting:

  • C++ exceptions:
    • Functions that may throw (this is the default, but are modelling of this in the IR is limited)
    • Functions that do never throw a C++ exception, i.e., functions marked either as `noexcept or that are C functions (that we model)
  • SEH exceptions:
    • Functions that may throw (this is the default, and we want to improve the modelling in the IR here)
    • Functions that always throw

Is this correct?

Copy link
Contributor

@jketema jketema Nov 21, 2024

Choose a reason for hiding this comment

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

We have a brief meeting. The conclusion was that my assessment above is correct. The proposal is do something simpler:

  • Rename, NonThrowing to something like NonCppThrowingFunction and introduce a deprecated NonThrowing alias.
  • Introduce a new class AlwaysSehThrowingFunction which is used to model functions that always throw an SEH exception.
  • Deprecate the Throwing class.

We should also remove the use of the Throwing class in the models, but we can only do that in the next phase when we update the IR (otherwise the IR breaks).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overhauled the PR, let me know if that works.

}

private string mempcpy() { result = ["mempcpy", "wmempcpy"] }
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
* functions. See `semmle.code.cpp.models.Models` for usage information.
*/

import semmle.code.cpp.Function

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.ArrayFunction
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.DataFlow
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.Alias
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.SideEffect
.
Redundant import, the module is already imported inside
semmle.code.cpp.models.interfaces.NonThrowing
.
import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, AliasFunction,
SideEffectFunction, NonThrowingFunction
Expand Down Expand Up @@ -74,6 +74,8 @@
i = 0 and
if this.hasGlobalName(bzero()) then result = 1 else result = 2
}

override TCxxException getExceptionType() { any() }
}

private string bzero() { result = ["bzero", "explicit_bzero"] }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* A function that is annotated with a `noexcept` specifier (or the equivalent
Expand All @@ -8,4 +8,6 @@ import semmle.code.cpp.models.interfaces.NonThrowing
*/
class NoexceptFunction extends NonThrowingFunction {
NoexceptFunction() { this.isNoExcept() or this.isNoThrow() }

override TCxxException getExceptionType() { any() }
}
12 changes: 11 additions & 1 deletion cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import semmle.code.cpp.models.interfaces.FormattingFunction
import semmle.code.cpp.models.interfaces.Alias
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard functions `printf`, `wprintf` and their glib variants.
Expand All @@ -32,6 +32,8 @@ private class Printf extends FormattingFunction, AliasFunction, NonThrowingFunct
override predicate parameterEscapesOnlyViaReturn(int n) { none() }

override predicate parameterIsAlwaysReturned(int n) { none() }

override TCxxException getExceptionType() { any() }
}

/**
Expand All @@ -50,6 +52,8 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction {
override int getFormatParameterIndex() { result = 1 }

override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -93,6 +97,8 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction {
then result = 4
else result = super.getFirstFormatArgumentIndex()
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -165,6 +171,8 @@ private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction,
// We don't know how many parameters are passed to the function since it's varargs, but they also have read side effects.
i = this.getFormatParameterIndex() and buffer = true
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down Expand Up @@ -215,4 +223,6 @@ private class Syslog extends FormattingFunction, NonThrowingFunction {
override int getFormatParameterIndex() { result = 1 }

override predicate isOutputGlobal() { any() }

override TCxxException getExceptionType() { any() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard function `strcat` and its wide, sized, and Microsoft variants.
Expand Down Expand Up @@ -94,6 +94,8 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
(i = 0 or i = 1) and
buffer = true
}

override TCxxException getExceptionType() { any() }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
import semmle.code.cpp.models.interfaces.DataFlow
import semmle.code.cpp.models.interfaces.Taint
import semmle.code.cpp.models.interfaces.SideEffect
import semmle.code.cpp.models.interfaces.NonThrowing
import semmle.code.cpp.models.interfaces.Throwing

/**
* The standard function `strcpy` and its wide, sized, and Microsoft variants.
Expand Down Expand Up @@ -145,4 +145,6 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
i = this.getParamDest() and
result = this.getParamSize()
}

override TCxxException getExceptionType() { any() }
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import semmle.code.cpp.models.interfaces.Throwing

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

final override predicate mayThrowException(boolean unconditional) { unconditional = true }
override predicate mayThrowException(boolean unconditional) { unconditional = true }

override TSehException getExceptionType() { any() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,7 @@ import semmle.code.cpp.models.Models

/**
* A function that is guaranteed to never throw.
*
* DEPRECATED: use `NonThrowingFunction` in `semmle.code.cpp.models.Models.Interfaces.Throwing` 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.

abstract deprecated class NonThrowingFunction extends Function { }
61 changes: 57 additions & 4 deletions 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,65 @@ import semmle.code.cpp.models.Models
import semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs

/**
* A class that models the exceptional behavior of a function.
* Represents a type of exception,
* either Structured Exception Handling (SEH) or C++ exceptions.
*/
abstract class ThrowingFunction extends Function {
newtype TException =
/** Structured Exception Handling (SEH) exception */
TSehException() or
/** C++ exception */
TCxxException()

/**
* Functions with information about how an exception is thrown or if one is thrown at all.
* If throwing details conflict for the same function, IR is assumed
* to use the most restricted interpretation, meaning taking options
* that stipulate no exception is raised, before the exception is always raised,
* before conditional exceptions.
*
* Annotations must specify if the exception is from SEH (structured exception handling)
* or ordinary c++ exceptions.
*/
abstract private class ExceptionAnnotation extends Function {
/**
* Returns the type of exception this annotation is for,
* either a CPP exception or a STructured Exception Handling (SEH) exception.
*/
abstract TException getExceptionType();

/**
* Holds if the exception type of this annotation is for a Structured Exception Handling (SEH) exception.
*/
final predicate isSeh() { this.getExceptionType() = TSehException() }

/**
* Holds if the exception type of this annotation is for a CPP exception.
*/
final predicate isCxx() { this.getExceptionType() = TCxxException() }
}

/**
* A Function that is known to not throw an exception.
*/
abstract class NonThrowingFunction extends ExceptionAnnotation { }

/**
* A function this is known to raise an exception.
*/
abstract class ThrowingFunction extends ExceptionAnnotation {
ThrowingFunction() { any() }

/**
* Holds if this function may throw an exception during evaluation.
* If `unconditional` is `true` the function always throws an exception.
* Holds if this function may raise an exception during evaluation.
* If `unconditional` is `false` the function may raise, and if `true` the function
* will always raise an exception.
* Do not specify `none()` if no exception is raised, instead use the
* `NonThrowingFunction` class instead.
*/
abstract predicate mayThrowException(boolean unconditional);

/**
* Holds if this function will always raise an exception if called
*/
final predicate alwaysThrowsException() { this.mayThrowException(true) }
}