Weird bug in syslog

Weird bug in syslog

[William Haddon]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]


Hi all.

I noticed seg-faults and other weird behavior when using the syslog() 
function with large messages. I've attached the simplest test program 
that reproduces the problem. I've observed it to break on 0.9.9 on i386 
and current git on x86_64. The problem seems to be that although the 
syslog function successfully truncates its input to 256 bytes, it 
passes the size of the un-truncated form to the sendto() call because 
snprintf returns the number of bytes that would be written if 
truncation did not occur. Fixing syslog to check if truncation occurred 
seems to fix the problem. I've attached the patch that does this.

William Haddon

[-- Attachment #2: test3.c --]
[-- Type: text/x-csrc, Size: 362 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <syslog.h>

#define A 8
#define B 8

int main(int argc, char **argv)
{
	char *temp;
	char v;
	size_t i, j;

	for (j = A; j < 10000; j += B) {
		temp = malloc(j+1);
		for (i = 0; i < j; i++)
			temp[i] = 'x';
		temp[j] = 0;
		syslog(LOG_ERR, temp);
		free(temp);
	}
	printf("Success\n");
}


[-- Attachment #3: musl-syslog.patch --]
[-- Type: text/x-patch, Size: 549 bytes --]

Report the correct length of the datagram to the kernel to fix strange behavior
in the syslog function.
--- musl-0.9.9/src/misc/syslog.c
+++ src/src/misc/syslog.c
@@ -90,9 +90,11 @@
 		priority, timebuf,
 		log_ident ? log_ident : "",
 		"["+!pid, pid, "]"+!pid);
+	if (l > sizeof buf) l = sizeof buf - 1;
 	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
 	if (l2 >= 0) {
 		l += l2;
+		if (l > sizeof buf) l = sizeof buf - 1;
 		if (buf[l-1] != '\n') buf[l++] = '\n';
 		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
 	}

Re: Weird bug in syslog

[Szabolcs Nagy]

* William Haddon <william@haddonthethird.net> [2013-03-19 15:32:35 -0400]:
> I noticed seg-faults and other weird behavior when using the syslog() 
> function with large messages. I've attached the simplest test program 
> that reproduces the problem. I've observed it to break on 0.9.9 on i386 
> and current git on x86_64. The problem seems to be that although the 
> syslog function successfully truncates its input to 256 bytes, it 
> passes the size of the un-truncated form to the sendto() call because 
> snprintf returns the number of bytes that would be written if 
> truncation did not occur. Fixing syslog to check if truncation occurred 
> seems to fix the problem. I've attached the patch that does this.

i can confirm this


> Report the correct length of the datagram to the kernel to fix strange behavior
> in the syslog function.
> --- musl-0.9.9/src/misc/syslog.c
> +++ src/src/misc/syslog.c
> @@ -90,9 +90,11 @@
>  		priority, timebuf,
>  		log_ident ? log_ident : "",
>  		"["+!pid, pid, "]"+!pid);
> +	if (l > sizeof buf) l = sizeof buf - 1;

l >= sizeof buf

(it is not correct when l<0 but that snprintf cannot fail)

>  	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
>  	if (l2 >= 0) {
>  		l += l2;

these are int values
maybe we should care about overflow
(eg making l size_t works)

> +		if (l > sizeof buf) l = sizeof buf - 1;

l >= sizeof buf

>  		if (buf[l-1] != '\n') buf[l++] = '\n';
>  		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
>  	}
> 

Re: Weird bug in syslog

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 82 bytes --]

syslog fix

proper bounds checks, size_t len and no unlock (wrapper should do it)

[-- Attachment #2: musl-syslog.diff --]
[-- Type: text/x-diff, Size: 1281 bytes --]

diff --git a/src/misc/syslog.c b/src/misc/syslog.c
index 8de34f8..ef8dcfb 100644
--- a/src/misc/syslog.c
+++ b/src/misc/syslog.c
@@ -8,6 +8,7 @@
 #include <signal.h>
 #include <string.h>
 #include <pthread.h>
+#include <limits.h>
 #include "libc.h"
 
 static int lock[2];
@@ -71,14 +72,11 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	struct tm tm;
 	char buf[256];
 	int pid;
-	int l, l2;
+	size_t l, l2;
 
 	if (log_fd < 0) {
 		__openlog(log_ident, log_opt | LOG_NDELAY, log_facility);
-		if (log_fd < 0) {
-			UNLOCK(lock);
-			return;
-		}
+		if (log_fd < 0) return;
 	}
 
 	now = time(NULL);
@@ -90,14 +88,13 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 		priority, timebuf,
 		log_ident ? log_ident : "",
 		"["+!pid, pid, "]"+!pid);
+	if (l >= sizeof buf) l = sizeof buf - 1;
 	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
-	if (l2 >= 0) {
-		l += l2;
-		if (buf[l-1] != '\n') buf[l++] = '\n';
-		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
-	}
-
-	UNLOCK(lock);
+	if (l2 > INT_MAX) return;
+	l += l2;
+	if (l >= sizeof buf) l = sizeof buf - 1;
+	if (buf[l-1] != '\n') buf[l++] = '\n';
+	sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
 }
 
 void __vsyslog(int priority, const char *message, va_list ap)

Re: Weird bug in syslog

[Rich Felker]

On Wed, Mar 20, 2013 at 01:41:26PM +0100, Szabolcs Nagy wrote:
> >  	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
> >  	if (l2 >= 0) {
> >  		l += l2;
> 
> these are int values
> maybe we should care about overflow
> (eg making l size_t works)

Not needed. snprintf returns int. But it *can* fail: EOVERFLOW.

Rich

Re: Weird bug in syslog

[William Haddon]

On 03/20/2013 08:41:26 AM, Szabolcs Nagy wrote:
> > --- musl-0.9.9/src/misc/syslog.c
> > +++ src/src/misc/syslog.c
> > @@ -90,9 +90,11 @@
> >  		priority, timebuf,
> >  		log_ident ? log_ident : "",
> >  		"["+!pid, pid, "]"+!pid);
> > +	if (l > sizeof buf) l = sizeof buf - 1;
> 
> l >= sizeof buf

Oops.