fix(cpworker): write log lines atomically; take config errors from the caller's parse end

cpworker/src/cjson_utils.c

@@ -7,6 +7,43 @@
 
 #include "cjson_utils.h"
 
+#define CJSON_ERROR_SNIPPET_LEN 32
+
+cJSON *cjson_parse(const char *json, cJSONParseError *err)
+{
+    const char *end = NULL;
+    cJSON *item = cJSON_ParseWithOpts(json, &end, false);
+    if (item)
+        return item;
+
+    if (!json || !end)
+    {
+        cjson_set_parse_error(err, "JSON parse error");
+        return NULL;
+    }
+
+    int line = 1, column = 1;
+    for (const char *p = json; p < end; p++)
+    {
+        if (*p == '\n')
+        {
+            line++;
+            column = 1;
+        }
+        else
+            column++;
+    }
+
+    // The snippet stays on the error's line, so the message remains a single log line.
+    int snippet_len = 0;
+    while (snippet_len < CJSON_ERROR_SNIPPET_LEN && end[snippet_len] != '\0' && end[snippet_len] != '\n' &&
+           end[snippet_len] != '\r')
+        snippet_len++;
+
+    cjson_set_parse_error(err, "JSON parse error at line %d, column %d near '%.*s'", line, column, snippet_len, end);
+    return NULL;
+}
+
 void cjson_set_parse_error(cJSONParseError *err, const char *format, ...)
 {
     if (!err)

cpworker/src/cjson_utils.h

@@ -22,6 +22,11 @@ typedef struct
     char message[CJSON_ERRBUF_SIZE];
 } cJSONParseError;
 
+// Parses the NUL-terminated `json`. On a syntax error, returns NULL and sets err to
+// "JSON parse error at line L, column C near '<snippet>'". The position comes from this call's own parse end, not from
+// cJSON_GetErrorPtr(), whose process-wide state another thread's parse can overwrite.
+cJSON *cjson_parse(const char *json, cJSONParseError *err);
+
 void cjson_set_parse_error(cJSONParseError *err, const char *format, ...);
 void cjson_wrap_parse_error(cJSONParseError *err, const char *format, ...);
 

cpworker/src/config.c

@@ -1,8 +1,10 @@
+#include <errno.h>
 #include <limits.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/stat.h>
 
 #include "cJSON/cJSON.h"
 
@@ -20,6 +22,9 @@
 // pcap_set_buffer_size() takes an int byte count; this is the largest whole-MB value that fits.
 #define MAX_LIBPCAP_BUFFER_SIZE_MB (INT_MAX / 1024 / 1024)
 
+// A config file is a few KiB; the cap only keeps a wrong path from being read into memory whole.
+#define CONFIG_FILE_MAX_SIZE (64 * 1024 * 1024)
+
 // Parses a capturer's snaplen. Values above MAX_SNAPLEN become MAX_SNAPLEN. Values <= 0 also become MAX_SNAPLEN
 // ("no truncation") when `non_positive_means_max`, and are rejected otherwise.
 static int parse_snaplen(cJSON *obj, const char *name, bool non_positive_means_max, int *out, cJSONParseError *err)
@@ -1218,16 +1223,35 @@ static char *read_file_contents(const char *filename, cJSONParseError *error)
         return NULL;
     }
 
-    fseek(fp, 0, SEEK_END);
-    long length = ftell(fp);
-    fseek(fp, 0, SEEK_SET);
-
-    if (length <= 0)
+    // fopen succeeds on a directory, and seeking one does not yield a usable size, so the type and size come from
+    // fstat instead.
+    struct stat st;
+    if (fstat(fileno(fp), &st) != 0)
+    {
+        cjson_set_parse_error(error, "failed to stat file: %s: %s", filename, strerror(errno));
+        fclose(fp);
+        return NULL;
+    }
+    if (!S_ISREG(st.st_mode))
+    {
+        cjson_set_parse_error(error, "not a regular file: %s", filename);
+        fclose(fp);
+        return NULL;
+    }
+    if (st.st_size == 0)
     {
         cjson_set_parse_error(error, "empty file: %s", filename);
         fclose(fp);
         return NULL;
     }
+    if (st.st_size > CONFIG_FILE_MAX_SIZE)
+    {
+        cjson_set_parse_error(error, "file too large: %s (%lld bytes, limit %d)", filename, (long long)st.st_size,
+                              CONFIG_FILE_MAX_SIZE);
+        fclose(fp);
+        return NULL;
+    }
+    size_t length = (size_t)st.st_size;
 
     char *buffer = (char *)malloc(length + 1);
     if (!buffer)
@@ -1237,7 +1261,7 @@ static char *read_file_contents(const char *filename, cJSONParseError *error)
         return NULL;
     }
 
-    if (fread(buffer, 1, length, fp) != (size_t)length)
+    if (fread(buffer, 1, length, fp) != length)
     {
         cjson_set_parse_error(error, "partial read of file: %s", filename);
         free(buffer);
@@ -1256,10 +1280,9 @@ TasksAllConfig *parse_tasks_file(const char *filename, cJSONParseError *err)
     if (!json_str)
         return NULL;
 
-    cJSON *json = cJSON_Parse(json_str);
+    cJSON *json = cjson_parse(json_str, err);
     if (!json)
     {
-        cjson_set_parse_error(err, "JSON parse error before: %s", cJSON_GetErrorPtr());
         free(json_str);
         return NULL;
     }
@@ -1473,10 +1496,9 @@ static Config *parse_config_json(cJSON *json, cJSONParseError *err)
 
 Config *parse_config_data(const char *json_str, cJSONParseError *err)
 {
-    cJSON *json = cJSON_Parse(json_str);
+    cJSON *json = cjson_parse(json_str, err);
     if (!json)
     {
-        cjson_set_parse_error(err, "JSON parse error before: %s", cJSON_GetErrorPtr());
         return NULL;
     }
 
@@ -1491,10 +1513,9 @@ Config *parse_config_file(const char *filename, cJSONParseError *err)
     if (!json_str)
         return NULL;
 
-    cJSON *json = cJSON_Parse(json_str);
+    cJSON *json = cjson_parse(json_str, err);
     if (!json)
     {
-        cjson_set_parse_error(err, "JSON parse error before: %s", cJSON_GetErrorPtr());
         free(json_str);
         return NULL;
     }

cpworker/src/log.c

@@ -1,9 +1,13 @@
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <time.h>
 
 #include "log.h"
 
+// Lines that fit are formatted on the stack; longer ones go to the heap, or are truncated if that fails.
+#define LOG_LINE_STACK_SIZE 1024
+
 static struct
 {
     int level;
@@ -15,17 +19,52 @@ const char *log_level_string(int level) { return level_strings[level]; }
 
 void log_set_level(int level) { L.level = level; }
 
+// Several threads log at once, and stdio locks each call rather than each line. The whole line is therefore
+// formatted first and written with a single fwrite, so lines from different threads never interleave.
 static void stderr_callback(log_event_t *ev)
 {
-    char buf[64];
-    if (ev->time)
-        buf[strftime(buf, sizeof(buf), "%Y-%m-%dT%H:%M:%S", ev->time)] = '\0';
-    else
-        snprintf(buf, sizeof(buf), "<time-unavailable>");
-    fprintf(stderr, "%s %-5s %s:%d: ", buf, log_level_string(ev->level), ev->file, ev->line);
-    vfprintf(stderr, ev->fmt, ev->ap);
-    fprintf(stderr, "\n");
+    char stack_line[LOG_LINE_STACK_SIZE];
+    char *line = stack_line;
+    size_t size = sizeof(stack_line);
+
+    char ts[32];
+    if (!ev->time || strftime(ts, sizeof(ts), "%Y-%m-%dT%H:%M:%S", ev->time) == 0)
+        snprintf(ts, sizeof(ts), "<time-unavailable>");
+    int prefix_len = snprintf(line, size, "%s %-5s %s:%d: ", ts, log_level_string(ev->level), ev->file, ev->line);
+    if (prefix_len < 0)
+        return;
+    if ((size_t)prefix_len > size - 2)
+        prefix_len = (int)size - 2;
+
+    va_list ap;
+    va_copy(ap, ev->ap);
+    int msg_len = vsnprintf(line + prefix_len, size - prefix_len, ev->fmt, ap);
+    va_end(ap);
+    if (msg_len < 0)
+        msg_len = 0;
+
+    // Room for the message, '\n' and the terminating '\0'.
+    size_t need = (size_t)prefix_len + (size_t)msg_len + 2;
+    if (need > size)
+    {
+        char *heap_line = malloc(need);
+        if (heap_line)
+        {
+            memcpy(heap_line, line, prefix_len);
+            vsnprintf(heap_line + prefix_len, need - prefix_len, ev->fmt, ev->ap);
+            line = heap_line;
+        }
+        else
+            msg_len = (int)(size - prefix_len - 2);
+    }
+
+    size_t len = (size_t)prefix_len + (size_t)msg_len;
+    line[len++] = '\n';
+    fwrite(line, 1, len, stderr);
     fflush(stderr);
+
+    if (line != stack_line)
+        free(line);
 }
 
 void log_log(int level, const char *file, int line, const char *fmt, ...)
@@ -40,9 +79,10 @@ void log_log(int level, const char *file, int line, const char *fmt, ...)
         .level = level,
     };
     time_t t = time(NULL);
-    ev.time = localtime(&t);
+    struct tm tm_buf;
+    ev.time = localtime_r(&t, &tm_buf);
 
     va_start(ev.ap, fmt);
     stderr_callback(&ev);
     va_end(ev.ap);
-}
+}

cpworker/tests/unit/CMakeLists.txt

@@ -25,7 +25,9 @@ endif()
 set(MAIN_CONFIG_DIR "${CMAKE_BINARY_DIR}")
 
 set(unity_tests
+    config_file
     config_validation
+    logging
     misc
     output_file
     output_rotating_file