Invalid error check due to signed/unsigned confusion

[hannob]

In the function _read_text_file_content_without_trailing_newline in the file
modules/afsocket/transport-unix-socket.c
there is a bug in the error check.

The code sets content_len to the return value of _read_text_file_content. That function returns -1 in case of an error. Then there is a check for (content_len <= 0).

The problem: cotent_len is of type gsize (unsigned), while _read_text_file_content returns gssize (signed). If it returns -1 this can't be stored in conten_len and will underflow, thus the error check will be skipped.

This causes some invalid memory reads which I detected with Address Sanitizer.

To fix this content_len must also be gssize (signed), so it can be -1. This patch (also attached) will fix it:

a/modules/afsocket/transport-unix-socket.c  2015-10-27 09:08:53.000000000 +0100
+++ b/modules/afsocket/transport-unix-socket.c  2015-11-11 17:30:21.019247981 +0100
@@ -86,7 +86,7 @@
 static gssize
 _read_text_file_content_without_trailing_newline(const gchar *filename, gchar *buf, gsize buflen)
 {
-  gsize content_len;
+  gssize content_len;

   content_len = _read_text_file_content(filename, buf, buflen);
   if (content_len <= 0)

syslog-ng-signed-underflow.txt

[hannob]

Some further Info: This affects both syslog-ng 3.7.2 and the current git code.

The Address Sanitizer stack trace looks like this:

==10668==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7f013e792f3e at pc 0x7f013fe1b643 bp 0x7f013e792e90 sp 0x7f013e792e80
READ of size 1 at 0x7f013e792f3e thread T2
    #0 0x7f013fe1b642 in _read_text_file_content_without_trailing_newline modules/afsocket/transport-unix-socket.c:94
    #1 0x7f013fe1b642 in _add_nv_pair_proc_read_unless_unset modules/afsocket/transport-unix-socket.c:109
    #2 0x7f013fe1bc15 in _feed_aux_from_procfs modules/afsocket/transport-unix-socket.c:170
    #3 0x7f013fe1bc15 in _feed_credentials_from_cmsg modules/afsocket/transport-unix-socket.c:195
    #4 0x7f013fe1bc15 in _feed_aux_from_cmsg modules/afsocket/transport-unix-socket.c:206
    #5 0x7f013fe1bc15 in _unix_socket_read modules/afsocket/transport-unix-socket.c:241
    #6 0x7f013fe1bdfc in log_transport_unix_dgram_socket_read_method modules/afsocket/transport-unix-socket.c:252
    #7 0x7f014511a537 in log_transport_read lib/transport/logtransport.h:51
    #8 0x7f014511a537 in log_proto_buffered_server_read_data_method lib/logproto/logproto-buffered-server.c:563
    #9 0x7f014511d4ec in log_proto_buffered_server_read_data lib/logproto/logproto-buffered-server.c:617
    #10 0x7f014511d4ec in log_proto_buffered_server_fetch_into_buffer lib/logproto/logproto-buffered-server.c:652
    #11 0x7f014511da80 in log_proto_buffered_server_fetch lib/logproto/logproto-buffered-server.c:803
    #12 0x7f01450bf183 in log_proto_server_fetch lib/logproto/logproto-server.h:118
    #13 0x7f01450bf183 in log_reader_fetch_log lib/logreader.c:357
    #14 0x7f01450bf778 in log_reader_work_perform lib/logreader.c:86
    #15 0x7f01450d55e0 in _work lib/mainloop-io-worker.c:52
    #16 0x7f0145149600 in iv_work_thread_do_work /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:118
    #17 0x7f0145146ca9 in iv_run_tasks /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_task.c:48
    #18 0x7f014514d16a in iv_main /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_main_posix.c:106
    #19 0x7f014514908a in iv_work_thread /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_work.c:200
    #20 0x7f014514e5e8 in iv_thread_handler /var/tmp/portage/app-admin/syslog-ng-3.7.2/work/syslog-ng-3.7.2/lib/ivykis/src/iv_thread_posix.c:142
    #21 0x7f0144ad9433 (/lib64/libpthread.so.0+0x7433)
    #22 0x7f014481e6dc in __clone (/lib64/libc.so.6+0xe96dc)

0x7f013e792f3e is located 132951710424894 bytes inside==10668==AddressSanitizer: while reporting a bug found another one.Ignoring.

Nice catch, could you open a pull request which contains this patch?

[hannob]

pull request sent:
#783

[bkil-syslogng]

-Werror with either -Wconversion or -Wsign-conversion would have caught this