Brodes/seh flow overhaul2 #17676

bdrodes · 2024-10-07T17:04:00Z

Overhaul of try/catch to support differentiating SEH vs C++ exception handling in IR generation.

…d updating uses of TExceptionEdge

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

MathiasVP

Some initial comments. I haven't looked through TranslatedFunction, TranslatedInitialization, or TranslatedStmt yet.

cpp/ql/lib/semmle/code/cpp/ir/implementation/EdgeKind.qll

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

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

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

MathiasVP

Some more IR construction comments. I think I've looked through all IR construction files now 🤞

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

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

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

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

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

MathiasVP

Final rounds of comments. Once these are resolved I suggest you pull it out of draft and we can pass the review torch to the GitHub folks 😄

MathiasVP · 2024-10-23T13:23:55Z

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

@@ -3437,8 +3460,7 @@ class TranslatedVarArg extends TranslatedNonConstantExpr {

  final override Instruction getInstructionSuccessorInternal(InstructionTag tag, EdgeKind kind) {
    tag = VarArgsVAListLoadTag() and
-    kind instanceof GotoEdge and
-    result = this.getInstruction(VarArgsArgAddressTag())
+    (kind instanceof GotoEdge and result = this.getInstruction(VarArgsArgAddressTag()))


This parenthesis here is also superfluous now

So much for the formatting tool fixing that automatically.

😅 Yeah, the autoformatter doesn't remove all superfluous parentheses (because that's apparently a super hard problem in general). By the way, it's still complaining about missing autoformat in a couple of places, though:

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll would change by autoformatting. cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedInitialization.qll would change by autoformatting. cpp/ql/lib/semmle/code/cpp/models/implementations/StructuredExceptionHandling.qll would change by autoformatting. cpp/ql/lib/semmle/code/cpp/models/implementations/Strcpy.qll would change by autoformatting.

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

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

MathiasVP · 2024-10-23T13:35:14Z

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

+ * unconditional or non-throwing. IR generation will enforce
+ * the most strict interpretation.


Where does this enforcement happen?

https://github.com/microsoft/codeql/blob/e86c7644d8a39f95f97d24f93c6db403191a9b99/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll#L89

MathiasVP · 2024-10-23T13:35:40Z

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

+ * the most strict interpretation.
+ */
+class DefaultSEHExceptionBehavior extends ThrowingFunction {
+  DefaultSEHExceptionBehavior() { this = any(Function f) }


I don't think we need this charpred as ThrowingFunction is already a Function (which Code Scanning is also telling us 🎉)

Let me ask this. It is currently defined as a function yes, but in my refactor of this concept I had intended that an ExceptionAnnotation can be on any expression. Hence, this char pred exists for that concept, but you are right it is not needed given I didn't think I could swing that concept for this pr. The question is, given I would hope it could generalize, should the char pred be defensive?

I'm fine with leaving it here. Although, I don't think we'll ever generalize ThrowingFunction to not be a function since that would probably be a bit confusing. Rather, if we generalize it to ThrowingExpr I think we'd keep ThrowingFunction as a special case that extends both Function and ThrowingExpr.

I moved the char pred up to ThrowingFunction, an abstract, as that makes more sense, However, removing the char pred does result in a warning. Apparently extending an abstract class without specifying a char pred is a no no? What do you suggest? Define one with "any()"?

I went with this 'any' approach, but depending on your comments I can revert it.

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

bdrodes · 2024-10-23T19:34:41Z

@MathiasVP I realize I need a change log, but previously I put change logs under src/change-notes. For libraries, I see lib/change-notes and a released directory. Do I put it at the same level of released or should there be another sub directory? I'm moving to ready to review while we figure that out.

MathiasVP · 2024-10-24T10:07:55Z

Yeah, the "unreleased" change notes goes in this folder: https://github.com/github/codeql/tree/main/cpp/ql/lib/change-notes

Then, as part of the release process, those files will be moved into the "released" folder.

jketema · 2024-10-24T11:59:49Z

@bdrodes Could you please fix all the QL-for-QL warnings except the redundant import ones?

…oft/codeql into brodes/seh_flow_overhaul2 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

bdrodes · 2024-10-24T13:54:01Z

@jketema or @MathiasVP I'm not sure what is required for the newtype qldocs, here's what's in the code, but it is still complaining:

MathiasVP · 2024-10-24T14:12:40Z

You need to use QLDoc on the branches as well:

/** Structure Exception Handling (SEH) exception */
TSEHException() or
/** C++ exception */
TCxxException()

By the way, I've seen "Structure Exception Handling" in multiple places. They should probably be fixed to be "Structured Exception Handling".

bdrodes · 2024-10-24T14:40:17Z

Good find @MathiasVP . Fixing...

bdrodes · 2024-10-24T14:41:16Z

@jketema , I think that covers your asks for the ql-for-ql with my last push.

jketema · 2024-10-24T14:56:55Z

@jketema , I think that covers your asks for the ql-for-ql with my last push.

I still see quite a large number of them when I look at the changes.

bdrodes · 2024-10-24T15:04:14Z

@jketema , my misunderstanding, I thought I was reviewing the actions warnings. @MathiasVP set me straight where to look. I'll get back to you.

bdrodes · 2024-10-24T17:27:45Z

@jketema , altered all the isSEH to isSeh to be in compliance with convention. I think that got rid of the warnings.

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

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

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

jketema · 2024-10-25T11:29:06Z

Note that this is currently breaking 100s of tests.

jketema · 2024-10-25T11:33:12Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/EdgeKind.qll


 private newtype TEdgeKind =
  TGotoEdge() or // Single successor (including fall-through)
  TTrueEdge() or // 'true' edge of conditional branch
  TFalseEdge() or // 'false' edge of conditional branch
-  TExceptionEdge() or // Thrown exception
+  TExceptionEdge(Boolean isSeh) or // Thrown exception, true for SEH exceptions, false otherwise


What is the reason for doing with with a boolean instead of just having two separate kinds of edges (one for normal exceptions and one for SEH exceptions)?

We want a single edge concept for all exceptions. A problem we have had generally with SEH vs traditional is the concept has been handled by different mechanisms making it error prone to do anything with exceptions as it is easy to forget the 'other' exception.

bdrodes added 5 commits October 1, 2024 15:12

Altering TExceptionEdge to include SEH and regular C++ exceptions, an…

652b74d

…d updating uses of TExceptionEdge

Overhaul of IR exception handling to account for SEH and C++ exceptions.

82cdd02

Fixing bug in translated call edge generation.

a818346

Finalizing a model for throwing and non-throwing functions.

a2ede08

Propagating the exception kind into getExceptionSuccessorInstruction

0ed72d1

github-actions bot added the C++ label Oct 7, 2024

github-advanced-security bot found potential problems Oct 7, 2024

View reviewed changes

updating getNextHandler logic.

e3d0015

MathiasVP reviewed Oct 7, 2024

View reviewed changes

bdrodes added 2 commits October 10, 2024 11:51

Review updates to EdgeKind.qll

7a3ab8c

Review updates.

d0a54a3

MathiasVP reviewed Oct 10, 2024

View reviewed changes

PR review corrections.

fcff607

MathiasVP reviewed Oct 23, 2024

View reviewed changes

bdrodes added 7 commits October 23, 2024 13:56

Formatting.

fdf5ac6

Removing SEH load exceptions for var args and lambda translations

a053519

Updated comments and formatting.

e86c764

Updating comments.

a40343e

Comments and changed char pred (moved up to abstract class)

cc4b280

Formatting.

fb16db2

Typo

781a9b7

bdrodes marked this pull request as ready for review October 23, 2024 19:33

bdrodes requested a review from a team as a code owner October 23, 2024 19:33

Merge branch 'main' into brodes/seh_flow_overhaul2

30810ce

bdrodes added 2 commits October 24, 2024 09:41

adding change log

a498c87

github-actions bot added the documentation label Oct 24, 2024

Comments

61b1fef

Updating docs

f85ba7a

Type in comments.

9dc776e

changing isSEH to isSeh to match conventions.

3f01e4d

github-advanced-security bot found potential problems Oct 24, 2024

View reviewed changes

Style changes.

222c9f2

jketema reviewed Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brodes/seh flow overhaul2 #17676

Brodes/seh flow overhaul2 #17676

bdrodes commented Oct 7, 2024

github-advanced-security bot left a comment

MathiasVP left a comment

MathiasVP left a comment

MathiasVP left a comment

MathiasVP Oct 23, 2024

bdrodes Oct 23, 2024

MathiasVP Oct 23, 2024 •

edited

Loading

bdrodes Oct 23, 2024

MathiasVP Oct 23, 2024

bdrodes Oct 23, 2024

MathiasVP Oct 23, 2024

bdrodes Oct 23, 2024

MathiasVP Oct 23, 2024 •

edited

Loading

bdrodes Oct 23, 2024

bdrodes Oct 23, 2024

bdrodes commented Oct 23, 2024

MathiasVP commented Oct 24, 2024

jketema commented Oct 24, 2024

bdrodes commented Oct 24, 2024

MathiasVP commented Oct 24, 2024

bdrodes commented Oct 24, 2024

bdrodes commented Oct 24, 2024

jketema commented Oct 24, 2024

bdrodes commented Oct 24, 2024

bdrodes commented Oct 24, 2024

jketema commented Oct 25, 2024

jketema Oct 25, 2024

bdrodes Nov 20, 2024

		* unconditional or non-throwing. IR generation will enforce
		* the most strict interpretation.

Brodes/seh flow overhaul2 #17676

Are you sure you want to change the base?

Brodes/seh flow overhaul2 #17676

Conversation

bdrodes commented Oct 7, 2024

github-advanced-security bot left a comment

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasVP Oct 23, 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

Choose a reason for hiding this comment

MathiasVP Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdrodes commented Oct 23, 2024

MathiasVP commented Oct 24, 2024

jketema commented Oct 24, 2024

bdrodes commented Oct 24, 2024

MathiasVP commented Oct 24, 2024

bdrodes commented Oct 24, 2024

bdrodes commented Oct 24, 2024

jketema commented Oct 24, 2024

bdrodes commented Oct 24, 2024

bdrodes commented Oct 24, 2024

jketema commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasVP Oct 23, 2024 •

edited

Loading

MathiasVP Oct 23, 2024 •

edited

Loading