-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 phas3.1 add basic seh edges #18253
base: main
Are you sure you want to change the base?
Brodes/seh flow phas3.1 add basic seh edges #18253
Conversation
… calls. Load and store SEH handling will be addressed in a separate PR.
…n SEH exception could be raised inside a microsoft try except statement, or if a function may always throw an exception.
…he IR implementation and revert consistency check issues. There is a larger issue of how to address erroneous mix and match with SEH and traditional exceptions.
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll
Fixed
Show resolved
Hide resolved
# 28| v28_1(void) = NoOp : | ||
# 18| v18_4(void) = ReturnVoid : | ||
#-----| Goto -> Block 1 | ||
#-----| SEH Exception -> Block 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. There should be both a Goto
and a SEH Exception
edge to Block 4 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm looking into this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jketema this is kinda interesting. The next block is a finally block. I can see a goto successor exists, but it seems like the IR must be optimizing that out since both the exception and the goto would go to the same block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I wasn't clear, the behavior is correct here as far as I can tell.
or | ||
// for now assuming all calls may throw for Seh only | ||
e instanceof SehExceptionEdge and | ||
exists(MicrosoftTryStmt trystmt | trystmt.getAChild*() = expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether you want this at this point, as __finally
and __except
blocks are also children of a MicrosoftTryStmt
. Hence, you'll also get SEH exception edges for calls in those blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had intentionally used the getAChild to include the finally and except since if there is an exception in those, I still want to unwind.
Phase 3.1 in providing SEH support. Adds basic edges for SEH edges, which are only considered for calls. As a result this PR will cause some previous exceptions to switch for C++ exceptions to SEH exceptions (previously all exceptions were considered C++ exceptions). Throws are currently allowed inside SEH exceptions, which to simplify this PR we are not addressing/fixing how those unwind semantics ought to work. A throw in those situations will flow in the IR to the SEH exception handling which is consistent with prior test results.