Handle payload sizes correctly

Change-Id: Ie809ace9b5463025caeefdbbda9d8d237bb318b9
Signed-off-by: Michal Bloch <m.bloch@samsung.com>

diff --git a/include/logcommon.h b/include/logcommon.h
index 524f646..6f3f753 100644 (file)
--- a/include/logcommon.h
+++ b/include/logcommon.h
@@ -19,8 +19,7 @@
 #ifndef _LOGCOMMON_H
 #define _LOGCOMMON_H
 
-#define LOG_MAX_PAYLOAD_SIZE 4076
-#define LOG_MAX_SIZE (LOG_MAX_PAYLOAD_SIZE + sizeof(struct logger_entry))
+#define LOG_MAX_PAYLOAD_SIZE 4076 // has to be at least the value in the kernel: drivers/staging/android/logger.h
 
 #include <assert.h>
 #include <stdio.h>

diff --git a/src/libdlog/log.c b/src/libdlog/log.c
index 65d62e7..89ba2a8 100644 (file)
--- a/src/libdlog/log.c
+++ b/src/libdlog/log.c
@@ -242,7 +242,7 @@ static int dlog_should_log(log_id_t log_id, const char* tag, int prio)
 
 static int __write_to_log(log_id_t log_id, int prio, const char *tag, const char *fmt, va_list ap, bool check_should_log)
 {
-       char buf[LOG_MAX_SIZE];
+       char buf[LOG_MAX_PAYLOAD_SIZE];
 
        __dlog_init();
 
@@ -250,7 +250,7 @@ static int __write_to_log(log_id_t log_id, int prio, const char *tag, const char
        if (ret < 0)
                return ret;
 
-       vsnprintf(buf, LOG_MAX_SIZE, fmt, ap);
+       vsnprintf(buf, sizeof buf, fmt, ap);
 
        return write_to_log(log_id, prio, tag, buf);
 }

diff --git a/src/libdlog/log_pipe.c b/src/libdlog/log_pipe.c
index 86929d9..b43878a 100644 (file)
--- a/src/libdlog/log_pipe.c
+++ b/src/libdlog/log_pipe.c
@@ -107,8 +107,8 @@ static int __reconnect_pipe(log_id_t log_id)
 static int __write_to_log_pipe(log_id_t log_id, log_priority prio, const char *tag, const char *msg)
 {
        ssize_t ret;
-       char buf[LOG_MAX_SIZE];
-       struct pipe_logger_entry* ple = (struct pipe_logger_entry*)buf;
+       char buf[LOG_MAX_PAYLOAD_SIZE + sizeof(struct pipe_logger_entry)];
+       struct pipe_logger_entry* ple = (struct pipe_logger_entry *)buf;
 
        if (!tag)
                tag = "";
@@ -117,7 +117,7 @@ static int __write_to_log_pipe(log_id_t log_id, log_priority prio, const char *t
            prio < DLOG_VERBOSE ||
            prio >= DLOG_PRIO_MAX ||
            !msg ||
-           strlen(tag) + strlen(msg) + sizeof(struct pipe_logger_entry) + 2 >= LOG_MAX_SIZE)
+           1 /* priority */ + strlen(tag) + 1 /* NULL delimiter */ + strlen(msg) + 1 /* NULL terminator */ >= LOG_MAX_PAYLOAD_SIZE)
                return DLOG_ERROR_INVALID_PARAMETER;
 
        create_pipe_message(buf, prio, tag, msg);

diff --git a/src/logger/logger.c b/src/logger/logger.c
index d140e16..ac585c8 100755 (executable)
--- a/src/logger/logger.c
+++ b/src/logger/logger.c
@@ -125,14 +125,16 @@ typedef int (*service_socket_t)(struct logger *server, struct writer *wr, struct
  */
 typedef int (*service_reader_t)(struct logger *server, struct reader *reader);
 
+#define LARGEST_STRUCT(a, b) (sizeof(struct a) > sizeof(struct b) ? sizeof(struct a) : sizeof(struct b))
 struct writer {
        struct fd_entity   fd_entity;
        struct log_buffer* buf_ptr;
        unsigned           readed;
        service_writer_t   service_writer;
        service_socket_t   service_socket;
-       char               buffer[LOG_MAX_SIZE];
+       char               buffer[LOG_MAX_PAYLOAD_SIZE + LARGEST_STRUCT(android_logger_entry, pipe_logger_entry)];
 };
+#undef LARGEST_STRUCT
 
 /**
  * @brief Service reader
@@ -153,7 +155,7 @@ struct reader {
        int                dumpcount;
        struct timespec    last_read_time;
        int                partial_log_size;
-       char               partial_log[LOG_MAX_SIZE];
+       char               partial_log[sizeof(struct logger_entry_with_msg)];
        service_reader_t   service_reader;
 };
 
@@ -868,8 +870,7 @@ static int print_out_logs(struct logger *server, struct reader *reader)
        assert(reader->buf_ptr);
 
        int r, ret = 0;
-       struct logger_entry* ple;
-       char tmp[LOG_MAX_SIZE];
+       struct logger_entry_with_msg lem;
        char * tag;
        unsigned from = reader->current;
        log_entry entry;
@@ -891,35 +892,34 @@ static int print_out_logs(struct logger *server, struct reader *reader)
        }
 
        while (from != reader->buf_ptr->tail && (!reader->dumpcount || (reader->bytes_to_read > 0))) {
-               typeof(ple->len) length;
+               typeof(lem.header.len) length;
                copy_from_buffer(&length, from, sizeof length, reader->buf_ptr);
 
-               copy_from_buffer(tmp, from, length, reader->buf_ptr);
-               ple = (struct logger_entry*)tmp;
+               copy_from_buffer(&lem, from, length, reader->buf_ptr);
 
-               from += ple->len;
+               from += lem.header.len;
                from %= reader->buf_ptr->size;
 
                if (reader->dumpcount) {
-                       if (reader->bytes_to_read > ple->len)
-                               reader->bytes_to_read -= ple->len;
+                       if (reader->bytes_to_read > lem.header.len)
+                               reader->bytes_to_read -= lem.header.len;
                        else
                                reader->bytes_to_read = 0;
                }
 
                if (plaintext) {
-                       logfile_write_with_rotation(ple, &reader->file);
+                       logfile_write_with_rotation(&lem.header, &reader->file);
                        continue;
                }
 
-               tag = ple->msg + 1;
+               tag = lem.msg + 1;
                if (!strlen(tag))
                        continue;
 
-               if (log_process_log_buffer(ple, &entry) || !log_should_print_line(reader->file.format, &entry))
+               if (log_process_log_buffer(&lem.header, &entry) || !log_should_print_line(reader->file.format, &entry))
                        continue;
 
-               r = write(reader->file.path ? reader->file.fd : reader->fd_entity.fd, ple, ple->len);
+               r = write(reader->file.path ? reader->file.fd : reader->fd_entity.fd, &lem, lem.header.len);
                if (r < 0) {
                        if (errno == EPIPE)
                                ret = 1;
@@ -927,9 +927,9 @@ static int print_out_logs(struct logger *server, struct reader *reader)
                }
 
                reader->file.size += r;
-               if (r < ple->len) {
-                       reader->partial_log_size = ple->len - r;
-                       memcpy(reader->partial_log, ple + r, reader->partial_log_size);
+               if (r < lem.header.len) {
+                       reader->partial_log_size = lem.header.len - r;
+                       memcpy(reader->partial_log, ((char *)&lem) + r, reader->partial_log_size);
                        goto cleanup;
                } else if (logfile_rotate_needed(&reader->file) > 0) {
                        logfile_do_rotate(&reader->file);
@@ -989,8 +989,8 @@ static int service_reader_file(struct logger * server, struct reader* reader)
 {
        assert(reader);
 
-       static char buffer[LOG_MAX_SIZE + 1];
        struct logger_entry_with_msg entry;
+       static char buffer[sizeof entry + 1];
        buffer[sizeof buffer - 1] = '\0';
 
        /* The devices for both KMSG and Android Logger only return one log per read().
@@ -1501,7 +1501,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
 
        assert(event);
        if (event->events & EPOLLIN)
-               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, sizeof wr->buffer - wr->readed);
 
        if ((r == 0 || r == -1) && event->events & EPOLLHUP)
                return -EINVAL;
@@ -1521,7 +1521,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
                if (r <= 0)
                        return r;
 
-               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, sizeof wr->buffer - wr->readed);
        } while (r > 0 || ((wr->readed >= sizeof(msg->length) && wr->readed >= msg->length)));
 
        return (r >= 0 || errno == EAGAIN)  ? 0 : r;
@@ -1538,7 +1538,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
 static int service_writer_pipe(struct logger *server, struct writer *wr, struct epoll_event *event)
 {
        if (event->events & EPOLLIN) {
-               int r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               int r = read(wr->fd_entity.fd, wr->buffer + wr->readed, sizeof wr->buffer - wr->readed);
 
                if (r == -1 && errno == EAGAIN)
                        return 0;
@@ -1551,7 +1551,8 @@ static int service_writer_pipe(struct logger *server, struct writer *wr, struct
 
                struct pipe_logger_entry *const ple = (struct pipe_logger_entry *const)wr->buffer;
                while ((wr->readed >= sizeof(ple->len)) && (ple->len <= wr->readed)) {
-                       if (ple->len < sizeof(struct pipe_logger_entry) || ple->len > LOG_MAX_SIZE)
+                       const int payload_size = ple->len - sizeof *ple;
+                       if (payload_size < 0 || payload_size > LOG_MAX_PAYLOAD_SIZE)
                                return -EINVAL;
 
                        struct logger_entry_with_msg lem;
@@ -1559,7 +1560,7 @@ static int service_writer_pipe(struct logger *server, struct writer *wr, struct
                        add_recv_timestamp(&lem.header);
                        buffer_append(&lem.header, wr->buf_ptr);
                        wr->readed -= ple->len;
-                       memmove(wr->buffer, wr->buffer + ple->len, LOG_MAX_SIZE - ple->len);
+                       memmove(wr->buffer, wr->buffer + ple->len, sizeof wr->buffer - ple->len);
                }
        } else if (event->events & EPOLLHUP)
                return -EBADF;
@@ -1582,7 +1583,7 @@ static int service_writer_kmsg(struct logger* server, struct writer* wr, struct
        if (!(event->events & EPOLLIN))
                return 0;
 
-       int r = read(wr->fd_entity.fd, wr->buffer, LOG_MAX_SIZE - 1);
+       int r = read(wr->fd_entity.fd, wr->buffer, sizeof wr->buffer - 1);
 
        if (r == -1 && (errno == EAGAIN || errno == EPIPE))
                return 0;
@@ -1673,7 +1674,7 @@ static int service_writer_syslog(struct logger* server, struct writer* wr, struc
        if (!(event->events & EPOLLIN))
                return 0;
 
-       int r = read(wr->fd_entity.fd, wr->buffer, LOG_MAX_SIZE - 1);
+       int r = read(wr->fd_entity.fd, wr->buffer, sizeof wr->buffer - 1);
 
        if (r == -1 && (errno == EAGAIN || errno == EPIPE))
                return 0;
@@ -1686,15 +1687,14 @@ static int service_writer_syslog(struct logger* server, struct writer* wr, struc
 
        wr->buffer[r] = '\0';
 
-       char buffer[LOG_MAX_SIZE];
-       if (parse_syslog_datagram(wr->buffer, r + 1, (struct logger_entry *) buffer)) {
+       struct logger_entry_with_msg lem;
+       if (parse_syslog_datagram(wr->buffer, r + 1, &lem.header)) {
                /* don't return parse error as writers are removed then */
                return 0;
        }
 
-       struct logger_entry * entry = (struct logger_entry*) buffer;
-       add_recv_timestamp(entry);
-       buffer_append(entry, wr->buf_ptr);
+       add_recv_timestamp(&lem.header);
+       buffer_append(&lem.header, wr->buf_ptr);
        return 0;
 }
 

diff --git a/src/logutil/fdi_logger.c b/src/logutil/fdi_logger.c
index 43648f9..03dc1f1 100644 (file)
--- a/src/logutil/fdi_logger.c
+++ b/src/logutil/fdi_logger.c
@@ -151,7 +151,7 @@ static int logger_read(struct fd_info *fdi)
        assert(lpd);
        assert(!lpd->entry);
 
-       char buff[LOG_MAX_SIZE];
+       char buff[LOG_MAX_PAYLOAD_SIZE + sizeof(struct android_logger_entry)];
        int r = read(fdi->fd, buff, sizeof buff);
        if (r < 0)
                return -errno;

diff --git a/src/shared/queued_entry.c b/src/shared/queued_entry.c
index e328b52..4c3e0e5 100644 (file)
--- a/src/shared/queued_entry.c
+++ b/src/shared/queued_entry.c
@@ -47,7 +47,7 @@ void create_pipe_message(void *buf, int prio, char const *tag, char const *msg)
        assert(tag);
        assert(prio >= DLOG_VERBOSE);
        assert(prio < DLOG_PRIO_MAX);
-       assert(strlen(tag) + strlen(msg) + sizeof(struct pipe_logger_entry) + 2 < LOG_MAX_SIZE);
+       assert(1 /* priority */ + strlen(tag) + 1 /* NULL delimiter */ + strlen(msg) + 1 /* NULL terminator */ <= LOG_MAX_PAYLOAD_SIZE);
 
        struct pipe_logger_entry *ple = (struct pipe_logger_entry *) buf;
        struct timespec ts;
@@ -79,7 +79,7 @@ void create_pipe_message(void *buf, int prio, char const *tag, char const *msg)
  */
 int parse_kmsg_message(char * buffer, int buffer_size)
 {
-       static char buffer_temp[LOG_MAX_SIZE];
+       static char buffer_temp[LOG_MAX_PAYLOAD_SIZE];
        struct logger_entry le;
        unsigned long long timestamp;
        char * tag_begin, * pri_begin, * msg_begin;
@@ -143,8 +143,8 @@ int parse_kmsg_message(char * buffer, int buffer_size)
        prio = strtol(pri_begin, NULL, 10);
 
        struct logger_entry * buf_le = (struct logger_entry *) buffer;
-       buf_le->len = 1 + snprintf(buffer_temp, LOG_MAX_SIZE - 1, "%c%s", prio, tag_begin);
-       buf_le->len += 1 + snprintf(buffer_temp + buf_le->len, LOG_MAX_SIZE - 1 - buf_le->len, "%s", msg_begin);
+       buf_le->len = 1 + snprintf(buffer_temp, sizeof buffer_temp - 1, "%c%s", prio, tag_begin);
+       buf_le->len += 1 + snprintf(buffer_temp + buf_le->len, sizeof buffer_temp - 1 - buf_le->len, "%s", msg_begin);
 
        memcpy(buf_le->msg, buffer_temp, buf_le->len);
        buf_le->len += sizeof(struct logger_entry);
@@ -166,6 +166,7 @@ int parse_kmsg_message(char * buffer, int buffer_size)
 void parse_androidlogger_message(struct android_logger_entry *ale, struct logger_entry *le, size_t dgram_size)
 {
        const size_t payload_size = dgram_size - sizeof *ale;
+       assert(payload_size <= LOG_MAX_PAYLOAD_SIZE);
 
        le->len = sizeof *le + payload_size;
        le->pid = ale->pid;

diff --git a/src/tests/pipe_message.c b/src/tests/pipe_message.c
index 4d9aeb4..515b1b6 100644 (file)
--- a/src/tests/pipe_message.c
+++ b/src/tests/pipe_message.c
@@ -21,8 +21,8 @@ int main()
        int prio = 5;
        char tag[] = "TEST_TAG";
        char msg[] = "Test message with a newline \n and some special characters \e[31m in colour";
-       struct pipe_logger_entry *ple = alloca(LOG_MAX_SIZE);
-       struct logger_entry *le = alloca(LOG_MAX_SIZE);
+       struct pipe_logger_entry *ple = alloca(LOG_MAX_PAYLOAD_SIZE + sizeof *ple);
+       struct logger_entry *le = alloca(LOG_MAX_PAYLOAD_SIZE + sizeof *le);
        struct timespec ts_pre, ts_post;
 
        clock_gettime(CLOCK_MONOTONIC, &ts_pre);
@@ -38,7 +38,7 @@ int main()
        assert(time_cmp(ts_pre,  le->sec, le->nsec) <= 0);
        assert(time_cmp(ts_post, le->sec, le->nsec) >= 0);
 
-       static const int SAFE_SIZE = (LOG_MAX_SIZE - sizeof(struct logger_entry) - 2) / 2;
+       static const int SAFE_SIZE = ((LOG_MAX_PAYLOAD_SIZE - 1 /* prio */) / 2 /* split over tag and msg */) - 1 /* null delimiter */;
        char random_tag[SAFE_SIZE];
        char random_msg[SAFE_SIZE];
        random_tag[SAFE_SIZE - 1] = random_msg[SAFE_SIZE - 1] = '\0';

diff --git a/tests/test_libdlog.c b/tests/test_libdlog.c
index 7e25fcc..3c6da55 100644 (file)
--- a/tests/test_libdlog.c
+++ b/tests/test_libdlog.c
@@ -21,21 +21,20 @@ void *func(void *data)
 int main(int argc, char **argv)
 {
        if (argc == 1) {
-               char huge_buffer[LOG_MAX_SIZE * 3];
-               char buffer[LOG_MAX_SIZE];
+               char huge_buffer[LOG_MAX_PAYLOAD_SIZE * 3];
+               char buffer[LOG_MAX_PAYLOAD_SIZE - 20 /* leeway for tag, prio etc */];
                int i;
                pthread_t threads[50];
 
                // check buffer overflow
-               memset(huge_buffer, 'a', LOG_MAX_SIZE);
-               memset(huge_buffer + LOG_MAX_SIZE, 'b', LOG_MAX_SIZE);
-               memset(huge_buffer + LOG_MAX_SIZE * 2, 'c', LOG_MAX_SIZE);
-               huge_buffer[LOG_MAX_SIZE * 3 - 1] = '\0';
+               for (int i = 0; i < 3; ++i)
+                       memset(huge_buffer + i * LOG_MAX_PAYLOAD_SIZE, 'a' + i, LOG_MAX_PAYLOAD_SIZE);
+               huge_buffer[sizeof huge_buffer - 1] = '\0';
 
                LOGE("%s", huge_buffer);
 
                // check garbage data
-               for (i = 0; i < LOG_MAX_SIZE; ++i)
+               for (i = 0; i < sizeof buffer; ++i)
                        buffer[i] = rand() & 255;
 
                LOGE("%s", buffer);