[BUG] Multiple syslog channels issue

[michallenc]

Description / Steps to reproduce the issue

Issue reported by Martin Krasula, forwarding to Github.

Description

When using multiple channels for syslog output (for example, combining the NSH console, USB CDCACM, and/or logging to
a file stored in NOR flash memory), an infinite loop (cyclical syslog output) can occur.

This issue happens because the functions syslograwstream_addstring and syslograwstream_addchar do not check and handle the return value of syslograwstream_flush.

The syslograwstream_flush function can return a negative errno propagated from syslog_write -> syslog_write_foreach -> write.
This typically happens if a write operation to the syslog file fails, or when the USB CDCACM interface for syslog is disconnected.

Affected files

• libs/libc/stream/lib_syslograwstream.c
• drivers/syslog/syslog_write.c

Possible Solution

The solution would be to properly handle the return value from syslograwstream_flush to prevent the infinite loop when a errno occurs.
Please see the attached patch for more details.

diff --git a/libs/libc/stream/lib_syslograwstream.c b/libs/libc/stream/lib_syslograwstream.c
index 76b149f1d3..61154deb01 100644
--- a/libs/libc/stream/lib_syslograwstream.c
+++ b/libs/libc/stream/lib_syslograwstream.c
@@ -123,7 +123,13 @@ static void syslograwstream_addchar(FAR struct lib_syslograwstream_s *stream,
 
       /* Yes.. then flush the buffer */
 
-      syslograwstream_flush(&stream->common);
+      int ret = syslograwstream_flush(&stream->common);
+      if (ret < 0)
+      {
+        fprintf(stderr, "syslograwstream_flush: %d char: %c\n", ret, ch);
+        stream->offset = 0;
+        stream->common.nput = 0;
+      }
     }
 }
 
@@ -136,7 +142,6 @@ syslograwstream_addstring(FAR struct lib_syslograwstream_s *stream,
                           FAR const char *buff, size_t len)
 {
   ssize_t ret = 0;
-
   do
     {
       size_t remain = CONFIG_SYSLOG_BUFSIZE - stream->offset;
@@ -154,8 +159,15 @@ syslograwstream_addstring(FAR struct lib_syslograwstream_s *stream,
           stream->offset = CONFIG_SYSLOG_BUFSIZE + 1;
 
           /* Yes.. then flush the buffer */
+          int ret_flush = syslograwstream_flush(&stream->common);
+          if (ret_flush < 0)
+          {
+             fprintf(stderr, "syslograwstream_flush: %d\n", ret_flush);
+             stream->offset = 0;
+             stream->common.nput = 0;
+             break;
+          }
 
-          syslograwstream_flush(&stream->common);
         }
     }
   while (ret < len);

On which OS does this issue occur?

[OS: Other], [OS: Linux], [OS: Windows], [OS: Mac], [OS: BSD]

What is the version of your OS?

NuttX

NuttX Version

master

Issue Architecture

[Arch: all]

Issue Area

[Area: OS Components]

Host information

No response

Verification

• I have verified before submitting the report.

[michallenc]

I think the solution for lib_syslograwstream.c is good. We should also fix the behavior of syslog_write_foreach in drivers/syslog/syslog_write.c. We return a negated value if write fails, but I don't think that is a correct behavior if multiple syslog channels are used. We should still log to other channels if write to one channel results in error (disconnected USB, faulty NOR flash etc...). I would suggest to do something like this in addition to the changes proposed by Martin.

diff --git a/drivers/syslog/syslog_write.c b/drivers/syslog/syslog_write.c
index 8b3fefda70..938c3f9412 100644
--- a/drivers/syslog/syslog_write.c
+++ b/drivers/syslog/syslog_write.c
@@ -107,7 +107,7 @@ ssize_t syslog_write_foreach(FAR const char *buffer,
   syslog_write_t write;
   syslog_putc_t  putc;
   size_t nwritten = 0;
-  size_t nwritten_max = 0;
+  ssize_t nwritten_max = -1;
   ssize_t ret;
   int i;
 
@@ -153,7 +153,7 @@ ssize_t syslog_write_foreach(FAR const char *buffer,
 
                   if (ret < 0)
                     {
-                      return ret;
+                      continue;
                     }
 
                   nwritten = head + 1;
@@ -166,7 +166,7 @@ ssize_t syslog_write_foreach(FAR const char *buffer,
               ret = write(channel, buffer + nwritten, buflen - nwritten);
               if (ret < 0)
                 {
-                  return ret;
+                  continue;
                 }
               else
                 {

This way we skip the faulty channel, but will still attempt to write to the other channels. We return -1 if all channels fail or the maximum bytes written to one of the channels if at least one worked. The current implementation has another issue - we don't provide syslog output at all if the faulty channel is the first one.

The issue is easily reproducible if you try to log both to standard serial output and to CDC ACM device. Disconnecting USB then leads to syslog_write_foreach and eventually to an infinite loop in syslograwstream_addstring.

ping for relevance @acassis @xiaoxiang781216 @linguini1 @cederom @raiden00pl @jerpelea

[linguini1]

I agree with the proposals here.

[acassis]

@michallenc I agree with this approach too. I didn't study the syslog channels, but what will happen after the first fail in that channel, will this logic still trying write to that channel that failed (it could be a good approach case the channel is able to recover, since it doesn't incur in some penalty to the other channels).