logger: Segmentation fault when reading from stdin and writing to socket

logger: Segmentation fault when reading from stdin and writing to socket

[Patrick Plagwitz]

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

Hi,

I have noticed a bug within the development version of the logger
utility (misc-utils/logger.c).
When reading messages from stdin (so that logger_stdin is called) and
outputting to a TCP/UDP socket (inet_socket) logger SEGFAULTs:

$ echo foo | logger -n localhost
Segmentation fault (core dumped)

This bug happens because, in this combination, the syslog header isn't
generated before calling strlen() on it and has probably been introduced
somewhere when separating writing the header and writing the message.
Calling generate_syslog_header in logger_open also for inet sockets
fixes this.
The attached patch does just that.

On an unrelated note, the write_output function leaks memory by not
freeing the buf local variable.

Patrick

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch; name="patch.diff", Size: 855 bytes --]

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index edc9483..fbe8ced 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -582,14 +582,15 @@ static void logger_open(struct logger_ctl *ctl)
 		ctl->fd = inet_socket(ctl->server, ctl->port, ctl->socket_type);
 		if (!ctl->syslogfp)
 			ctl->syslogfp = syslog_rfc5424_header;
-		return;
+	} else {
+		if (!ctl->unix_socket)
+			ctl->unix_socket = _PATH_DEVLOG;
+
+		ctl->fd = unix_socket(ctl, ctl->unix_socket, ctl->socket_type);
+		if (!ctl->syslogfp)
+			ctl->syslogfp = syslog_local_header;
 	}
-	if (!ctl->unix_socket)
-		ctl->unix_socket = _PATH_DEVLOG;
 
-	ctl->fd = unix_socket(ctl, ctl->unix_socket, ctl->socket_type);
-	if (!ctl->syslogfp)
-		ctl->syslogfp = syslog_local_header;
 	if (!ctl->tag)
 		ctl->tag = xgetlogin();
 	generate_syslog_header(ctl);

Re: logger: Segmentation fault when reading from stdin and writing to socket

[Sami Kerola]

On Mon, 6 Apr 2015, Patrick Plagwitz wrote:

> I have noticed a bug within the development version of the logger
> utility (misc-utils/logger.c).
> When reading messages from stdin (so that logger_stdin is called) and
> outputting to a TCP/UDP socket (inet_socket) logger SEGFAULTs:
> 
> $ echo foo | logger -n localhost
> Segmentation fault (core dumped)
> 
> This bug happens because, in this combination, the syslog header isn't
> generated before calling strlen() on it and has probably been introduced
> somewhere when separating writing the header and writing the message.
> Calling generate_syslog_header in logger_open also for inet sockets
> fixes this.
> The attached patch does just that.
> 
> On an unrelated note, the write_output function leaks memory by not
> freeing the buf local variable.

Hi Patrick and others,

The below appears to fix the issue. And this reminds me logger(1) ought to 
have 'remote' logging tests writing to netcat(1) server.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Mon, 6 Apr 2015 22:42:54 +0100
Subject: [PATCH] logger: generate header when reading message from stdin

This change fixes crashing error, that ought not to be simply avoided.

$ echo foo | logger -n localhost
Segmentation fault (core dumped)

If the ctl->hdr is just checked not to be NULL syslog message will not
have valid header, so generating such is not optional when reading
message from stdin and writing it to remote destination.

Reported-by: Patrick Plagwitz <patrick.plagwitz@fau.de>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/logger.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index edc9483..753cd7f 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -582,14 +582,14 @@ static void logger_open(struct logger_ctl *ctl)
 		ctl->fd = inet_socket(ctl->server, ctl->port, ctl->socket_type);
 		if (!ctl->syslogfp)
 			ctl->syslogfp = syslog_rfc5424_header;
-		return;
-	}
-	if (!ctl->unix_socket)
-		ctl->unix_socket = _PATH_DEVLOG;
+	} else {
+		if (!ctl->unix_socket)
+			ctl->unix_socket = _PATH_DEVLOG;
 
-	ctl->fd = unix_socket(ctl, ctl->unix_socket, ctl->socket_type);
-	if (!ctl->syslogfp)
-		ctl->syslogfp = syslog_local_header;
+		ctl->fd = unix_socket(ctl, ctl->unix_socket, ctl->socket_type);
+		if (!ctl->syslogfp)
+			ctl->syslogfp = syslog_local_header;
+	}
 	if (!ctl->tag)
 		ctl->tag = xgetlogin();
 	generate_syslog_header(ctl);
-- 
2.3.5

Re: logger: Segmentation fault when reading from stdin and writing to socket

[Bernhard Voelker]

On 04/06/2015 11:50 PM, Sami Kerola wrote:
> On Mon, 6 Apr 2015, Patrick Plagwitz wrote:
>> The attached patch does just that.

> The below appears to fix the issue. 

Sami, your patch differs from Patrick's just in a newline, and you only
turned his - already great - description into a commit message.
Therefore I think setting Patrick as the author would be both correct
and just fair.

Please note that seeing the own name in the commit log encourages
people to continue contributing in future.

BTW: what about the memleak he reported?

Thanks & have a nice day,
Berny

Re: logger: Segmentation fault when reading from stdin and writing to socket

[Sami Kerola]

On 7 April 2015 at 07:36, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> On 04/06/2015 11:50 PM, Sami Kerola wrote:
>> On Mon, 6 Apr 2015, Patrick Plagwitz wrote:
>>> The attached patch does just that.
>
>> The below appears to fix the issue.
>
> Sami, your patch differs from Patrick's just in a newline, and you only
> turned his - already great - description into a commit message.
> Therefore I think setting Patrick as the author would be both correct
> and just fair.
>
> Please note that seeing the own name in the commit log encourages
> people to continue contributing in future.

Hi Bernhard,

Oh, I did not even notice there was an attachment.  What comes to attributing
contributors you are absolutely right. Adjusted change can be found from my
remote branch logger-fix.

https://github.com/kerolasa/lelux-utiliteetit/commit/4a8919a4e5b28a47cd61fc8b774a6eaee943b90e

> BTW: what about the memleak he reported?

Can be fixed. And since starting to look this I took liberty to check
with valgrind
if there are other leaks. One more found from logger_command_line().

https://github.com/kerolasa/lelux-utiliteetit/commit/c3dd2ecd5fcaf30860d5fcfd74edfd70a3dd7603

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: logger: Segmentation fault when reading from stdin and writing to socket

[Karel Zak]

On Tue, Apr 07, 2015 at 09:00:25AM +0100, Sami Kerola wrote:
> On 7 April 2015 at 07:36, Bernhard Voelker <mail@bernhard-voelker.de> wrote:
> > On 04/06/2015 11:50 PM, Sami Kerola wrote:
> >> On Mon, 6 Apr 2015, Patrick Plagwitz wrote:
> https://github.com/kerolasa/lelux-utiliteetit/commit/4a8919a4e5b28a47cd61fc8b774a6eaee943b90e
> https://github.com/kerolasa/lelux-utiliteetit/commit/c3dd2ecd5fcaf30860d5fcfd74edfd70a3dd7603

 Thanks Patrick and Sami.

 ...by the way -- it means we don't have stdin logger tests at all!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com