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

Stability fixes in native code #226

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Stability fixes in native code #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42a9333
Stability fixes in native code
starix Jan 19, 2024
5118e53
Roll back some changes
starix Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions krabs/krabs/etw.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ namespace krabs { namespace details {
PVOID trace_information,
ULONG information_length)
{
if (trace_.registrationHandle_ == INVALID_PROCESSTRACE_HANDLE)
return;

ULONG status = TraceSetInformation(
trace_.registrationHandle_,
information_class,
Expand Down Expand Up @@ -283,12 +286,16 @@ namespace krabs { namespace details {
template <typename T>
void trace_manager<T>::stop_trace()
{
if (trace_.registrationHandle_ == INVALID_PROCESSTRACE_HANDLE)
return;

trace_info info = fill_trace_info();
ULONG status = ControlTrace(
NULL,
trace_.name_.c_str(),
&info.properties,
EVENT_TRACE_CONTROL_STOP);
trace_.registrationHandle_ = INVALID_PROCESSTRACE_HANDLE;

if (status != ERROR_WMI_INSTANCE_NOT_FOUND) {
error_check_common_conditions(status);
Expand Down
32 changes: 31 additions & 1 deletion krabs/krabs/trace.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ namespace krabs {
*/
void set_default_event_callback(c_provider_callback callback);

template <typename U>
void set_default_event_callback(U& callback);

template <typename U>
void set_default_event_callback(const U& callback);

private:

/**
Expand Down Expand Up @@ -348,7 +354,8 @@ namespace krabs {
template <typename T>
trace<T>::~trace()
{
stop();
// Never throw in destructor
try { stop(); } catch (...) {}
Copy link
Member

Choose a reason for hiding this comment

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

Did you see this throw? I'd need to look to see how trace_manager can throw.
Usually, we don't want to catch everything. There's also implications for SEH callbacks that might have been set up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing an exception out of a destructor is dangerous. If another exception is already propagating the application will terminate.
ControlTrace(EVENT_TRACE_CONTROL_STOP) or CloseTrace() can return error inside stop(). Using SEH is to much.

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 aware that destructors shouldn't throw, however it's not a good pattern to just add "catch all" blocks in all the destructors. Catch what we expect.

Copy link
Contributor Author

@starix starix Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. If we add error_callback to the trace object it may solve the user notification issue.

}

template <typename T>
Expand Down Expand Up @@ -440,7 +447,30 @@ namespace krabs {
template <typename T>
void trace<T>::set_default_event_callback(c_provider_callback callback)
{
// C function pointers don't interact well with std::ref, so we
// overload to take care of this scenario.
default_callback_ = callback;
}

template <typename T>
template <typename U>
void trace<T>::set_default_event_callback(U& callback)
Copy link
Member

Choose a reason for hiding this comment

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

Do you have an example of what you're trying to solve here? I don't think you should need these at all.
If there is a good reason to have this, you should use a single function:

    template <typename T>
    template <typename U>
    void trace<T>::set_default_event_callback(U&& callback)

This can bind to all flavors of parameters correctly.

Copy link
Contributor Author

@starix starix Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you have an example of what you're trying to solve here?

auto fun = [&](const EVENT_RECORD& record) { ... };
krabs::user_trace trace;
trace.set_default_event_callback(fun);

The same approach is used in event_filter::add_on_error_callback or base_provider<T>::add_on_event_callback

{
// std::function copies its argument -- because our callbacks list
// is a list of std::function, this causes problems when a user
// intended for their particular instance to be called.
// std::ref lets us get around this and point to a specific instance
// that they handed us.
default_callback_ = std::ref(callback);
}

template <typename T>
template <typename U>
void trace<T>::set_default_event_callback(const U& callback)
{
// This is where temporaries bind to. Temporaries can't be wrapped in
// a std::ref because they'll go away very quickly. We are forced to
// actually copy these.
default_callback_ = callback;
}
}
19 changes: 11 additions & 8 deletions krabs/krabs/ut.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ namespace krabs { namespace details {
std::vector<BYTE> filterEventIdBuffer;
auto filterEventIdCount = settings.provider_filter_event_ids_.size();

if (filterEventIdCount > 0) {
if (filterEventIdCount > 0 && filterEventIdCount <= MAX_EVENT_FILTER_EVENT_ID_COUNT) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be an error condition that is bubbled up to the caller and then we can either:

  • Return an error condition (throw or return an error code)
  • Use the first MAX_EVENT_FILTER_EVENT_ID_COUNT items

I prefer throwing so it's obvious what went wrong.

Copy link
Contributor Author

@starix starix Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Both variants not ideal. We limit the user's use of krabs filtering. In my case we just "turn off" system pre-filtration. We will get all events (even unneeded) from provider but have a chance to filter them from in callback.

//event filters existing, set native filters using API
parameters.FilterDescCount = 1;
filterDesc.Type = EVENT_FILTER_TYPE_EVENT_ID;
Expand Down Expand Up @@ -237,15 +237,18 @@ namespace krabs { namespace details {

// for MOF providers, EventHeader.Provider is the *Message* GUID
// we need to ask TDH for event information in order to determine the
// correct provider to pass this event to
auto schema = get_event_schema_from_tdh(record);
auto eventInfo = reinterpret_cast<PTRACE_EVENT_INFO>(schema.get());
for (auto& provider : trace.providers_) {
if (eventInfo->ProviderGuid == provider.get().guid_) {
provider.get().on_event(record, trace.context_);
return;
// correct provider to pass this event to. Don't throw in callback.
try
{
auto eventInfo = trace.context_.schema_locator.get_event_schema(record);
for (auto& provider : trace.providers_) {
if (eventInfo->ProviderGuid == provider.get().guid_) {
provider.get().on_event(record, trace.context_);
return;
}
}
}
catch (krabs::could_not_find_schema&) {}
Copy link
Member

Choose a reason for hiding this comment

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

This exception should not be caught. How else will a caller know why the callback is not working?
This layer needs to bubble up errors so that the calling application can know something went wrong.

Copy link
Contributor Author

@starix starix Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

And trace session will close without any chance to resume. In my case we can still handle event in default callback. May be adding an error_callback to trace object is a good idea? We can call it from catch.


if (trace.default_callback_ != nullptr)
trace.default_callback_(record, trace.context_);
Expand Down