Stability fixes in native code #226
Conversation
krabs/krabs/etw.hpp
Outdated
| @@ -349,6 +357,15 @@ namespace krabs { namespace details { | |||
| status = ERROR_SUCCESS; | |||
| trace_.registrationHandle_ = INVALID_PROCESSTRACE_HANDLE; | |||
| } | |||
| catch (trace_already_registered) { | |||
| // We can't StartTrace for "Microsoft-Windows-Security-Auditing" | |||
| // provider. If open/close doesn't throw, then we can continually | |||
There was a problem hiding this comment.
I don't understand this comment. There are other ways that exception can be thrown, right. What do you mean by "we can continually processing events" here?
There was a problem hiding this comment.
In some rare cases StartTrace() return ERROR_ALREADY_EXISTS. ControlTrace(EVENT_TRACE_CONTROL_STOP) inside stop_trace() return ERROR_SUCCESS, but second call of StartTrace() give us ERROR_ALREADY_EXISTS again.
It's rare case so let's rollback now. But I will try to return with POC.
| @@ -96,7 +96,7 @@ namespace krabs { namespace testing { | |||
| // No null terminator | |||
| BYTE guid_data[GUID_STRING_LENGTH_NO_BRACES] = {}; | |||
|
|
|||
| StringFromGUID2(container_id, wide_guid_buffer, GUID_STRING_LENGTH_WITH_BRACES + 1); | |||
| StringFromGUID2(container_id, wide_guid_buffer, sizeof(wide_guid_buffer)); | |||
There was a problem hiding this comment.
This isn't correct. The third parameter should be the count of characters not count of bytes. What symptom were you seeing that made you change this?
There was a problem hiding this comment.
My mistake, rolling back
| @@ -348,7 +354,8 @@ namespace krabs { | |||
| template <typename T> | |||
| trace<T>::~trace() | |||
| { | |||
| stop(); | |||
| // Never throw in destructor | |||
| try { stop(); } catch (...) {} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok. If we add error_callback to the trace object it may solve the user notification issue.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| catch (krabs::could_not_find_schema&) {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| default_callback_ = callback; | ||
| } | ||
|
|
||
| template <typename T> | ||
| template <typename U> | ||
| void trace<T>::set_default_event_callback(U& callback) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Some exceptions handling