C++: Generate IR for __try __finally and __try __except
#11761
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll
Fixed
Show fixed
Hide fixed
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll
Fixed
Show fixed
Hide fixed
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll
Fixed
Show fixed
Hide fixed
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll
Fixed
Show fixed
Hide fixed
|
DCA looks fine. |
| # 6| v6_6(void) = ExitFunction : | ||
|
|
||
| # 13| Block 1 | ||
| # 13| r13_1(int) = Constant[0] : |
There was a problem hiding this comment.
What do we do in the case of a normal C++ catch? Is that just as messy as this right now?
There was a problem hiding this comment.
Hopefully this will be fixed automagically once I fix the catch handling you helped me realize was bugged in your other comment.
There was a problem hiding this comment.
This is (partly) fixed in ff6e8a2. Just like for normal C++ exceptions, the catch block isn't actually reached by any gotos (this is something we really should fix in the IR eventually).
This Constant[0] was generated by the exception conditional in __except(0) { ... }, but it didn't continue to the exception block (which was definitely a bug). In ff6e8a2 we now correctly model the behavior specified by https://learn.microsoft.com/en-us/cpp/cpp/try-except-statement?view=msvc-170.
|
Thanks for the comments @jketema. It highlighted some bugs which should now be fixed. I'll start a new DCA run since quite a lot has changed since the first run. |
There was a problem hiding this comment.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll
Fixed
Show fixed
Hide fixed
MathiasVP
force-pushed
the
ir-for-microsoft-try-except-finally
branch
from
94ff797 to
bbf0ec8
Compare
DCA still looks good :phew: |
| # 23| r23_2(int) = Constant[0] : | ||
| # 23| v23_3(void) = Call[ProbeFunction] : func:r23_1, 0:r23_2 | ||
| # 23| mu23_4(unknown) = ^CallSideEffect : ~m? | ||
| # 26| r26_1(glval<unknown>) = FunctionAddress[sink] : |
There was a problem hiding this comment.
I guess technically a new block should start here, or we need a special instruction here, as this point is directly reachable from all the statements in the __try block. Again this is probably now very relevant for dataflow at this point, so I propose to file an issue for this.
There was a problem hiding this comment.
Yeah, I completely agree. There's probably a large room for design here. I'll create an issue for it.
| __finally { | ||
| sink(x); | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at syntax zoo, could we also add something like the following here:
void h(int b) {
__try {
if (b) {
throw 1;
}
}
__except (1) {
sink(x)
}
}There was a problem hiding this comment.
Good call. I've added it in 5fa9681. Since this is a C file I had to "throw" using AfxThrowMemoryException.
There was a problem hiding this comment.
Ahh, but with AfxThrowMemoryException it degenerates to the first case above. Could we add a C++ file with an actual throw?
There was a problem hiding this comment.
Done in a974cb1. It looks like the test behaves as expected: we generate a ThrowValue instruction that correctly transfers flow to the handler block.
There was a problem hiding this comment.
we generate a
ThrowValueinstruction that correctly transfers flow to the handler block.
Is that actually what should be happening? I really don't know...
There was a problem hiding this comment.
There was a problem hiding this comment.
So, this indeed seems to be the correct behaviour.
This PR pickybags on our support for regular
trystatements to implement IR generation for__try __except(and__try __finally).The QL changes are pretty simple, but due to the change in .expected files it's good to review this PR commit-by-commit.