-
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 phase1 throwing models #18014
base: main
Are you sure you want to change the base?
Changes from 14 commits
de05aee
4b83a45
1c7b5ae
792231c
1c874d3
5bb765d
26d590a
63ddd81
0784776
ae1ed38
a69daa0
23485f1
4e77756
69df07e
6aa7412
9b2590e
4412691
7059fc3
248f1c4
583651b
66cf736
37365c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: deprecated | ||
--- | ||
* The `NonThrowing` class (`semmle.code.cpp.models.interfaces.NonThrowing`) has been deprecated. Please use the `NonThrowing` class from `semmle.code.cpp.models.interfaces.Throwing` instead. |
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 |
---|---|---|
|
@@ -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 { } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
And then it'll be deleted by someone in a PR a year from now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about that after I submitted, fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just do:
and write in the change note that (a) the class is deprecated, and (b) requires a new member to be implemented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or is that too dirty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can definitely do that, yes! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { } |
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 find this confusing. Together with the
NonThrowing
this seems to say that:Is that the correct reading?
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.
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.
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.
Thanks.
I wonder if this is all somewhat overly complicated. In my understanding the following cases are interesting:
Is this correct?
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.
We have a brief meeting. The conclusion was that my assessment above is correct. The proposal is do something simpler:
NonThrowing
to something likeNonCppThrowingFunction
and introduce a deprecatedNonThrowing
alias.AlwaysSehThrowingFunction
which is used to model functions that always throw an SEH exception.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).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.
Overhauled the PR, let me know if that works.