macos: Add polling to OpenBSM publisher #6436
Conversation
4c269c3
to
c1dc9e6
Compare
|
@uptycs-nishant, I'll take a closer look at waiting in the |
c1dc9e6
to
ea9aafa
Compare
|
I looked at the So there are some distinct changes in this PR:
|
ea9aafa
to
b295b78
Compare
b295b78
to
0000670
Compare
|
Even though TSAN did not warn about a potential race, I wanted to add mutexes because these data members can be accessed from different threads. |
|
I built 6003890, using |
osquery/events/darwin/openbsm.cpp
Outdated
| } | ||
|
|
||
| WriteLock lock(event_ids_mutex_); | ||
| event_ids_ = event_ids; |
There was a problem hiding this comment.
event_ids_ = std::move(event_ids);
There was a problem hiding this comment.
I can make this change but heads up I'm only really worried about optimizing the critical dequeue/parse path. A good follow up on the optimization route is looking at the process_events implementation on macOS and improving the string concatenation. (if you have more time to dedicate here)
| ev_classes.insert("fw"); | ||
| ev_classes.insert("fr"); | ||
| ev_classes.insert("fa"); | ||
| ev_classes.insert("fm"); | ||
| } | ||
|
|
||
| struct au_class_ent* ace = nullptr; |
There was a problem hiding this comment.
How about?
while ((ace = getauclassent()) != nullptr) {
auto it = ev_classes.find(ace->ac_name);
if (it != ev_classes.end()) {
ADD_TO_MASK(&pr_flags, ace->ac_class, AU_PRS_BOTH);
}
}
There was a problem hiding this comment.
Since this only runs once I don't think we need to be super optimized.
There was a problem hiding this comment.
good point, then why changing from std::vector to std::set?
There was a problem hiding this comment.
I guess because we started using set for event_ids and it's the more appropriate container for this code here too, so I figured why not.
There was a problem hiding this comment.
std::vector allows duplicates? set feels more appropriate.
There was a problem hiding this comment.
Which property of set are we using here? We are iterating all the elements here for which vector or array is sufficient, I believe.
6003890
to
6a8ecf5
Compare
| ev_classes.insert("fw"); | ||
| ev_classes.insert("fr"); | ||
| ev_classes.insert("fa"); | ||
| ev_classes.insert("fm"); | ||
| } | ||
|
|
||
| struct au_class_ent* ace = nullptr; |
There was a problem hiding this comment.
std::vector allows duplicates? set feels more appropriate.
| subscriber->tearDown(); | ||
| subscriber->state(EventState::EVENT_NONE); |
There was a problem hiding this comment.
Why invert the order here?
There was a problem hiding this comment.
To be consistent with the other call sites that set state.
I don't think this is the root cause of #6431, but while reviewing the OpenBSM publisher I noticed it can hang osquery if there are no events in the audit queue.
This change applies the same polling interval and error handling as Linux.