[QUIC] Fix native crashes and heap corruption via "generated-like" interop #74669
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs
Outdated
Show resolved
Hide resolved
|
Can we also make the Also, since we are touching this, what about getting rid of the |
ManickaP
force-pushed
the
quic-crash-fix-v2
branch
from
bc96468
to
0653eb5
Compare
ManickaP
force-pushed
the
quic-crash-fix-v2
branch
from
0653eb5
to
72367aa
Compare
| ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ApiTable->ConnectionOpen( | ||
| MsQuicApi.Api.Registration.QuicHandle, | ||
| ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConnectionOpen( | ||
| MsQuicApi.Api.Registration, | ||
| &NativeCallback, | ||
| (void*)GCHandle.ToIntPtr(context), | ||
| &handle), |
There was a problem hiding this comment.
For the *Open methods we could make the wrapper use out MsQuicContextSafeHandle handle parameter and construct the safe handle inside. What do you think?
There was a problem hiding this comment.
I'll try it out and see if makes sense. If yes, I'll work it in. I like the idea.
There was a problem hiding this comment.
This would be hard to make work automagically (tm) with source generators. But we could introduce manual helpers. Once again, postponing this for a follow up.
We're using it directly for
I'd love that, but as I'll the same reasoning as above, minimize churn and we can address this as a follow up. |
ManickaP
force-pushed
the
quic-crash-fix-v2
branch
from
287b350
to
4667cef
Compare
ManickaP
force-pushed
the
quic-crash-fix-v2
branch
from
4667cef
to
657639f
Compare
|
For follow up, filed: #75009 |
| { | ||
| _sendBuffers.Reset(); | ||
| _sendTcs.TrySetException(exception, final: true); | ||
| if (_sendBuffers.Count > 0 && _sendBuffers.Buffers[0].Buffer != null) |
There was a problem hiding this comment.
I believe this will become a non-issue when #73691 is implemented. I made it a high prio issue for 8.0 since it seems more important now.
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs
Outdated
Show resolved
Hide resolved
| @@ -458,7 +458,6 @@ private unsafe int HandleEventConnected(ref CONNECTED_DATA data) | |||
| _connectedTcs.TrySetResult(); | |||
| return QUIC_STATUS_SUCCESS; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Are these whitespace changes intentional?
There was a problem hiding this comment.
Yes, they are. Either we do have whitespace between Handle... or not, but we should be consistent in all classes (Listener, Connection, Stream). I don't mind going either way, but I mind having them in Connection and nowhere else.
…terop (dotnet#74669) * Send buffers and handles crash fixes * Add generated-like interop * Apply PR feedback from dotnet#74611 * Change asserts * Feedback + moved native methods to their own file * PR feedback Co-authored-by: ManickaP <mapichov@microsoft.com>
…terop (#74669) (#75192) * Send buffers and handles crash fixes * Add generated-like interop * Apply PR feedback from #74611 * Change asserts * Feedback + moved native methods to their own file * PR feedback Co-authored-by: ManickaP <mapichov@microsoft.com> Co-authored-by: ManickaP <mapichov@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Alternative to #74611
We should explore how to generate stuff like that in the future. Relevant PR: #65204
Fixes #72696