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.

Sign up for GitHub

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

Jump to bottom

Fixing corruption in callbacks introduced by x86 changes #217

Merged
merged 1 commit into from Oct 13, 2021
Merged

Fixing corruption in callbacks introduced by x86 changes #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/etw/sample/sample.go
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ import (
"bufio"
"fmt"
"os"
"runtime"

"github.com/Microsoft/go-winio/pkg/etw"
"github.com/Microsoft/go-winio/pkg/guid"
Expand All @@ -18,6 +19,8 @@ func callback(sourceID guid.GUID, state etw.ProviderState, level etw.Level, matc
}

func main() {
fmt.Printf("Running on %s/%s\n", runtime.GOOS, runtime.GOARCH)

group, err := guid.FromString("12341234-abcd-abcd-abcd-123412341234")
if err != nil {
logrus.Error(err)
Expand Down
8 changes: 4 additions & 4 deletions pkg/etw/wrapper_32.go
View file
Open in desktop
Expand Up @@ -59,9 +59,9 @@ func eventSetInformation(
// For x86, the matchAny and matchAll keywords need to be assembled from two
// 32-bit integers, because the max size of an argument is uintptr, but those
// two arguments are actually 64-bit integers.
func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword_low uint32, matchAnyKeyword_high uint32, matchAllKeyword_low uint32, matchAllKeyword_high uint32, filterData uintptr, i uintptr) uintptr {
matchAnyKeyword := uint64(matchAnyKeyword_high) << 32 | uint64(matchAnyKeyword_low)
matchAllKeyword := uint64(matchAllKeyword_high) << 32 | uint64(matchAllKeyword_low)
func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint32, matchAnyKeyword_low uint32, matchAnyKeyword_high uint32, matchAllKeyword_low uint32, matchAllKeyword_high uint32, filterData uintptr, i uintptr) uintptr {
matchAnyKeyword := uint64(matchAnyKeyword_high)<<32 | uint64(matchAnyKeyword_low)
matchAllKeyword := uint64(matchAllKeyword_high)<<32 | uint64(matchAllKeyword_low)
providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i)
return 0
}
}
4 changes: 2 additions & 2 deletions pkg/etw/wrapper_64.go
View file
Open in desktop
Expand Up @@ -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

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

func providerCallbackAdapter(sourceID *guid.GUID, state uintptr, level uintptr, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr {
providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i)
return 0
}
}