search

microsoft / krabsetw

KrabsETW provides a modern C++ wrapper and a .NET wrapper around the low-level ETW trace consumption functions.
Other
589 stars 147 forks source link

Fix for Windows 7 compatibility of group mask feature #110

Closed jrave closed 4 years ago

jrave commented 4 years ago

This should address #109

swannman commented 4 years ago

@jrave could we try the NtSetSystemInformation approach that @jdu2600 recommended on #109?

jrave commented 4 years ago

Note that although EventTraceGroupMaskInformation is valid for querying in version 6.0 and higher, versions before 6.2 reject it for setting. The returned error code is ERROR_NOT_IMPLEMENTED.

That is from https://geoffchappell.com/studies/windows/km/ntoskrnl/api/etw/tracesup/groupmask.htm and means that we could use NtQuerySystemInformation to extract the group mask. However, then we would not be able to update it. So I am not sure that feature can be made to work on Windows 7 at all.

Therefore the graceful fallback here seems like a sane choice to me.

Proposal could be to add an exception to the kernel_provider(const GUID& id, PERFINFO_MASK group_mask) constructor that would throw on Win 7 to make it easier to catch issues due to this.

swannman commented 4 years ago

Thanks @jrave. I like your suggestion - I would prefer a change that throws an exception when an API is called on an unsupported operating system (like Win 7) rather than introducing a separate codepath that can only be tested on a certain downlevel OS. We will never be confident that future changes in this area work on Win 7 without a manual test pass, which I'd like to avoid.

jdu2600 commented 4 years ago

Sorry for the earlier terse comment - I've been out of the 'office'.

Looks like non-trivial investigation would be required to determine if PERFINFO_MASK support is possible on Windows 7 - which isn't likely worth the effort.

I've thrown up the alternate approach for the fix (https://github.com/microsoft/krabsetw/pull/113). The code is simpler - but it relies on the native API...

swannman commented 4 years ago

Fixed by #113