[release/10.0] Fix collided unwind detection#121626
Conversation
A recent fix that has moved 2nd pass of the exception handling to native code has accidentally broken detectin of collided unwind, that means detection of when exception escapes a catch or exceptionally called finally funclets. Unfortunately none of our coreclr and libraries test exercise the specific case when the problem occurs, which is as follows: * There is a try / catch inside of an outer catch * The try block contains try / finally and the non-exceptionally called finally throws. In this case, the stack walk out of the throwing finally funclet was incorrectly classifed as an exception collision and the catch was skipped. The problem was that the last part of the condition to detect the collided unwind was wrong. In fact, it was always true, so any unwind out of a funclet was considered to be a collision even if the funclet was a finally that was called non-exceptionally. This change fixes it by identifying the collision by the fact that the stack walker has moved from a funclet to native code, which is the only case when the exception is really escaping an exceptionally called funclet. I have also added a regression test for the issue. Close #121578
Co-authored-by: Copilot <[email protected]>
This is the way changes are done. We make them in main and then backport them to the release/10.0 branch. Once they are here, they need to be approved for the servicing release. The problem with this one is that the fix was made too late for the December servicing release, so it will have to wait for the next one. I was hoping it could still make it to the December one, but it was not the case. |
Yes that is how it should always be. It is more your use of milestones, adding pull-requests that will never be included in the milestone like #121625 (merged to |
|
I am not sure I understand the problem. The milestone of the change in the main branch was set to 10.0.x. By that I meant that it should be ported to release/10.0 and go to a servicing release. |
|
So a critical fix that needs to back ported to multiple milestones, then needs to be assigned all milestones? The logical way is creating new pull-request for backport to each milestone. Like this pull-request for milestone 10.x (That links back to the original-pull-request for main-branch). Anyway just ignore me. Should not be telling you how Microsoft should be using milestones on Github :) |
|
Milestone 10.0.x. means "any future milestone for .NET 10". Only the backporting PR to the release/10.0 branch gets the specific milestone (10.0.1, 10.0.2, ...) once it gets approved for that milestone. |
|
And a backported change always ends up going to one milestone, not multiple. Basically, the milestone represents a servicing release. |
|
When looking at all closed/merged pull-requests in 9.0.x and 8.0x Then it doesn't look like they are for the main-branch alone. Have someone forgot to move them into the right release-milestone? But understand now that |
|
Hmm, you are right, it seems that things have changed in this respect and the milestone stays with the .x and it is not updated |
rbhanda
added
Servicing-approved
|
/ba-g Known issues dotnet/dnceng#1883 dotnet/dnceng#3008 dotnet/dnceng#6408 |
5 participants
Backport of #121625 to release/10.0
/cc @janvorli
Customer Impact
The issue causes an exception thrown within a non-exceptionally called finally to not to be handled by a proper catch handler in some cases. It was independently reported by two different customers, the RavenDB being one of them. In the RavenDB case, it was causing a deadlock.
Regression
Testing
The fix was verified using a customer provided minimal repro app. This app was added as a regression test to the coreclr tests suite. The issue was missed because none of our coreclr and libraries tests exercise that specific case.
Risk
Low, it just fixes a part of the condition that was always true and replaces it by a proper check for collided unwind.