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

Fixing corruption in callbacks introduced by x86 changes #217

Merged
merged 1 commit into from Oct 13, 2021

Conversation

@kylewo
Copy link
Contributor

@kylewo kylewo commented Aug 16, 2021

In my previous PR, when breaking out providerCallbackAdapter into separate 32 and 64 bit implementations, I changed the typing of the state and level arguments erroneously. These parameters should be treated as pointers, not as raw integers. This issue will cause the parameters passed into providerCallback to be corrupted.

Typically this is benign, however it causes a crash in the following scenario:

  • Callbacks are enabled (i.e. someone is listening for the event provider)
  • An event provider was previous registered and unregistered in the same executable.

Under these conditions, the global provider map (in providerglobal.go) will not have an entry at index 0 (the index only ever increases, so the previous registering/unregistering of a provider leaves the 0 slot empty). The corrupted arguments into providerCallback will have an index of 0 instead of the correct index, and then providerCallback (provider.go line 69) will get a null provider and then attempt to update its state, causing a null reference panic.

This was missed in my initial testing, because for the test providers nothing is listening for the events, so the callback never gets called.

@kylewo kylewo requested a review from as a code owner Aug 16, 2021
@kevpar kevpar self-assigned this Sep 9, 2021
@@ -46,7 +46,7 @@ func eventSetInformation(
// for provider notifications. Because Go has trouble with callback arguments of
// different size, it has only pointer-sized arguments, which are then cast to
// the appropriate types when calling providerCallback.
func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr {
Copy link
Member

@kevpar kevpar Sep 16, 2021

Choose a reason for hiding this comment

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

I'm a bit confused as to why we need to change these types. From PENABLECALLBACK it looks like they are ULONG and UCHAR, which should correspond to uint32 and uint8.

Loading

Copy link
Contributor Author

@kylewo kylewo Sep 16, 2021

Choose a reason for hiding this comment

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

So this is actually reverting the types to what they were before my initial changes. The comment above explains why they were that way. Go appears to give us all the callback variables as pointers, so if you leave level as a uint8, then part of the level gets shoved into matchAnyKeyword and everything becomes misaligned, causing the corruption.

Loading

Copy link
Member

@kevpar kevpar Sep 16, 2021

Choose a reason for hiding this comment

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

Ah, I think I see now, this is presumably something to do with how Go's callback code is translating the native args to the Go ABI?

Loading

kevpar
kevpar approved these changes Sep 16, 2021
Copy link
Member

@kevpar kevpar left a comment

LGTM

Loading

@kevpar
Copy link
Member

@kevpar kevpar commented Sep 16, 2021

Oh, one thing. Can you please certify your commit with the following command?

git commit --amend -s --no-edit
git push --force

Loading

Copy link
Contributor

@dcantah dcantah left a comment

lgtm too, barring the same sign-off/certify comment from Kevin. Thanks!

Loading

Signed-off-by: Kyle Wojtaszek <kylewo@microsoft.com>
@kevpar kevpar merged commit 6c24dfa into microsoft:master Oct 13, 2021
4 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants