Plug a fd leak, add a bunch of warnings and fix minor things (#191)

- Get_auxiliary_vector_base() leaks an fd as it never calls fclose() - Fix a bunch of signed vs unsigned - Use ssize_t and size_t where deemed - Avoid void * arithmetic, while gcc and clang capitulated and implicit cast to char *, it's still undef behavior by the standard. - Declare some remaining prototypes as void - Add the following to CFLAGS only on our code (non-GPL/*) - Consistently use NLMSG_TAIL, I think there is one remaining bug where one caller doesn't align the length up, but I'd address that in a separate PR, I don't this diff to be too invasive.
elastic · May 6, 2024 · 2253790 · 2253790
1 parent 62d0aff
commit 2253790
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 37 deletions.
diff --git a/non-GPL/CMakeLists.txt b/non-GPL/CMakeLists.txt
@@ -4,5 +4,27 @@
 # or more contributor license agreements. Licensed under the Elastic License 2.0;
 # you may not use this file except in compliance with the Elastic License 2.0.
 
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wextra")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wchar-subscripts")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wcomment")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wformat")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wformat-security")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wimplicit")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Winline")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wmissing-declarations")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wmissing-prototypes")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wparentheses")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wpointer-arith")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wreturn-type")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wshadow")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wsign-compare")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wstrict-prototypes")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wswitch")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wtrigraphs")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wuninitialized")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wunused")
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-unused-parameter")
+
 add_subdirectory(Events)
 add_subdirectory(HostIsolation)
diff --git a/non-GPL/Events/EventsTrace/EventsTrace.c b/non-GPL/Events/EventsTrace/EventsTrace.c
@@ -171,32 +171,32 @@ static void sig_int(int signo)
     fprintf(stdout, "Received SIGINT, exiting...\n");
 }
 
-static void out_comma()
+static void out_comma(void)
 {
     printf(",");
 }
 
-static void out_newline()
+static void out_newline(void)
 {
     printf("\n");
 }
 
-static void out_array_start()
+static void out_array_start(void)
 {
     printf("[");
 }
 
-static void out_array_end()
+static void out_array_end(void)
 {
     printf("]");
 }
 
-static void out_object_start()
+static void out_object_start(void)
 {
     printf("{");
 }
 
-static void out_object_end()
+static void out_object_end(void)
 {
     printf("}");
 }

diff --git a/non-GPL/Events/Lib/EbpfEvents.c b/non-GPL/Events/Lib/EbpfEvents.c
@@ -111,13 +111,13 @@ static int resolve_btf_field_off(struct btf *btf, const char *struct_name, const
     return ret;
 }
 
-const struct btf_type *resolve_btf_type_by_func(struct btf *btf, const char *func)
+static const struct btf_type *resolve_btf_type_by_func(struct btf *btf, const char *func)
 {
     if (func == NULL) {
         goto out;
     }
 
-    for (int i = 0; i < btf__type_cnt(btf); i++) {
+    for (u_int i = 0; i < btf__type_cnt(btf); i++) {
         int btf_type = btf__resolve_type(btf, i);
         if (btf_type < 0)
             continue;
@@ -406,7 +406,7 @@ static inline int probe_set_autoload(struct btf *btf, struct EventProbe_bpf *obj
     return err;
 }
 
-static bool system_has_bpf_tramp()
+static bool system_has_bpf_tramp(void)
 {
     /*
      * This is somewhat-fragile but as far as I can see, is the most robust
@@ -498,7 +498,7 @@ static bool system_has_bpf_tramp()
     return ret;
 }
 
-static uint64_t detect_system_features()
+static uint64_t detect_system_features(void)
 {
     uint64_t features = 0;
 

diff --git a/non-GPL/HostIsolation/Lib/Common.h b/non-GPL/HostIsolation/Lib/Common.h
@@ -63,7 +63,7 @@ void ebpf_log(const char *format, ...);
  * @brief Returns the default log function used by the library
  * @return
  */
-libbpf_print_fn_t ebpf_default_log_func();
+libbpf_print_fn_t ebpf_default_log_func(void);
 
 /**
  * @brief Set a custom log function to be used by the eBPF library and libbpf

diff --git a/non-GPL/HostIsolation/Lib/KprobeLoader.c b/non-GPL/HostIsolation/Lib/KprobeLoader.c
@@ -79,7 +79,7 @@ static unsigned int find_version_note(unsigned long base)
     return version_code;
 }
 
-static unsigned long get_auxiliary_vector_base(int at_key)
+static unsigned long get_auxiliary_vector_base(unsigned long at_key)
 {
     unsigned long base = 0;
 
@@ -88,19 +88,17 @@ static unsigned long get_auxiliary_vector_base(int at_key)
     return base;
 #else
     FILE *f = NULL;
-    int err = 0;
 
     f = fopen("/proc/self/auxv", "r");
     if (!f) {
-        err = -errno;
-        ebpf_log("failed to open /proc/self/auxv: %d\n", err);
+        ebpf_log("failed to open /proc/self/auxv: %d\n", -errno);
         return 0;
     }
 
     while (true) {
         unsigned long key   = 0;
         unsigned long value = 0;
-        int ret             = -1;
+        size_t ret          = 0;
         ret                 = fread(&key, sizeof(key), 1, f);
         if (ret != 1)
             break;
@@ -111,9 +109,10 @@ static unsigned long get_auxiliary_vector_base(int at_key)
             break;
         if (key == at_key) {
             base = value;
-            return base;
+            break;
         }
     }
+    fclose(f);
     return base;
 #endif
 }

diff --git a/non-GPL/HostIsolation/Lib/TcLoader.c b/non-GPL/HostIsolation/Lib/TcLoader.c
@@ -55,11 +55,11 @@ enum {
     __TCA_BPF_MAX,
 };
 
-#define NLMSG_TAIL(nmsg) ((struct rtattr *)(((void *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len)))
+#define NLMSG_TAIL(nmsg) ((struct rtattr *)(((char *)(nmsg)) + NLMSG_ALIGN((nmsg)->nlmsg_len)))
 
-static int attr_put(struct nlmsghdr *n, int max, int type, const void *buf, int attr_len)
+static int attr_put(struct nlmsghdr *n, size_t max, int type, const void *buf, size_t attr_len)
 {
-    int len            = RTA_LENGTH(attr_len);
+    size_t len         = RTA_LENGTH(attr_len);
     struct rtattr *rta = NULL;
     int rv             = -1;
 
@@ -83,12 +83,12 @@ static int attr_put(struct nlmsghdr *n, int max, int type, const void *buf, int
     return rv;
 }
 
-static int attr_put_32(struct nlmsghdr *n, int max, int type, __u32 data)
+static int attr_put_32(struct nlmsghdr *n, size_t max, int type, __u32 data)
 {
     return attr_put(n, max, type, &data, sizeof(__u32));
 }
 
-static int attr_put_str(struct nlmsghdr *n, int max, int type, const char *s)
+static int attr_put_str(struct nlmsghdr *n, size_t max, int type, const char *s)
 {
     return attr_put(n, max, type, s, strlen(s) + 1);
 }
@@ -185,7 +185,7 @@ static int rtnetlink_recv(int fd, struct msghdr *msg, char **answer)
 {
     struct iovec *iov = NULL;
     char *buf         = NULL;
-    int len           = 0;
+    ssize_t len       = 0;
     int rv            = 0;
 
     if (!msg) {
@@ -260,7 +260,7 @@ static int rtnetlink_send(struct rtnetlink_handle *rtnl, struct nlmsghdr *nlmsg)
 
     unsigned int seq   = 0;
     struct nlmsghdr *h = NULL;
-    int recv_len       = 0;
+    ssize_t recv_len   = 0;
     char *buf          = NULL;
     int rv             = -1;
 
@@ -299,9 +299,9 @@ static int rtnetlink_send(struct rtnetlink_handle *rtnl, struct nlmsghdr *nlmsg)
         goto out;
     }
 
-    for (h = (struct nlmsghdr *)buf; recv_len >= sizeof(*h);) {
-        int len = h->nlmsg_len;
-        int l   = len - sizeof(*h);
+    for (h = (struct nlmsghdr *)buf; recv_len >= (ssize_t)sizeof(*h);) {
+        ssize_t len = h->nlmsg_len;
+        ssize_t l   = len - sizeof(*h);
 
         if (l < 0 || len > recv_len) {
             if (msg.msg_flags & MSG_TRUNC) {
@@ -327,7 +327,7 @@ static int rtnetlink_send(struct rtnetlink_handle *rtnl, struct nlmsghdr *nlmsg)
             struct nlmsgerr *err = (struct nlmsgerr *)NLMSG_DATA(h);
             int error            = err->error;
 
-            if (l < sizeof(struct nlmsgerr)) {
+            if (l < (ssize_t)sizeof(struct nlmsgerr)) {
                 ebpf_log("error truncated\n");
                 rv = -1;
                 goto out;
@@ -462,7 +462,7 @@ int netlink_filter_add_begin(struct netlink_ctx *ctx, const char *ifname)
     }
 
     n         = &ctx->msg.n;
-    ctx->tail = (struct rtattr *)(((void *)n) + NLMSG_ALIGN(n->nlmsg_len));
+    ctx->tail = NLMSG_TAIL(n);
     attr_put(n, MAX_MSG, TCA_OPTIONS, NULL, 0);
 
     rv = 0;
@@ -487,7 +487,7 @@ int netlink_filter_add_end(int fd, struct netlink_ctx *ctx, const char *ebpf_obj
     memset(buf, 0, sizeof(buf));
 
     len = snprintf(buf, sizeof(buf), "%s:[.text]", ebpf_obj_filename);
-    if (len < 0 || len >= sizeof(buf)) {
+    if (len < 0 || len >= (int)sizeof(buf)) {
         ebpf_log("netlink_filter_add_end error: filename too long\n");
         rv = -1;
         goto out;
@@ -496,7 +496,8 @@ int netlink_filter_add_end(int fd, struct netlink_ctx *ctx, const char *ebpf_obj
     attr_put_32(nl, MAX_MSG, TCA_BPF_FD, fd);
     attr_put_str(nl, MAX_MSG, TCA_BPF_NAME, buf);
     attr_put_32(nl, MAX_MSG, TCA_BPF_FLAGS, TCA_BPF_FLAG_ACT_DIRECT);
-    ctx->tail->rta_len = (((void *)nl) + nl->nlmsg_len) - (void *)ctx->tail;
+    /* XXX MISSING NLMSG_ALIGN */
+    ctx->tail->rta_len = (((char *)nl) + nl->nlmsg_len) - (char *)ctx->tail;
 
     /* talk to netlink */
     if (rtnetlink_send(&ctx->filter_rth, &ctx->msg.n) < 0) {

diff --git a/non-GPL/HostIsolation/Lib/UpdateMaps.c b/non-GPL/HostIsolation/Lib/UpdateMaps.c
@@ -86,17 +86,17 @@ int ebpf_map_allowed_pids_delete(uint32_t pid)
     return ebpf_map_delete_key(EBPF_ALLOWED_PIDS_MAP_PATH, &key);
 }
 
-int ebpf_map_allowed_IPs_clear()
+int ebpf_map_allowed_IPs_clear(void)
 {
     return ebpf_clear_map(EBPF_ALLOWED_IPS_MAP_PATH, EBPF_MAP_ALLOWED_IPS);
 }
 
-int ebpf_map_allowed_subnets_clear()
+int ebpf_map_allowed_subnets_clear(void)
 {
     return ebpf_clear_map(EBPF_ALLOWED_SUBNETS_MAP_PATH, EBPF_MAP_ALLOWED_SUBNETS);
 }
 
-int ebpf_map_allowed_pids_clear()
+int ebpf_map_allowed_pids_clear(void)
 {
     return ebpf_clear_map(EBPF_ALLOWED_PIDS_MAP_PATH, EBPF_MAP_ALLOWED_PIDS);
 }

diff --git a/non-GPL/HostIsolation/Lib/UpdateMaps.h b/non-GPL/HostIsolation/Lib/UpdateMaps.h
@@ -71,19 +71,19 @@ int ebpf_map_allowed_pids_delete(uint32_t pid);
  *
  * @return Error value (0 for success)
  */
-int ebpf_map_allowed_IPs_clear();
+int ebpf_map_allowed_IPs_clear(void);
 
 /**
  * @brief Clear IP subnet allowlist
  *
  * @return Error value (0 for success)
  */
-int ebpf_map_allowed_subnets_clear();
+int ebpf_map_allowed_subnets_clear(void);
 
 /**
  * @brief Clear pid allowlist
  *
  * @return Error value (0 for success)
  */
-int ebpf_map_allowed_pids_clear();
+int ebpf_map_allowed_pids_clear(void);
 #endif