From 1edf6d920053646dad8cee2ccf3fa90e461ccaf9 Mon Sep 17 00:00:00 2001 From: Kent Ma Date: Fri, 13 Aug 2021 11:31:41 -0400 Subject: [PATCH] Enable -Werror -Wall on our build rules (#572) * Reorder init lists for -Wreorder-init-lists * Add nullability annotations to the rest of EndpointSecurityTestUtil * Added fake uses for -Wunused-variable * Corrected signed/unsigned int conversions in SNTPrefixTree * Explicitly convert implicit conversions in Santacache * Set bazelrc to -Werror -Wall --- .bazelrc | 3 + Source/common/SNTPrefixTree.cc | 47 ++++++---- Source/common/SantaCache.h | 6 +- .../EventProviders/EndpointSecurityTestUtil.h | 16 ++-- .../EndpointSecurityTestUtil.mm | 12 +-- .../SNTEndpointSecurityManagerTest.mm | 88 ++++++++++--------- Source/santad/SNTApplicationTest.m | 16 ++-- 7 files changed, 104 insertions(+), 84 deletions(-) diff --git a/.bazelrc b/.bazelrc index e0828438..c1791a64 100644 --- a/.bazelrc +++ b/.bazelrc @@ -1,2 +1,5 @@ build --apple_generate_dsym --define=apple.propagate_embedded_extra_outputs=yes build --host_force_python=PY2 + +build --copt=-Werror +build --copt=-Wall diff --git a/Source/common/SNTPrefixTree.cc b/Source/common/SNTPrefixTree.cc index e73f3b0a..f6c07cdf 100644 --- a/Source/common/SNTPrefixTree.cc +++ b/Source/common/SNTPrefixTree.cc @@ -16,27 +16,37 @@ #ifdef KERNEL #include + #include "Source/common/SNTLogging.h" #else -#include #include -#define LOGD(format, ...) // NOP -#define LOGE(format, ...) // NOP +#include + +#define LOGD(format, ...) // NOP +#define LOGE(format, ...) // NOP #define lck_rw_lock_shared(l) pthread_rwlock_rdlock(&l) #define lck_rw_unlock_shared(l) pthread_rwlock_unlock(&l) #define lck_rw_lock_exclusive(l) pthread_rwlock_wrlock(&l) #define lck_rw_unlock_exclusive(l) pthread_rwlock_unlock(&l) -#define lck_rw_lock_shared_to_exclusive(l) ({ pthread_rwlock_unlock(&l); false; }) -#define lck_rw_lock_exclusive_to_shared(l) ({ pthread_rwlock_unlock(&l); pthread_rwlock_rdlock(&l); }) +#define lck_rw_lock_shared_to_exclusive(l) \ + ({ \ + pthread_rwlock_unlock(&l); \ + false; \ + }) +#define lck_rw_lock_exclusive_to_shared(l) \ + ({ \ + pthread_rwlock_unlock(&l); \ + pthread_rwlock_rdlock(&l); \ + }) #define lck_mtx_lock(l) l->lock() #define lck_mtx_unlock(l) l->unlock() -#endif // KERNEL +#endif // KERNEL SNTPrefixTree::SNTPrefixTree(uint32_t max_nodes) { root_ = new SantaPrefixNode(); @@ -45,7 +55,8 @@ SNTPrefixTree::SNTPrefixTree(uint32_t max_nodes) { #ifdef KERNEL spt_lock_grp_attr_ = lck_grp_attr_alloc_init(); - spt_lock_grp_ = lck_grp_alloc_init("santa-prefix-tree-lock", spt_lock_grp_attr_); + spt_lock_grp_ = + lck_grp_alloc_init("santa-prefix-tree-lock", spt_lock_grp_attr_); spt_lock_attr_ = lck_attr_alloc_init(); spt_lock_ = lck_rw_alloc_init(spt_lock_grp_, spt_lock_attr_); @@ -57,9 +68,9 @@ SNTPrefixTree::SNTPrefixTree(uint32_t max_nodes) { } IOReturn SNTPrefixTree::AddPrefix(const char *prefix, uint64_t *node_count) { - // Serialize requests to AddPrefix. Otherwise one AddPrefix thread could overwrite whole - // branches of another. HasPrefix is still free to read the tree, until AddPrefix needs to - // modify it. + // Serialize requests to AddPrefix. Otherwise one AddPrefix thread could + // overwrite whole branches of another. HasPrefix is still free to read the + // tree, until AddPrefix needs to modify it. lck_mtx_lock(spt_add_lock_); // Don't allow an empty prefix. @@ -74,13 +85,13 @@ IOReturn SNTPrefixTree::AddPrefix(const char *prefix, uint64_t *node_count) { lck_rw_lock_shared(spt_lock_); SantaPrefixNode *node = root_; - for (int i = 0; i < len; ++i) { + for (size_t i = 0; i < len; ++i) { // If there is a node in the path that is considered a prefix, stop adding. // For our purposes we only care about the shortest path that matches. if (node->isPrefix) break; // Only process a byte at a time. - uint8_t value = prefix[i]; + uint8_t value = (uint8_t)prefix[i]; // Create the child if it does not exist. if (!node->children[value]) { @@ -103,7 +114,7 @@ IOReturn SNTPrefixTree::AddPrefix(const char *prefix, uint64_t *node_count) { // Create the rest of the prefix. while (i < len) { - value = prefix[i++]; + value = (uint8_t)prefix[i++]; SantaPrefixNode *new_node = new SantaPrefixNode(); node->children[value] = new_node; @@ -159,7 +170,8 @@ bool SNTPrefixTree::HasPrefix(const char *string) { SantaPrefixNode *node = root_; - // A well formed tree will always break this loop. Even if string doesn't terminate. + // A well formed tree will always break this loop. Even if string doesn't + // terminate. const char *p = string; while (*p) { // Only process a byte at a time. @@ -193,8 +205,8 @@ void SNTPrefixTree::Reset() { void SNTPrefixTree::PruneNode(SantaPrefixNode *target) { if (!target) return; - // For deep trees, a recursive approach will generate too many stack frames. Make a "stack" - // and walk the tree. + // For deep trees, a recursive approach will generate too many stack frames. + // Make a "stack" and walk the tree. auto stack = new SantaPrefixNode *[node_count_ + 1]; if (!stack) { LOGE("Unable to prune tree!"); @@ -206,7 +218,8 @@ void SNTPrefixTree::PruneNode(SantaPrefixNode *target) { // Seed the "stack" with a starting node. stack[count++] = target; - // Start at the target node and walk the tree to find and delete all the sub-nodes. + // Start at the target node and walk the tree to find and delete all the + // sub-nodes. while (count) { auto node = stack[--count]; diff --git a/Source/common/SantaCache.h b/Source/common/SantaCache.h index ce1747eb..3d76af01 100644 --- a/Source/common/SantaCache.h +++ b/Source/common/SantaCache.h @@ -51,7 +51,7 @@ */ template uint64_t SantaCacheHasher(T const &t) { - return t * 11400714819323198549UL; + return (uint64_t)t * 11400714819323198549UL; }; /** @@ -80,7 +80,7 @@ class SantaCache { usage. Cannot be higher than 64 to try and ensure buckets don't overflow. */ SantaCache(uint64_t maximum_size = 10000, uint8_t per_bucket = 5) { - if (unlikely(per_bucket > maximum_size)) per_bucket = maximum_size; + if (unlikely(per_bucket > maximum_size)) per_bucket = (uint8_t)maximum_size; if (unlikely(per_bucket < 1)) per_bucket = 1; if (unlikely(per_bucket > 64)) per_bucket = 64; max_size_ = maximum_size; @@ -214,7 +214,7 @@ class SantaCache { } uint16_t size = *array_size; - if (start + size > bucket_count_) size = bucket_count_ - start; + if (start + size > bucket_count_) size = (uint16_t)(bucket_count_ - start); for (uint16_t i = 0; i < size; ++i) { uint16_t count = 0; diff --git a/Source/santad/EventProviders/EndpointSecurityTestUtil.h b/Source/santad/EventProviders/EndpointSecurityTestUtil.h index e176546c..7626247d 100644 --- a/Source/santad/EventProviders/EndpointSecurityTestUtil.h +++ b/Source/santad/EventProviders/EndpointSecurityTestUtil.h @@ -17,7 +17,7 @@ #include CF_EXTERN_C_BEGIN -es_string_token_t MakeStringToken(const NSString *s); +es_string_token_t MakeStringToken(const NSString *_Nonnull s); CF_EXTERN_C_END @interface ESResponse : NSObject @@ -25,17 +25,17 @@ CF_EXTERN_C_END @property(nonatomic) bool shouldCache; @end -typedef void (^ESCallback)(ESResponse *); +typedef void (^ESCallback)(ESResponse *_Nonnull); // Singleton wrapper around all of the kernel-level EndpointSecurity framework functions. @interface MockEndpointSecurity : NSObject @property BOOL subscribed; - (void)reset; -- (void)registerResponseCallback:(ESCallback)callback; -- (void)triggerHandler:(es_message_t *)msg; +- (void)registerResponseCallback:(ESCallback _Nonnull)callback; +- (void)triggerHandler:(es_message_t *_Nonnull)msg; /// Retrieve an initialized singleton MockEndpointSecurity object -+ (instancetype)mockEndpointSecurity; ++ (instancetype _Nonnull)mockEndpointSecurity; @end API_UNAVAILABLE(ios, tvos, watchos) @@ -64,6 +64,6 @@ API_AVAILABLE(macos(10.15)) API_UNAVAILABLE(ios, tvos, watchos) es_return_t es_delete_client(es_client_t *_Nullable client); API_AVAILABLE(macos(10.15)) -API_UNAVAILABLE(ios, tvos, watchos) es_return_t - es_unsubscribe(es_client_t *_Nonnull client, const es_event_type_t *_Nonnull events, - uint32_t event_count); +API_UNAVAILABLE(ios, tvos, watchos) +es_return_t es_unsubscribe(es_client_t *_Nonnull client, const es_event_type_t *_Nonnull events, + uint32_t event_count); diff --git a/Source/santad/EventProviders/EndpointSecurityTestUtil.mm b/Source/santad/EventProviders/EndpointSecurityTestUtil.mm index 1c661cfa..6feb6ed4 100644 --- a/Source/santad/EventProviders/EndpointSecurityTestUtil.mm +++ b/Source/santad/EventProviders/EndpointSecurityTestUtil.mm @@ -19,10 +19,10 @@ #import "Source/santad/EventProviders/EndpointSecurityTestUtil.h" CF_EXTERN_C_BEGIN -es_string_token_t MakeStringToken(const NSString *s) { +es_string_token_t MakeStringToken(const NSString *_Nonnull s) { return (es_string_token_t){ - .data = [s UTF8String], .length = [s length], + .data = [s UTF8String], }; } CF_EXTERN_C_END @@ -65,11 +65,11 @@ CF_EXTERN_C_END self.handler = handler; } -- (void)triggerHandler:(es_message_t *)msg { - self.handler((__bridge es_client_t *)self.client, msg); +- (void)triggerHandler:(es_message_t *_Nonnull)msg { + self.handler((__bridge es_client_t *_Nullable)self.client, msg); } -- (void)registerResponseCallback:(ESCallback)callback { +- (void)registerResponseCallback:(ESCallback _Nonnull)callback { @synchronized(self) { [self.responseCallbacks addObject:callback]; } @@ -89,7 +89,7 @@ CF_EXTERN_C_END return ES_RESPOND_RESULT_SUCCESS; }; -+ (instancetype)mockEndpointSecurity { ++ (instancetype _Nonnull)mockEndpointSecurity { static MockEndpointSecurity *sharedES; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ diff --git a/Source/santad/EventProviders/SNTEndpointSecurityManagerTest.mm b/Source/santad/EventProviders/SNTEndpointSecurityManagerTest.mm index b7ff0300..e1cc54e7 100644 --- a/Source/santad/EventProviders/SNTEndpointSecurityManagerTest.mm +++ b/Source/santad/EventProviders/SNTEndpointSecurityManagerTest.mm @@ -42,6 +42,7 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; MockEndpointSecurity *mockES = [MockEndpointSecurity mockEndpointSecurity]; [mockES reset]; SNTEndpointSecurityManager *snt = [[SNTEndpointSecurityManager alloc] init]; + (void)snt; // Make it appear used for the sake of -Wunused-variable XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for santa's Auth dispatch queue"]; @@ -60,26 +61,26 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; es_file_t dbFile = {.path = MakeStringToken(kEventsDBPath)}; es_file_t otherBinary = {.path = MakeStringToken(@"somebinary")}; es_process_t proc = { - .executable = &otherBinary, - .is_es_client = false, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = false, + .executable = &otherBinary, }; es_event_unlink_t unlink_event = {.target = &dbFile}; es_events_t event = {.unlink = unlink_event}; es_message_t m = { .version = 4, - .event_type = ES_EVENT_TYPE_AUTH_UNLINK, - .event = event, .mach_time = 1234, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = 1234, .process = &proc, .seq_num = 1337, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_UNLINK, + .event = event, }; [mockES triggerHandler:&m]; @@ -105,6 +106,7 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; MockEndpointSecurity *mockES = [MockEndpointSecurity mockEndpointSecurity]; [mockES reset]; SNTEndpointSecurityManager *snt = [[SNTEndpointSecurityManager alloc] init]; + (void)snt; // Make it appear used for the sake of -Wunused-variable XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for response from ES"]; __block ESResponse *got; @@ -116,26 +118,26 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; es_file_t dbFile = {.path = MakeStringToken(testPath)}; es_file_t otherBinary = {.path = MakeStringToken(@"somebinary")}; es_process_t proc = { - .executable = &otherBinary, - .is_es_client = false, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = false, + .executable = &otherBinary, }; es_event_unlink_t unlink_event = {.target = &dbFile}; es_events_t event = {.unlink = unlink_event}; es_message_t m = { .version = 4, - .event_type = ES_EVENT_TYPE_AUTH_UNLINK, - .event = event, .mach_time = DISPATCH_TIME_NOW, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = DISPATCH_TIME_FOREVER, .process = &proc, .seq_num = 1337, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_UNLINK, + .event = event, }; [mockES triggerHandler:&m]; @@ -155,9 +157,9 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; MockEndpointSecurity *mockES = [MockEndpointSecurity mockEndpointSecurity]; [mockES reset]; SNTEndpointSecurityManager *snt = [[SNTEndpointSecurityManager alloc] init]; + (void)snt; // Make it appear used for the sake of -Wunused-variable XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for response from ES"]; - __block ESResponse *got; [mockES registerResponseCallback:^(ESResponse *r) { got = r; @@ -167,26 +169,26 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; es_file_t dbFile = {.path = MakeStringToken(@"/some/other/path")}; es_file_t otherBinary = {.path = MakeStringToken(@"somebinary")}; es_process_t proc = { - .executable = &otherBinary, - .is_es_client = true, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = true, + .executable = &otherBinary, }; es_event_unlink_t unlink_event = {.target = &dbFile}; es_events_t event = {.unlink = unlink_event}; es_message_t m = { .version = 4, - .event_type = ES_EVENT_TYPE_AUTH_UNLINK, - .event = event, .mach_time = DISPATCH_TIME_NOW, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = DISPATCH_TIME_FOREVER, .process = &proc, .seq_num = 1337, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_UNLINK, + .event = event, }; [mockES triggerHandler:&m]; @@ -205,6 +207,7 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; MockEndpointSecurity *mockES = [MockEndpointSecurity mockEndpointSecurity]; [mockES reset]; SNTEndpointSecurityManager *snt = [[SNTEndpointSecurityManager alloc] init]; + (void)snt; // Make it appear used for the sake of -Wunused-variable XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for response from ES"]; __block ESResponse *got; @@ -216,33 +219,33 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; es_file_t dbFile = {.path = MakeStringToken(testPath)}; es_event_rename_t renameEvent = { - .destination_type = ES_DESTINATION_TYPE_EXISTING_FILE, .source = &otherFile, + .destination_type = ES_DESTINATION_TYPE_EXISTING_FILE, .destination = {.existing_file = &dbFile}, }; es_file_t otherBinary = {.path = MakeStringToken(@"somebinary")}; es_process_t proc = { - .executable = &otherBinary, - .is_es_client = false, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = false, + .executable = &otherBinary, }; es_events_t event = {.rename = renameEvent}; es_message_t m = { .version = 4, - .event_type = ES_EVENT_TYPE_AUTH_RENAME, - .event = event, .mach_time = DISPATCH_TIME_NOW, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = DISPATCH_TIME_FOREVER, .process = &proc, .seq_num = 1337, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_RENAME, + .event = event, }; [mockES triggerHandler:&m]; @@ -264,6 +267,7 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; MockEndpointSecurity *mockES = [MockEndpointSecurity mockEndpointSecurity]; [mockES reset]; SNTEndpointSecurityManager *snt = [[SNTEndpointSecurityManager alloc] init]; + (void)snt; // Make it appear used for the sake of -Wunused-variable XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for response from ES"]; __block ESResponse *got; @@ -289,26 +293,26 @@ const NSString *const kRulesDBPath = @"/private/var/db/santa/rules.db"; es_file_t otherBinary = {.path = MakeStringToken(@"somebinary")}; es_process_t proc = { - .executable = &otherBinary, - .is_es_client = false, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = false, + .executable = &otherBinary, }; es_events_t event = {.rename = renameEvent}; es_message_t m = { .version = 4, - .event_type = ES_EVENT_TYPE_AUTH_RENAME, - .event = event, .mach_time = DISPATCH_TIME_NOW, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = DISPATCH_TIME_FOREVER, .process = &proc, .seq_num = 1337, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_RENAME, + .event = event, }; [mockES triggerHandler:&m]; diff --git a/Source/santad/SNTApplicationTest.m b/Source/santad/SNTApplicationTest.m index 16f9dab0..f375f9ea 100644 --- a/Source/santad/SNTApplicationTest.m +++ b/Source/santad/SNTApplicationTest.m @@ -85,14 +85,14 @@ NSString *binaryPath = [NSString pathWithComponents:@[ testPath, binaryName ]]; es_file_t binary = {.path = MakeStringToken(binaryPath)}; es_process_t proc = { - .executable = &binary, - .is_es_client = false, - .codesigning_flags = 570509313, - .session_id = 12345, - .group_id = 12345, .ppid = 12345, .original_ppid = 12345, + .group_id = 12345, + .session_id = 12345, + .codesigning_flags = 570509313, .is_platform_binary = false, + .is_es_client = false, + .executable = &binary, }; es_event_exec_t exec_event = { .target = &proc, @@ -101,12 +101,12 @@ es_message_t m = { .version = 4, .mach_time = 181576143417379, - .event_type = ES_EVENT_TYPE_AUTH_EXEC, - .event = event, - .action_type = ES_ACTION_TYPE_AUTH, .deadline = DISPATCH_TIME_FOREVER, .process = &proc, .seq_num = 1, + .action_type = ES_ACTION_TYPE_AUTH, + .event_type = ES_EVENT_TYPE_AUTH_EXEC, + .event = event, }; [mockES triggerHandler:&m];