From 8a218a7ac2c7580e201857e9097c19104292b124 Mon Sep 17 00:00:00 2001
From: hippoz <10706925-hippoz@users.noreply.gitlab.com>
Date: Mon, 26 Dec 2022 20:28:56 +0200
Subject: [PATCH] improve error handling

---
 main.c   |  4 +++-
 server.c | 37 +++++++++++++++++++++++++++++--------
 wire.c   | 17 ++++++++++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/main.c b/main.c
index 5258b0f..ad9263f 100644
--- a/main.c
+++ b/main.c
@@ -1,6 +1,8 @@
 #include <stdint.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <string.h>
+#include <errno.h>
 #include "server.h"
 
 const char *arg_shift(int *argc, char ***argv) {
@@ -30,7 +32,7 @@ int main(int argc, char *argv[])
 
     while (1) {
         if (jb_server_turn(srv) < 0) {
-            fprintf(stderr, "server_turn failed\n");
+            fprintf(stderr, "server_turn failed (errno=%d, strerror=%s)\n", errno, strerror(errno));
             jb_server_free(srv);
             return EXIT_FAILURE;
         }
diff --git a/server.c b/server.c
index e6c29c0..d5eb01b 100644
--- a/server.c
+++ b/server.c
@@ -403,6 +403,12 @@ void jb_server_free(struct jb_server *s)
 {
     if (!s) return;
     if (s->sock_fd) close(s->sock_fd);
+    // names are allocated on the heap
+    for (int i = 0; i < JB_MAX_NAMES; i++) {
+        if (s->names[i].name) {
+            free(s->names[i].name);
+        }
+    }
     free(s);
 }
 
@@ -472,6 +478,27 @@ int jb_server_turn(struct jb_server *s)
 
     for (int i = 0; i < s->fd_num; i++) {
         int fd = s->fds[i].fd;
+        if (s->fds[i].revents & POLLNVAL) {
+            fprintf(stderr, "jb_server_turn: error: got POLLNVAL for fds[%d]. This is considered a bug in the server implementation.\n", i);
+            return -1;
+        }
+        if (s->fds[i].revents & POLLERR) {
+            if (fd == s->sock_fd) {
+                fprintf(stderr, "jb_server_turn: error: got POLLERR for sock_fd\n");
+                return -1;
+            }
+            fprintf(stderr, "jb_server_turn: warn: got POLLERR for fds[%d], removing...\n", i);
+            jb_server_client_remove(s, i);
+            continue;
+        }
+        if (s->fds[i].revents & POLLHUP) {
+            if (fd == s->sock_fd) {
+                fprintf(stderr, "jb_server_turn: error: got POLLHUP for sock_fd\n");
+                return -1;
+            }
+            jb_server_client_remove(s, i);
+            continue;
+        }
         if (s->fds[i].revents & POLLIN) {
             // file descriptor ready for reading
 
@@ -488,15 +515,9 @@ int jb_server_turn(struct jb_server *s)
             char data[data_buffer_len];
             memset(data, 0, sizeof(data));
             ssize_t bytes = recv(fd, data, sizeof(data) - 1, 0);
-            if (bytes < 0) {
-                // error during recv(), disconnect the client
+            if (bytes <= 0) {
+                // error during recv() OR client disconnected, disconnect the client
                 // TODO: should we actually do this?
-                close(fd);
-                jb_server_client_remove(s, i);
-                return -1;
-            }
-            if (bytes == 0) {
-                // client disconnected
                 jb_server_client_remove(s, i);
                 continue;
             }
diff --git a/wire.c b/wire.c
index 7669e40..f8ac6c4 100644
--- a/wire.c
+++ b/wire.c
@@ -3,6 +3,13 @@
 #include "try.h"
 #include "wire.h"
 
+#define WIRE_ENABLE_VERBOSE 0
+#if WIRE_ENABLE_VERBOSE
+#define WIRE_VERBOSE printf
+#else
+#define WIRE_VERBOSE(...)
+#endif
+
 bool wire_align(wire_context_t *c, uint32_t alignment)
 {
     if ((c->byte_cursor % alignment) == 0) {
@@ -130,7 +137,7 @@ int wire_parse_message(wire_context_t *c, wire_message_t *msg)
 
     size_t header_fields_end = c->byte_cursor + msg->header_fields_length;
 
-    printf("start parse message: type=%d\n", msg->type);
+    WIRE_VERBOSE("start parse message: type=%d\n", msg->type);
 
     while (c->byte_cursor < header_fields_end) {
         uint8_t field_code = *(uint8_t*)TRYPTR(wire_get_u8(c));
@@ -142,17 +149,17 @@ int wire_parse_message(wire_context_t *c, wire_message_t *msg)
             case DBUS_HEADER_FIELD_MEMBER: /* through */
             case DBUS_HEADER_FIELD_DESTINATION: {
                 char *str = TRYPTR(wire_get_string(c, NULL));
-                printf("field: %s\n", str);
+                WIRE_VERBOSE("field: %s\n", str);
                 msg->fields[field_code].t.str = str;
             } break;
             case DBUS_HEADER_FIELD_SIGNATURE: {
                 char *str = TRYPTR(wire_get_signature(c, NULL));
-                printf("field: %s\n", str);
+                WIRE_VERBOSE("field: %s\n", str);
                 msg->fields[field_code].t.str = str;
             } break;
             case DBUS_HEADER_FIELD_REPLY_SERIAL: {
                 uint32_t u = *(uint32_t*)TRYPTR(wire_get_u32(c));
-                printf("field: %d\n", u);
+                WIRE_VERBOSE("field: %d\n", u);
                 msg->fields[field_code].t.u32 = u;
             } break;
             default: {
@@ -171,7 +178,7 @@ int wire_parse_message(wire_context_t *c, wire_message_t *msg)
         }
     }
 
-    printf("end parse message\n");
+    WIRE_VERBOSE("end parse message\n");
 
     /* header ends at 8 byte boundry */
     TRYPTR(wire_align(c, 8));