Brodes/seh flow phase1 throwing models #18014

bdrodes · 2024-11-18T16:15:11Z

Part of a phased approach to an overhaul of how exceptions are modeled and handled in IR, in order to handle both SEH (structured exception handling) and ordinary C++ exceptions. These two types of exceptions have subtly different mechanics and uses. Consequently, users cannot treat all exceptions as the same. E.g., a call to memset can raise an SEH exception but it would not raise an exception with C++ exceptions.

In this PR, I start by creating a new Throwing.qll library to provide metadata for any kind of exception, and a predicate to indicate if it is SEH or CXX. This change impacts models for NonThrowingFunctions, such as (as previously mentioned) memset. This PR, however, does not fully provide support to differentiate SEH and CXX exception handling in the IR. This will be addressed in future PRs.

…EH is not yet part of the IR generation to make this logic work.

cpp/ql/lib/semmle/code/cpp/models/interfaces/NonThrowing.qll

jketema · 2024-11-19T15:58:25Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll

   */
-  abstract predicate mayThrowException(boolean unconditional);
+  final predicate mayRaiseException() { this.raisesException(false) }


Similarly, the mayThrowException predicate will need to be kept for now and marked as deprecated.

Need to rethink this generally for backwards compatibility given the original thing being deprecated is abstract. Stand by...

Ok I decided to move back to the old mechanic. I switched because SEH doesn't really throw an exception per se. But it's fine, and easier to just keep it the way it was. The changes have been made, just waiting for the checks to pass.

I'm seeing failures with some security/CWE tests, but I cannot recreate that locally. @MathiasVP is that a common discrepancy? can you maybe run the test locally as well? Thye all passed for me.

…rowException as well.

…ub.com/microsoft/codeql into brodes/seh_flow_phase1_throwing_models

…ds compatibility.

…ub.com/microsoft/codeql into brodes/seh_flow_phase1_throwing_models

…ption to be consistent with other backward compatibility changes.

cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md

paldepind · 2024-11-20T11:53:07Z

Why do we need to deprecate NonThrowingFunction and add a new NonThrowingFunction that extends ExceptionAnnotation? To me it seems that the things in ExceptionAnnotation are not relevant for NonThrowingFunction? E.g., having a getExceptionType on a function that doesn't throw exceptions.

Perhaps the stuff in ExceptionAnnotation could be moved inside ThrowingFunction, the ExceptionAnnotation could be removed, and NonThrowingFunction could be kept as is. Would that not work?

MathiasVP · 2024-11-20T13:44:07Z

Why do we need to deprecate NonThrowingFunction and add a new NonThrowingFunction that extends ExceptionAnnotation? To me it seems that the things in ExceptionAnnotation are not relevant for NonThrowingFunction? E.g., having a getExceptionType on a function that doesn't throw exceptions.

This is because we want to distinguish between the kinds of exceptions that cannot be thrown. For example, memcpy cannot throw any C++ exceptions. However, when running on a Windows machine it may throw a structured exception (for example, if memcpy dereferences an invalid pointer). So we need to be able to distinguish between "a function that cannot throw a C++ exception, but may throw a structured exception" and "a function that may throw a C++ exception".

I don't care specifically about how we distinguish between those two concepts. I agree that it's a bit unintuitive that you implement a getExceptionType predicate when extending NonThrowing. The interpretation is that this models that the function cannot throw an exception of this specific type (where "type" here refers to whether it's a structured exception or a C++ exception).

Other API suggestions could be:

4 classes: NonThrowingSehException, NonThrowingCppException, ThrowingSehException, ThrowingCppException (where, for the last two classes, you'd need to implement a predicate that specifies whether this is a "must" or a "may" throw).
Get rid of NonThrowing and Throwing and simply have one class ExceptionFunction (similar to the one in this PR which is named ExceptionAnnotation) where you implement a predicate like hasException(Exception e, Option<Boolean>::Option unconditional) (or any other three-valued type) with the specification that:
- A function may throw a C++ exception if hasException(TCxxException(), Option<Boolean>::some(false)) holds
- A function may throw a SEH exception if hasException(TSehException(), Option<Boolean>::some(false)) holds (this one isn't really needed since any function should be assumed to may throw a SEH exception inside a try block)
- A function must throw a C++ exception if hasException(TCxxException(), Option<Boolean>::some(true)) holds
- A function must throw a SEH exception if hasException(TSehException(), Option<Boolean>::some(true)) holds
- A function will not throw a C++ exception if hasException(TCxxException(), Option<Boolean>::none()) holds
- A function will not throw a SEH exception if hasException(TSehException(), Option<Boolean>::none()) holds (I don't imagine this will ever be necessary as this guarantee cannot really be made about any function)

Co-authored-by: Mathias Vorreiter Pedersen <[email protected]>

jketema · 2024-11-21T10:48:09Z

cpp/ql/lib/semmle/code/cpp/models/implementations/Memcpy.qll

@@ -106,6 +106,8 @@ private class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffect
    not this.hasGlobalName(["bcopy", mempcpy(), "memccpy"]) and
    index = this.getParamDest()
  }
+
+  override TCxxException getExceptionType() { any() }


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?

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.

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?

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).

Overhauled the PR, let me know if that works.

Introduce a new class AlwaysSehThrowingFunction which is used to model functions that always throw an SEH exception.

@jketema When you say "always throw an SEH exception" does that mean that the function will always throw an SEH exception when called or that the function can only throw an SEH exception but doesn't necessarily always do so? To me the name sounds like the former but I guess it's actually the latter?

The former: the function will always throw an SEH exception

Thanks. I see. So I guess it's for functions whose purpose is to raise exceptions? Like something along the lines of throw_my_SEH_exception?

Yes: RaiseException, ExRaiseAccessViolation, ExRaiseDatatypeMisalignment.

Alright, thanks :)

…dling.ql

jketema

Some small comments, otherwise LGTM.

jketema · 2024-11-22T10:16:47Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/NonThrowing.qll

+ * A function that is guaranteed to never throw a C++ exception
+ * (distinct from a structured exception handling, SEH, exception).


Suggested change

* A function that is guaranteed to never throw a C++ exception

* (distinct from a structured exception handling, SEH, exception).

* A function that is guaranteed to never throw a C++ exception.

*

* The function may still raise a structured exception handling (SEH) exception.

jketema · 2024-11-22T10:17:43Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll

- * A class that models the exceptional behavior of a function.
+ * A function that is known to raise an exception.


I would leave this as it was, as we're going to deprecate this anyway.

jketema · 2024-11-22T10:17:57Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll

+  ThrowingFunction() { any() }
+


This doesn't seem necessary?

jketema · 2024-11-22T10:19:50Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/Throwing.qll

+/**
+ * A function that is known to raise an exception unconditionally.
+ * The only cases known where this happens is for SEH
+ * (structured exception handling) exceptions.
+ */


Since "SEH" is in the name of the class, I would change this to just:

Suggested change

/**

* A function that is known to raise an exception unconditionally.

* The only cases known where this happens is for SEH

* (structured exception handling) exceptions.

*/

/**

* A function that unconditionally raises a structured exception handling (SEH) exception.

*/

jketema · 2024-11-22T10:20:58Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

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


I think we should be able to remove this predicate completely, but let's leave that as a follow up.

bdrodes added 2 commits November 18, 2024 11:11

Adding model transition to using Throwing.qll.

de05aee

Change log

4b83a45

github-actions bot added C++ documentation labels Nov 18, 2024

bdrodes added 3 commits November 18, 2024 12:48

Merge branch 'main' into brodes/seh_flow_phase1_throwing_models

1c7b5ae

Removing SEH default case for function calls as the logic to handle S…

792231c

…EH is not yet part of the IR generation to make this logic work.

Fixed usage raisesException

1c874d3

bdrodes marked this pull request as ready for review November 19, 2024 15:46

bdrodes requested a review from a team as a code owner November 19, 2024 15:46

Merge branch 'main' into brodes/seh_flow_phase1_throwing_models

5bb765d

jketema reviewed Nov 19, 2024

View reviewed changes

cpp/ql/lib/semmle/code/cpp/models/interfaces/NonThrowing.qll Show resolved Hide resolved

jketema reviewed Nov 19, 2024

View reviewed changes

bdrodes added 7 commits November 19, 2024 12:57

Putting back deleted file, and deprecating instead. Deprecating mayTh…

26d590a

…rowException as well.

Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…

63ddd81

…ub.com/microsoft/codeql into brodes/seh_flow_phase1_throwing_models

bringing back mayThrowException to make it cleaner/easier for backwar…

0784776

…ds compatibility.

Merge branch 'main' into brodes/seh_flow_phase1_throwing_models

ae1ed38

Missing change to 'mayThrowException' in StructuredExceptionHandling.qll

a69daa0

Merge branch 'brodes/seh_flow_phase1_throwing_models' of https://gith…

23485f1

…ub.com/microsoft/codeql into brodes/seh_flow_phase1_throwing_models

Changing terminology back to "throws" vs "rasis" for alwaysThrowsExce…

4e77756

…ption to be consistent with other backward compatibility changes.

MathiasVP reviewed Nov 20, 2024

View reviewed changes

cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md Outdated Show resolved Hide resolved

bdrodes and others added 2 commits November 20, 2024 09:06

Update cpp/ql/lib/change-notes/2024-11-18-throwing-functions.md

69df07e

Co-authored-by: Mathias Vorreiter Pedersen <[email protected]>

Merge branch 'main' into brodes/seh_flow_phase1_throwing_models

6aa7412

jketema reviewed Nov 21, 2024

View reviewed changes

bdrodes added 5 commits November 21, 2024 11:28

Updating PR per review comments. Moving more towards a simplified model.

9b2590e

Delaying deprecation of ThrowingFunction.

4412691

Adding intermediate solution towards deprecating ThrowingFunction

7059fc3

Updating change log

248f1c4

Missing NonCppThrowingFunction changes in Printf.qll

583651b

bdrodes added 2 commits November 21, 2024 12:44

printf formatting.

66cf736

Updating to NonCppThrowingFunction use in IncorrectALlocationErrorHan…

37365c7

…dling.ql

jketema reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brodes/seh flow phase1 throwing models #18014

Brodes/seh flow phase1 throwing models #18014

bdrodes commented Nov 18, 2024 •

edited

Loading

jketema Nov 19, 2024 •

edited

Loading

bdrodes Nov 19, 2024 •

edited

Loading

bdrodes Nov 19, 2024 •

edited

Loading

bdrodes Nov 19, 2024

paldepind commented Nov 20, 2024

MathiasVP commented Nov 20, 2024 •

edited

Loading

jketema Nov 21, 2024

bdrodes Nov 21, 2024 •

edited

Loading

jketema Nov 21, 2024

jketema Nov 21, 2024 •

edited

Loading

bdrodes Nov 21, 2024

paldepind Nov 22, 2024 •

edited

Loading

jketema Nov 22, 2024

paldepind Nov 22, 2024

jketema Nov 22, 2024

paldepind Nov 22, 2024

jketema left a comment

jketema Nov 22, 2024 •

edited

Loading

jketema Nov 22, 2024

jketema Nov 22, 2024

jketema Nov 22, 2024 •

edited

Loading

jketema Nov 22, 2024

		* A function that is guaranteed to never throw a C++ exception
		* (distinct from a structured exception handling, SEH, exception).

		* A class that models the exceptional behavior of a function.
		* A function that is known to raise an exception.

Brodes/seh flow phase1 throwing models #18014

Are you sure you want to change the base?

Brodes/seh flow phase1 throwing models #18014

Conversation

bdrodes commented Nov 18, 2024 • edited Loading

jketema Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

bdrodes Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

bdrodes Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paldepind commented Nov 20, 2024

MathiasVP commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

bdrodes Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paldepind Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema left a comment

Choose a reason for hiding this comment

jketema Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdrodes commented Nov 18, 2024 •

edited

Loading

jketema Nov 19, 2024 •

edited

Loading

bdrodes Nov 19, 2024 •

edited

Loading

bdrodes Nov 19, 2024 •

edited

Loading

MathiasVP commented Nov 20, 2024 •

edited

Loading

bdrodes Nov 21, 2024 •

edited

Loading

jketema Nov 21, 2024 •

edited

Loading

paldepind Nov 22, 2024 •

edited

Loading

jketema Nov 22, 2024 •

edited

Loading

jketema Nov 22, 2024 •

edited

Loading