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
google · Aug 13, 2021 · 1edf6d9 · 1edf6d9
1 parent ac1f8ea
commit 1edf6d9
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 84 deletions.
diff --git 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
@@ -16,27 +16,37 @@
 
 #ifdef KERNEL
 #include <libkern/locks.h>
+
 #include "Source/common/SNTLogging.h"
 
 #else
 
-#include <mutex>
 #include <string.h>
 
-#define LOGD(format, ...) // NOP
-#define LOGE(format, ...) // NOP
+#include <mutex>
+
+#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
@@ -51,7 +51,7 @@
 */
 template <typename T>
 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
@@ -17,25 +17,25 @@
 #include <bsm/libbsm.h>
 
 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
 @property(nonatomic) es_auth_result_t result;
 @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
@@ -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 @@ - (void)newClient:(es_client_t *_Nullable *_Nonnull)client
   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 @@ - (es_respond_result_t)respond_auth_result:(const es_message_t *_Nonnull)msg
   return ES_RESPOND_RESULT_SUCCESS;
 };
 
-+ (instancetype)mockEndpointSecurity {
++ (instancetype _Nonnull)mockEndpointSecurity {
   static MockEndpointSecurity *sharedES;
   static dispatch_once_t onceToken;
   dispatch_once(&onceToken, ^{