From 58e399400c4ef4b71472ab7c2c71c54a677a05a8 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Wed, 28 Oct 2026 14:18:18 +0200 Subject: log: bound log lines and share target unpacking A log line is flushed only when its text ends in a newline, so it can be assembled from any number of log calls. Packet printers make one call per attribute, so a single line could grow far beyond LOG_MAX_SIZE, while the tcp, syslog and pgsql targets each copied the whole chunk list into a fixed size buffer without checking its length. Cap the accumulated line at LOG_MAX_SIZE in do_log(), keeping the trailing newline so the line is still flushed, and replace the three duplicated unpack loops with log_unpack_msg(), which never writes past the size it is given. The syslog bound is needed on its own: a session header plus a full length line does not fit its buffer. Also skip empty formatted strings, which made do_log() read the byte before its buffer. When a single call is cut short by vsnprintf its newline is lost; restore it when the format ends in one, so the line is closed instead of swallowing the next one. The syslog and pgsql targets now strip the trailing newline only when it is there, instead of dropping the last character of a line cut to fit. Signed-off-by: Denys Fedoryshchenko --- tests/unit/log_line_safeguards_test.c | 164 ++++++++++++++++++++++++++++++++++ tests/unit/run_safeguards.sh | 6 +- 2 files changed, 168 insertions(+), 2 deletions(-) create mode 100644 tests/unit/log_line_safeguards_test.c (limited to 'tests/unit') diff --git a/tests/unit/log_line_safeguards_test.c b/tests/unit/log_line_safeguards_test.c new file mode 100644 index 00000000..487e763b --- /dev/null +++ b/tests/unit/log_line_safeguards_test.c @@ -0,0 +1,164 @@ +/* A log line assembled from many calls must not outgrow LOG_MAX_SIZE, + * nor overflow a target buffer once a session header is prepended. */ +#include +#include +#include "triton.h" +#undef DEFINE_INIT +#define DEFINE_INIT(o, f) +#define load_config core_load_config +#include "../../accel-pppd/log.c" +#undef load_config +#include "../../accel-pppd/logs/log_syslog.c" + +static char *line; +static int line_len, lines; + +mempool_t *mempool_create(int size) +{ + int *p = malloc(sizeof(*p)); + *p = size; + return (mempool_t *)p; +} +void *mempool_alloc(mempool_t *pool) { return calloc(1, *(int *)pool); } +void mempool_free(void *ptr) { free(ptr); } +char *conf_get_opt(const char *sect, const char *name) +{ + return strcmp(name, "level") ? NULL : "5"; +} +int triton_event_register_handler(int ev_id, triton_event_func func) { return 0; } + +/* Unpack like the syslog/pgsql targets do, into a buffer of the same size, + * without a bound of its own so ASan reports an oversized line. */ +static void test_log(struct log_target_t *t, struct log_msg_t *msg, struct ap_session *ses) +{ + struct log_chunk_t *chunk; + + line_len = 0; + list_for_each_entry(chunk, msg->chunks, entry) { + memcpy(line + line_len, chunk->msg, chunk->len); + line_len += chunk->len; + } + line[line_len] = 0; + lines++; + log_free_msg(msg); +} + +static struct log_target_t test_target = { + .log = test_log, +}; + +/* the longest line the core passes on, behind the longest session header */ +static void test_syslog(void) +{ + struct ap_ctrl ctrl = {}; + struct ap_session ses = {.ctrl = &ctrl}; + struct log_msg_t msg = {}; + struct log_chunk_t *chunk; + LIST_HEAD(chunks); + char username[256]; + int i, n = LOG_MAX_SIZE / LOG_CHUNK_SIZE; + + log_buf = malloc(LOG_MAX_SIZE + 1); + msg.hdr = malloc(sizeof(struct log_chunk_t) + LOG_CHUNK_SIZE + 1); + assert(log_buf && msg.hdr); + strcpy(ses.ifname, "ppp0"); + memset(username, 'u', sizeof(username) - 1); + username[sizeof(username) - 1] = 0; + ses.username = username; + + for (i = 0; i < n; i++) { + chunk = malloc(sizeof(*chunk) + LOG_CHUNK_SIZE + 1); + assert(chunk); + chunk->len = LOG_CHUNK_SIZE; + memset(chunk->msg, 'A', LOG_CHUNK_SIZE); + list_add_tail(&chunk->entry, &chunks); + } + chunk->msg[LOG_CHUNK_SIZE - 1] = '\n'; + msg.chunks = &chunks; + + set_hdr(&msg, &ses); + assert(strlen(msg.hdr->msg) == LOG_CHUNK_SIZE - 1); + unpack_msg(&msg); + assert(strlen(log_buf) == LOG_MAX_SIZE); + assert(!memcmp(log_buf, "ppp0:uuu", 8)); + assert(log_buf[LOG_MAX_SIZE - 1] == 'A'); + + /* nothing is written to a buffer that is already full */ + assert(log_unpack_msg(&msg, NULL, 0) == 0); + assert(log_unpack_msg(&msg, NULL, -LOG_CHUNK_SIZE) == 0); + + /* a short line is unaffected */ + chunk = list_first_entry(&chunks, typeof(*chunk), entry); + strcpy(chunk->msg, "hello\n"); + chunk->len = 6; + while (chunks.prev != &chunk->entry) { + struct log_chunk_t *c = list_entry(chunks.prev, typeof(*c), entry); + list_del(&c->entry); + free(c); + } + set_hdr(&msg, NULL); + unpack_msg(&msg); + assert(!strcmp(log_buf, "hello")); + + list_del(&chunk->entry); + free(chunk); + free(msg.hdr); + free(log_buf); +} + +int main(void) +{ + char big[1024], *huge; + int i; + + line = malloc(LOG_MAX_SIZE + 1); + assert(line); + log_init(); + log_register_target(&test_target); + memset(big, 'A', sizeof(big) - 1); + big[sizeof(big) - 1] = 0; + + /* the L2TP packet printer: one call per AVP, newline at the end */ + log_info2("recv ["); + for (i = 0; i < 64; i++) + log_info2(" <%s>", big); + log_info2("]\n"); + assert(lines == 1); + assert(line_len == LOG_MAX_SIZE); + assert(!memcmp(line, "recv [