Skip to content
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

[release/6.0] Use BOOL (vs. bool) in event pipe qcall signatures #81238

Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jan 26, 2023

Backport of #74389 to release/6.0
Fixes: #81181

There is an API signature mismatch (C++ bool vs. C BOOL , 1 vs. 4 byte) when exporting native event pipe helpers, which causes occasional nondeterministic crashes in apps.

Customer Impact

Applications that use EventSource and published as SingleFile will randomly crash with AccessViolationException.
(the crash may happen in non-singlefile configuration as well, in theory, but was not observed)

Testing

Manual validation of a repro scenario. (app crashes after a minute or so, no crash after the fix)
The same fix was ported to 7.0 a while ago and there is a lot of evidence that the crash no longer happens.

Risk

Low. We are fixing obviously incorrect signatures.

@ghost ghost assigned VSadov Jan 26, 2023
@VSadov VSadov added EventPipe Servicing-consider Issue for next servicing release review and removed EventPipe labels Jan 26, 2023
@VSadov
Copy link
Member Author

VSadov commented Jan 26, 2023

cc: @AaronRobinsonMSFT @agocke

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the DllImport definitions weren't changed in .NET 6, this looks good.

@VSadov
Copy link
Member Author

VSadov commented Jan 26, 2023

Assuming the DllImport definitions weren't changed in .NET 6

The definitions changed in 7.0 slightly and we got more entries like this, so this is not an automated backport, but the fix is essentially the same: bool -> BOOL replacing in signatures in eventpipeinternal.cpp, eventpipeinternal.h

@VSadov
Copy link
Member Author

VSadov commented Jan 26, 2023

The DllImport in 6.0 does not specify bool marshaling explicitly, which means it defaults to 4-byte BOOL.

        //
        // These PInvokes are used as part of the EventPipeEventDispatcher.
        //
        [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
        internal static extern unsafe bool GetSessionInfo(ulong sessionID, EventPipeSessionInfo* pSessionInfo);

        [DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)]
        internal static extern unsafe bool GetNextEvent(ulong sessionID, EventPipeEventInstanceData* pInstance);

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. we will take for consideration in 6.0.x

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 31, 2023
@jeffschwMSFT jeffschwMSFT modified the milestones: 7.0.4, 6.0.15 Jan 31, 2023
@carlossanlop
Copy link
Member

Closing and reopening to rebase the PR on top of the latest infra fixes merged to the branch. The CI had too many failures and the logs are unfortunately gone already.

@carlossanlop
Copy link
Member

Approved by Tactics for 6.0.15.
Signed off by area owner.
No OOB changes needed for this PR.
CI failures are known and unrelated: #81807, #80476, #81851, #81391.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit a744288 into dotnet:release/6.0 Feb 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants