fix GCHandle free for connection and related fixes and asserts #55303
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55044 For QuicConnection, ensure we only free the state GCHandle when we receive the shutdown event (or if creating/starting the connection fails). This should avoid use-after-free issues for this handle. Note that for QuicStream, we already defer the GCHandle free until after we receive the last event, so we do not have an issue here. We also don't have an issue for QuicListener -- I added a comment explaining why. Also add related asserts, and add some asserts about not throwing during event callbacks. This surfaced one exception on the connection which I fixed by changing Complete to TryComplete on the AcceptQueue. Unfortunately it's also failing on QuicStream, for reasons that are not obvious to me. I filed #55302 to investigate.
|
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #55044 For QuicConnection, ensure we only free the state GCHandle when we receive the shutdown event (or if creating/starting the connection fails). This should avoid use-after-free issues for this handle. Note that for QuicStream, we already defer the GCHandle free until after we receive the last event, so we do not have an issue here. We also don't have an issue for QuicListener -- I added a comment explaining why. Also add related asserts, and add some asserts about not throwing during event callbacks. This surfaced one exception on the connection which I fixed by changing Complete to TryComplete on the AcceptQueue. Unfortunately it's also failing on QuicStream, for reasons that are not obvious to me. I filed #55302 to investigate.
|
| return MsQuicStatusCodes.Success; | ||
| } | ||
|
|
||
| private static uint HandleEventShutdownComplete(State state, ref ConnectionEvent connectionEvent) | ||
| { | ||
| // This is the final event on the connection, so free the GCHandle used by the event callback. | ||
| state.StateGCHandle.Free(); |
There was a problem hiding this comment.
So we either free it in constructor on failure or we expect HandleEventShutdownComplete to always happen, right?
There was a problem hiding this comment.
We either (a) fail to register for events somehow (this can happen if either Open or Start fails), in which case we free it immediately and we'll never get any events.
Or (b) we expect to get the SHUTDOWN_COMPLETE event eventually, and we free it then.
|
Failures appear to be noise |
ghost
locked as resolved and limited conversation to collaborators
Fixes #55044
For QuicConnection, ensure we only free the state GCHandle when we receive the shutdown event (or if creating/starting the connection fails). This should avoid use-after-free issues for this handle.
Note that for QuicStream, we already defer the GCHandle free until after we receive the last event, so we do not have an issue here. We also don't have an issue for QuicListener -- I added a comment explaining why.
Also add related asserts, and add some asserts about not throwing during event callbacks. This surfaced one exception on the connection which I fixed by changing Complete to TryComplete on the AcceptQueue. Unfortunately it's also failing on QuicStream, for reasons that are not obvious to me. I filed #55302 to investigate.
@wfurt @ManickaP @CarnaViire