[PATCH v3 0/7] Refactor syslog implementation

[PATCH v3 0/7] Refactor syslog implementation

[Adhemerval Zanella]

The main driver of this change is to move away of using 32-bit
timestamps.  Based on discussion where systemd should support
RFC5424 [1], it decided to move away from implementing on glibc [2]
and just replace the use of localtime by gmtime.  It is a deviation
from RFC3164, but it improves some corner cases [3].

[1] https://github.com/systemd/systemd/issues/19251
[2] https://sourceware.org/pipermail/libc-alpha/2022-February/136595.html
[3] https://sourceware.org/pipermail/libc-alpha/2021-March/123583.html

Adhemerval Zanella (7):
  support: Add xmkfifo
  misc: Add syslog test
  misc: syslog: Fix indentation and style
  misc: syslog: Simplify implementation
  misc: syslog: Use fixed-sized buffer
  misc: syslog: Move SYSLOG_NAME to USE_MISC (BZ #16355)
  misc: Use gmtime instead of localtime

 misc/Makefile     |   2 +
 misc/sys/syslog.h |   4 +-
 misc/syslog.c     | 488 +++++++++++++++++++++++-----------------------
 misc/tst-syslog.c | 477 ++++++++++++++++++++++++++++++++++++++++++++
 support/Makefile  |   1 +
 support/xmkfifo.c |  29 +++
 support/xunistd.h |   1 +
 7 files changed, 756 insertions(+), 246 deletions(-)
 create mode 100644 misc/tst-syslog.c
 create mode 100644 support/xmkfifo.c

-- 
2.32.0

[PATCH v3 1/7] support: Add xmkfifo

[Adhemerval Zanella]

Wrapper support mkfifo.
---
 support/Makefile  |  1 +
 support/xmkfifo.c | 29 +++++++++++++++++++++++++++++
 support/xunistd.h |  1 +
 3 files changed, 31 insertions(+)
 create mode 100644 support/xmkfifo.c

diff --git a/support/Makefile b/support/Makefile
index 5ddcb8d158..c3609e211b 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -133,6 +133,7 @@ libsupport-routines = \
   xmemstream \
   xmkdir \
   xmkdirp \
+  xmkfifo \
   xmmap \
   xmprotect \
   xmunmap \
diff --git a/support/xmkfifo.c b/support/xmkfifo.c
new file mode 100644
index 0000000000..a8e196dbc2
--- /dev/null
+++ b/support/xmkfifo.c
@@ -0,0 +1,29 @@
+/* mkfifo with error checking.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <support/check.h>
+#include <support/xunistd.h>
+#include <sys/stat.h>
+
+void
+xmkfifo (const char *pathname, mode_t mode)
+{
+  int r = mkfifo (pathname, mode);
+  if (r < 0)
+    FAIL_EXIT1 ("mkfifo (%s, 0%o): %m", pathname, mode);
+}
diff --git a/support/xunistd.h b/support/xunistd.h
index 0454d83cf1..960a62d412 100644
--- a/support/xunistd.h
+++ b/support/xunistd.h
@@ -61,6 +61,7 @@ void xsymlink (const char *target, const char *linkpath);
 void xchdir (const char *path);
 void xfchmod (int fd, mode_t mode);
 void xchmod (const char *pathname, mode_t mode);
+void xmkfifo (const char *pathname, mode_t mode);
 
 /* Equivalent of "mkdir -p".  */
 void xmkdirp (const char *, mode_t);
-- 
2.32.0

[PATCH v3 2/7] misc: Add syslog test

[Adhemerval Zanella]

The test cover:

  - All possible priorities and facilities through TCP and UDP.
  - Same syslog tests for vsyslog.
  - Some openlog/syslog/close combinations.
  - openlog with LOG_CONS, LOG_PERROR, and LOG_PID.

Internally is done with a test-container where the main process mimics
the syslog server interface.

The test does not cover multithread and async-signal usage.

Checked on x86_64-linux-gnu.
---
 misc/Makefile     |   2 +
 misc/tst-syslog.c | 477 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 479 insertions(+)
 create mode 100644 misc/tst-syslog.c

diff --git a/misc/Makefile b/misc/Makefile
index 3d8a569d06..ba8232a0e9 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -115,6 +115,8 @@ tests-special += $(objpfx)tst-error1-mem.out \
   $(objpfx)tst-allocate_once-mem.out
 endif
 
+tests-container := tst-syslog
+
 CFLAGS-select.c += -fexceptions -fasynchronous-unwind-tables
 CFLAGS-tsearch.c += $(uses-callbacks)
 CFLAGS-lsearch.c += $(uses-callbacks)
diff --git a/misc/tst-syslog.c b/misc/tst-syslog.c
new file mode 100644
index 0000000000..8505178616
--- /dev/null
+++ b/misc/tst-syslog.c
@@ -0,0 +1,477 @@
+/* Basic tests for syslog interfaces.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <fcntl.h>
+#include <paths.h>
+#include <netinet/in.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/temp_file.h>
+#include <support/xstdio.h>
+#include <support/xsocket.h>
+#include <support/xunistd.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdlib.h>
+#include <syslog.h>
+#include <sys/un.h>
+
+static const int facilities[] =
+  {
+    LOG_KERN,
+    LOG_USER,
+    LOG_MAIL,
+    LOG_DAEMON,
+    LOG_AUTH,
+    LOG_SYSLOG,
+    LOG_LPR,
+    LOG_NEWS,
+    LOG_UUCP,
+    LOG_CRON,
+    LOG_AUTHPRIV,
+    LOG_FTP,
+    LOG_LOCAL0,
+    LOG_LOCAL1,
+    LOG_LOCAL2,
+    LOG_LOCAL3,
+    LOG_LOCAL4,
+    LOG_LOCAL5,
+    LOG_LOCAL6,
+    LOG_LOCAL7,
+  };
+
+static const int priorities[] =
+  {
+    LOG_EMERG,
+    LOG_ALERT,
+    LOG_CRIT,
+    LOG_ERR,
+    LOG_WARNING,
+    LOG_NOTICE,
+    LOG_INFO,
+    LOG_DEBUG
+  };
+
+enum
+  {
+    ident_length = 64,
+    msg_length = 64
+  };
+
+#define SYSLOG_MSG_BASE "syslog_message"
+#define OPENLOG_IDENT   "openlog_ident"
+
+struct msg_t
+  {
+    int priority;
+    int facility;
+    char ident[ident_length];
+    char msg[msg_length];
+    pid_t pid;
+  };
+
+static void
+call_vsyslog (int priority, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vsyslog (priority, format, ap);
+  va_end (ap);
+}
+
+static void
+send_vsyslog (int options)
+{
+  for (size_t i = 0; i < array_length (facilities); i++)
+    {
+      for (size_t j = 0; j < array_length (priorities); j++)
+        {
+          int facility = facilities[i];
+          int priority = priorities[j];
+          call_vsyslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE,
+                        facility, priority);
+        }
+    }
+}
+
+static void
+send_syslog (int options)
+{
+  for (size_t i = 0; i < array_length (facilities); i++)
+    {
+      for (size_t j = 0; j < array_length (priorities); j++)
+        {
+          int facility = facilities[i];
+          int priority = priorities[j];
+          syslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE, facility,
+                  priority);
+        }
+    }
+}
+
+static bool
+check_syslog_message (const struct msg_t *msg, int msgnum, int options,
+                      pid_t pid)
+{
+  if (msgnum == array_length (facilities) * array_length (priorities) - 1)
+    return false;
+
+  int i = msgnum / array_length (priorities);
+  int j = msgnum % array_length (priorities);
+
+  int expected_facility = facilities[i];
+  /* With no preceding openlog, syslog default to LOG_USER.  */
+  if (expected_facility == LOG_KERN)
+      expected_facility = LOG_USER;
+  int expected_priority = priorities[j];
+
+  TEST_COMPARE (msg->facility, expected_facility);
+  TEST_COMPARE (msg->priority, expected_priority);
+
+  return true;
+}
+
+static void
+send_openlog (int options)
+{
+  /* Define a non-default IDENT and a not default facility.  */
+  openlog (OPENLOG_IDENT, options, LOG_LOCAL0);
+  for (size_t j = 0; j < array_length (priorities); j++)
+    {
+      int priority = priorities[j];
+      syslog (priority, "%s %d %d", SYSLOG_MSG_BASE, LOG_LOCAL0, priority);
+    }
+  closelog ();
+
+  /* Back to the default IDENT with a non default facility.  */
+  openlog (NULL, 0, LOG_LOCAL6);
+  for (size_t j = 0; j < array_length (priorities); j++)
+    {
+      int priority = priorities[j];
+      syslog (LOG_LOCAL7 | priority, "%s %d %d", SYSLOG_MSG_BASE, LOG_LOCAL7,
+        priority);
+    }
+  closelog ();
+
+  /* LOG_KERN does not change the internal default facility.  */
+  openlog (NULL, 0, LOG_KERN);
+  for (size_t j = 0; j < array_length (priorities); j++)
+    {
+      int priority = priorities[j];
+      syslog (priority, "%s %d %d", SYSLOG_MSG_BASE, LOG_KERN, priority);
+    }
+  closelog ();
+}
+
+static bool
+check_openlog_message (const struct msg_t *msg, int msgnum,
+                       int options, pid_t pid)
+{
+  if (msgnum == 3 * array_length (priorities) - 1)
+    return false;
+
+  int expected_priority = priorities[msgnum % array_length (priorities)];
+  TEST_COMPARE (msg->priority, expected_priority);
+
+  char expected_ident[ident_length];
+  snprintf (expected_ident, sizeof (expected_ident), "%s%s%.0d%s:",
+            OPENLOG_IDENT,
+            options & LOG_PID ? "[" : "",
+            options & LOG_PID ? pid : 0,
+            options & LOG_PID ? "]" : "");
+
+  if (msgnum < array_length (priorities))
+    {
+      if (options & LOG_PID)
+        TEST_COMPARE (msg->pid, pid);
+      TEST_COMPARE_STRING (msg->ident, expected_ident);
+      TEST_COMPARE (msg->facility, LOG_LOCAL0);
+    }
+  else if (msgnum < 2 * array_length (priorities))
+    TEST_COMPARE (msg->facility, LOG_LOCAL7);
+  else if (msgnum < 3 * array_length (priorities))
+    TEST_COMPARE (msg->facility, LOG_KERN);
+
+  return true;
+}
+
+static struct msg_t
+parse_syslog_msg (const char *msg)
+{
+  struct msg_t r = { .pid = -1 };
+  int number;
+
+  /* The message in the form:
+     <179>Apr  8 14:51:19 tst-syslog: syslog message 176 3  */
+  int n = sscanf (msg, "<%3d>%*s %*d %*d:%*d:%*d %32s %64s %*d %*d",
+                  &number, r.ident, r.msg);
+  TEST_COMPARE (n, 3);
+
+  r.facility = number & LOG_FACMASK;
+  r.priority = number & LOG_PRIMASK;
+
+  char *pid_start = strchr (r.ident, '[');
+  if (pid_start != NULL)
+    {
+       char *pid_end = strchr (r.ident, ']');
+       if (pid_end != NULL)
+         r.pid = strtoul (pid_start + 1, NULL, 10);
+    }
+
+  return r;
+}
+
+static struct msg_t
+parse_syslog_console (const char *msg)
+{
+  int priority;
+  int facility;
+  struct msg_t r;
+
+  /* The message in the form:
+     openlog_ident: syslog_message 128 0  */
+  int n = sscanf (msg, "%32s %64s %d %d",
+      r.ident, r.msg, &facility, &priority);
+  TEST_COMPARE (n, 4);
+
+  r.facility = facility;
+  r.priority = priority;
+
+  return r;
+}
+
+static void
+check_syslog_udp (void (*syslog_send)(int), int options,
+                  bool (*syslog_check)(const struct msg_t *, int, int,
+                                       pid_t))
+{
+  struct sockaddr_un addr =
+    {
+      .sun_family = AF_UNIX,
+      .sun_path = _PATH_LOG
+    };
+
+  socklen_t addrlen = sizeof (addr);
+  int server_udp = xsocket (AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+  xbind (server_udp, (struct sockaddr *) &addr, addrlen);
+
+  pid_t sender_pid = xfork ();
+  if (sender_pid == 0)
+    {
+      syslog_send (options);
+      _exit (0);
+    }
+
+  int msgnum = 0;
+  while (1)
+    {
+      char buf[512];
+      size_t l = xrecvfrom (server_udp, buf, sizeof (buf), 0,
+                            (struct sockaddr *) &addr, &addrlen);
+      buf[l] = '\0';
+
+      struct msg_t msg = parse_syslog_msg (buf);
+      if (!syslog_check (&msg, msgnum++, options, sender_pid))
+        break;
+     }
+
+  xclose (server_udp);
+
+  int status;
+  xwaitpid (sender_pid, &status, 0);
+  TEST_COMPARE (status, 0);
+
+  unlink (_PATH_LOG);
+}
+
+static void
+check_syslog_tcp (void (*syslog_send)(int), int options,
+                  bool (*syslog_check)(const struct msg_t *, int, int,
+                                       pid_t))
+{
+  struct sockaddr_un addr =
+    {
+      .sun_family = AF_UNIX,
+      .sun_path = _PATH_LOG
+    };
+  socklen_t addrlen = sizeof (addr);
+
+  int server_tcp = xsocket (AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);
+  xbind (server_tcp, (struct sockaddr *) &addr, addrlen);
+  xlisten (server_tcp, 5);
+
+  pid_t sender_pid = xfork ();
+  if (sender_pid == 0)
+    {
+      syslog_send (options);
+      _exit (0);
+    }
+
+  int client_tcp = xaccept (server_tcp, NULL, NULL);
+
+  char buf[512], *rb = buf;
+  size_t rbl = sizeof (buf);
+  size_t prl = 0;  /* Track the size of the partial record.  */
+  int msgnum = 0;
+
+  while (1)
+    {
+      size_t rl = xrecvfrom (client_tcp, rb, rbl - prl, 0, NULL, NULL);
+      if (rl == 0)
+        break;
+
+      /* Iterate over the buffer to find and check the record.  */
+      size_t l = rl + prl;
+      char *b = buf;
+      while (1)
+	{
+          /* With TCP each record ends with a '\0'.  */
+          char *e = memchr (b, '\0', l);
+          if (e != NULL)
+            {
+              struct msg_t msg = parse_syslog_msg (b);
+              if (!syslog_check (&msg, msgnum++, options, sender_pid))
+                break;
+
+	      /* Advance to the next record.  */
+	      ptrdiff_t diff = e + 1 - b;
+	      b += diff;
+	      l -= diff;
+	    }
+	  else
+	    {
+              /* Move the partial record to the start of the buffer.  */
+	      memmove (buf, b, l);
+	      rb = buf + l;
+	      prl = l;
+	      break;
+            }
+        }
+    }
+
+  xclose (client_tcp);
+  xclose (server_tcp);
+
+  int status;
+  xwaitpid (sender_pid, &status, 0);
+  TEST_COMPARE (status, 0);
+
+  unlink (_PATH_LOG);
+}
+
+static void
+check_syslog_console_read (FILE *fp)
+{
+  char buf[512];
+  int msgnum = 0;
+  while (fgets (buf, sizeof (buf), fp) != NULL)
+    {
+      struct msg_t msg = parse_syslog_console (buf);
+      TEST_COMPARE_STRING (msg.ident, OPENLOG_IDENT ":");
+      TEST_COMPARE (msg.priority, priorities[msgnum]);
+      TEST_COMPARE (msg.facility, LOG_LOCAL0);
+
+      if (++msgnum == array_length (priorities))
+        break;
+    }
+}
+
+static void
+check_syslog_console (void)
+{
+  xmkfifo (_PATH_CONSOLE, 0666);
+
+  pid_t sender_pid = xfork ();
+  if (sender_pid == 0)
+    {
+      send_openlog (LOG_CONS);
+      _exit (0);
+    }
+
+  {
+    FILE *fp = xfopen (_PATH_CONSOLE, "r+");
+    check_syslog_console_read (fp);
+    xfclose (fp);
+  }
+
+  int status;
+  xwaitpid (sender_pid, &status, 0);
+  TEST_COMPARE (status, 0);
+
+  unlink (_PATH_CONSOLE);
+}
+
+static void
+send_openlog_callback (void *clousure)
+{
+  int options = *(int *) clousure;
+  send_openlog (options);
+}
+
+static void
+check_syslog_perror (void)
+{
+  struct support_capture_subprocess result;
+  result = support_capture_subprocess (send_openlog_callback,
+                                       &(int){LOG_PERROR});
+
+  FILE *mfp = fmemopen (result.err.buffer, result.err.length, "r");
+  if (mfp == NULL)
+    FAIL_EXIT1 ("fmemopen: %m");
+  check_syslog_console_read (mfp);
+  xfclose (mfp);
+
+  support_capture_subprocess_check (&result, "tst-openlog-child", 0,
+                                    sc_allow_stderr);
+  support_capture_subprocess_free (&result);
+}
+
+static int
+do_test (void)
+{
+  add_temp_file (_PATH_LOG);
+  add_temp_file (_PATH_CONSOLE);
+
+  /* Send every combination of facility/priority over UDP and TCP.  */
+  check_syslog_udp (send_syslog, 0, check_syslog_message);
+  check_syslog_tcp (send_syslog, 0, check_syslog_message);
+
+  /* Also check vsyslog.  */
+  check_syslog_udp (send_vsyslog, 0, check_syslog_message);
+  check_syslog_tcp (send_vsyslog, 0, check_syslog_message);
+
+  /* Run some openlog/syslog/closelog combinations.  */
+  check_syslog_udp (send_openlog, 0, check_openlog_message);
+  check_syslog_tcp (send_openlog, 0, check_openlog_message);
+
+  /* Check the LOG_PID option.  */
+  check_syslog_udp (send_openlog, LOG_PID, check_openlog_message);
+  check_syslog_tcp (send_openlog, LOG_PID, check_openlog_message);
+
+  /* Check the LOG_CONS option.  */
+  check_syslog_console ();
+
+  /* Check the LOG_PERROR option.  */
+  check_syslog_perror ();
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.32.0

[PATCH v3 3/7] misc: syslog: Fix indentation and style

[Adhemerval Zanella]

And also clenaup the headers, no semantic changes.
---
 misc/syslog.c | 471 ++++++++++++++++++++++++--------------------------
 1 file changed, 228 insertions(+), 243 deletions(-)

diff --git a/misc/syslog.c b/misc/syslog.c
index ee83b1bb76..0736459e7b 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -31,49 +31,33 @@
 static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
 #endif /* LIBC_SCCS and not lint */
 
-#include <sys/types.h>
-#include <sys/socket.h>
-#include <sys/syslog.h>
-#include <sys/uio.h>
-#include <sys/un.h>
-#include <netdb.h>
-
-#include <errno.h>
-#include <fcntl.h>
+#include <libio/libioP.h>
 #include <paths.h>
+#include <stdarg.h>
+#include <stdlib.h>
 #include <stdio.h>
 #include <stdio_ext.h>
-#include <string.h>
-#include <time.h>
-#include <unistd.h>
-#include <stdlib.h>
-#include <libc-lock.h>
-#include <signal.h>
-#include <locale.h>
-
-#include <stdarg.h>
-
-#include <libio/libioP.h>
-#include <math_ldbl_opt.h>
-
-#include <kernel-features.h>
+#include <sys/socket.h>
+#include <sys/uio.h>
+#include <sys/un.h>
+#include <syslog.h>
 
 #define ftell(s) _IO_ftell (s)
 
-static int	LogType = SOCK_DGRAM;	/* type of socket connection */
-static int	LogFile = -1;		/* fd for log */
-static bool	connected;		/* have done connect */
-static int	LogStat;		/* status bits, set by openlog() */
+static int LogType = SOCK_DGRAM;	/* type of socket connection */
+static int LogFile = -1;		/* fd for log */
+static bool connected;			/* have done connect */
+static int LogStat;			/* status bits, set by openlog() */
 static const char *LogTag;		/* string to tag the entry with */
-static int	LogFacility = LOG_USER;	/* default facility code */
-static int	LogMask = 0xff;		/* mask of priorities to be logged */
-extern char	*__progname;		/* Program name, from crt0. */
+static int LogFacility = LOG_USER;	/* default facility code */
+static int LogMask = 0xff;		/* mask of priorities to be logged */
+extern char *__progname;		/* Program name, from crt0. */
 
 /* Define the lock.  */
 __libc_lock_define_initialized (static, syslog_lock)
 
-static void openlog_internal(const char *, int, int);
-static void closelog_internal(void);
+static void openlog_internal (const char *, int, int);
+static void closelog_internal (void);
 
 struct cleanup_arg
 {
@@ -101,205 +85,204 @@ cancel_handler (void *ptr)
  *	print message on log file; output is intended for syslogd(8).
  */
 void
-__syslog(int pri, const char *fmt, ...)
+__syslog (int pri, const char *fmt, ...)
 {
-	va_list ap;
+  va_list ap;
 
-	va_start(ap, fmt);
-	__vsyslog_internal(pri, fmt, ap, 0);
-	va_end(ap);
+  va_start (ap, fmt);
+  __vsyslog_internal (pri, fmt, ap, 0);
+  va_end (ap);
 }
 ldbl_hidden_def (__syslog, syslog)
 ldbl_strong_alias (__syslog, syslog)
 
 void
-__vsyslog(int pri, const char *fmt, va_list ap)
+__vsyslog (int pri, const char *fmt, va_list ap)
 {
-	__vsyslog_internal(pri, fmt, ap, 0);
+  __vsyslog_internal (pri, fmt, ap, 0);
 }
 ldbl_weak_alias (__vsyslog, vsyslog)
 
 void
-__syslog_chk(int pri, int flag, const char *fmt, ...)
+__syslog_chk (int pri, int flag, const char *fmt, ...)
 {
-	va_list ap;
+  va_list ap;
 
-	va_start(ap, fmt);
-	__vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
-	va_end(ap);
+  va_start (ap, fmt);
+  __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
+  va_end (ap);
 }
 
 void
 __vsyslog_chk(int pri, int flag, const char *fmt, va_list ap)
 {
-	__vsyslog_internal(pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
+  __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
 }
 
 void
 __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		   unsigned int mode_flags)
 {
-	struct tm now_tm;
-	time_t now;
-	int fd;
-	FILE *f;
-	char *buf = 0;
-	size_t bufsize = 0;
-	size_t msgoff;
-	int saved_errno = errno;
-	char failbuf[3 * sizeof (pid_t) + sizeof "out of memory []"];
-
-#define	INTERNALLOG	LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
-	/* Check for invalid bits. */
-	if (pri & ~(LOG_PRIMASK|LOG_FACMASK)) {
-		syslog(INTERNALLOG,
-		    "syslog: unknown facility/priority: %x", pri);
-		pri &= LOG_PRIMASK|LOG_FACMASK;
+  struct tm now_tm;
+  time_t now;
+  int fd;
+  FILE *f;
+  char *buf = 0;
+  size_t bufsize = 0;
+  size_t msgoff;
+  int saved_errno = errno;
+  char failbuf[3 * sizeof (pid_t) + sizeof "out of memory []"];
+
+#define	INTERNALLOG LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
+  /* Check for invalid bits. */
+  if (pri & ~(LOG_PRIMASK|LOG_FACMASK))
+    {
+      syslog (INTERNALLOG, "syslog: unknown facility/priority: %x", pri);
+      pri &= LOG_PRIMASK|LOG_FACMASK;
+    }
+
+  /* Prepare for multiple users.  We have to take care: most syscalls we are
+     using are cancellation points.  */
+  struct cleanup_arg clarg;
+  clarg.buf = NULL;
+  clarg.oldaction = NULL;
+  __libc_cleanup_push (cancel_handler, &clarg);
+  __libc_lock_lock (syslog_lock);
+
+  /* Check priority against setlogmask values. */
+  if ((LOG_MASK (LOG_PRI (pri)) & LogMask) == 0)
+    goto out;
+
+  /* Set default facility if none specified. */
+  if ((pri & LOG_FACMASK) == 0)
+    pri |= LogFacility;
+
+  /* Build the message in a memory-buffer stream.  */
+  f = __open_memstream (&buf, &bufsize);
+  if (f == NULL)
+    {
+      /* We cannot get a stream.  There is not much we can do but emitting an
+	 error messages.  */
+      char numbuf[3 * sizeof (pid_t)];
+      char *nump;
+      char *endp = __stpcpy (failbuf, "out of memory [");
+      pid_t pid = __getpid ();
+
+      nump = numbuf + sizeof (numbuf);
+      /* The PID can never be zero.  */
+      do
+	*--nump = '0' + pid % 10;
+      while ((pid /= 10) != 0);
+
+      endp = __mempcpy (endp, nump, (numbuf + sizeof (numbuf)) - nump);
+      *endp++ = ']';
+      *endp = '\0';
+      buf = failbuf;
+      bufsize = endp - failbuf;
+      msgoff = 0;
+    }
+  else
+    {
+      __fsetlocking (f, FSETLOCKING_BYCALLER);
+      fprintf (f, "<%d>", pri);
+      now = time_now ();
+      f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
+					f->_IO_write_end - f->_IO_write_ptr,
+					"%h %e %T ",
+					__localtime_r (&now, &now_tm),
+					_nl_C_locobj_ptr);
+      msgoff = ftell (f);
+      if (LogTag == NULL)
+	LogTag = __progname;
+      if (LogTag != NULL)
+	__fputs_unlocked (LogTag, f);
+      if (LogStat & LOG_PID)
+	fprintf (f, "[%d]", (int) __getpid ());
+      if (LogTag != NULL)
+	{
+	  __putc_unlocked (':', f);
+	  __putc_unlocked (' ', f);
 	}
 
-	/* Prepare for multiple users.  We have to take care: most
-	   syscalls we are using are cancellation points.  */
-	struct cleanup_arg clarg;
-	clarg.buf = NULL;
-	clarg.oldaction = NULL;
-	__libc_cleanup_push (cancel_handler, &clarg);
-	__libc_lock_lock (syslog_lock);
-
-	/* Check priority against setlogmask values. */
-	if ((LOG_MASK (LOG_PRI (pri)) & LogMask) == 0)
-		goto out;
-
-	/* Set default facility if none specified. */
-	if ((pri & LOG_FACMASK) == 0)
-		pri |= LogFacility;
-
-	/* Build the message in a memory-buffer stream.  */
-	f = __open_memstream (&buf, &bufsize);
-	if (f == NULL)
-	  {
-	    /* We cannot get a stream.  There is not much we can do but
-	       emitting an error messages.  */
-	    char numbuf[3 * sizeof (pid_t)];
-	    char *nump;
-	    char *endp = __stpcpy (failbuf, "out of memory [");
-	    pid_t pid = __getpid ();
-
-	    nump = numbuf + sizeof (numbuf);
-	    /* The PID can never be zero.  */
-	    do
-	      *--nump = '0' + pid % 10;
-	    while ((pid /= 10) != 0);
-
-	    endp = __mempcpy (endp, nump, (numbuf + sizeof (numbuf)) - nump);
-	    *endp++ = ']';
-	    *endp = '\0';
-	    buf = failbuf;
-	    bufsize = endp - failbuf;
-	    msgoff = 0;
-	  }
-	else
-	  {
-	    __fsetlocking (f, FSETLOCKING_BYCALLER);
-	    fprintf (f, "<%d>", pri);
-	    now = time_now ();
-	    f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
-					      f->_IO_write_end
-					      - f->_IO_write_ptr,
-					      "%h %e %T ",
-					      __localtime_r (&now, &now_tm),
-					      _nl_C_locobj_ptr);
-	    msgoff = ftell (f);
-	    if (LogTag == NULL)
-	      LogTag = __progname;
-	    if (LogTag != NULL)
-	      __fputs_unlocked (LogTag, f);
-	    if (LogStat & LOG_PID)
-	      fprintf (f, "[%d]", (int) __getpid ());
-	    if (LogTag != NULL)
-	      {
-		__putc_unlocked (':', f);
-		__putc_unlocked (' ', f);
-	      }
-
-	    /* Restore errno for %m format.  */
-	    __set_errno (saved_errno);
-
-	    /* We have the header.  Print the user's format into the
-	       buffer.  */
-	    __vfprintf_internal (f, fmt, ap, mode_flags);
-
-	    /* Close the memory stream; this will finalize the data
-	       into a malloc'd buffer in BUF.  */
-	    fclose (f);
-
-	    /* Tell the cancellation handler to free this buffer.  */
-	    clarg.buf = buf;
-	  }
-
-	/* Output to stderr if requested. */
-	if (LogStat & LOG_PERROR) {
-		struct iovec iov[2];
-		struct iovec *v = iov;
-
-		v->iov_base = buf + msgoff;
-		v->iov_len = bufsize - msgoff;
-		/* Append a newline if necessary.  */
-		if (buf[bufsize - 1] != '\n')
-		  {
-		    ++v;
-		    v->iov_base = (char *) "\n";
-		    v->iov_len = 1;
-		  }
-
-		/* writev is a cancellation point.  */
-		(void)__writev(STDERR_FILENO, iov, v - iov + 1);
+      /* Restore errno for %m format.  */
+      __set_errno (saved_errno);
+
+      /* We have the header.  Print the user's format into the buffer.  */
+      __vfprintf_internal (f, fmt, ap, mode_flags);
+
+      /* Close the memory stream; this will finalize the data into a malloc'd
+	 buffer in BUF.  */
+      fclose (f);
+
+      /* Tell the cancellation handler to free this buffer.  */
+      clarg.buf = buf;
+    }
+
+  /* Output to stderr if requested. */
+  if (LogStat & LOG_PERROR)
+    {
+      struct iovec iov[2];
+      struct iovec *v = iov;
+
+      v->iov_base = buf + msgoff;
+      v->iov_len = bufsize - msgoff;
+      /* Append a newline if necessary.  */
+      if (buf[bufsize - 1] != '\n')
+	{
+	  ++v;
+	  v->iov_base = (char *) "\n";
+	  v->iov_len = 1;
+	}
+
+      /* writev is a cancellation point.  */
+      __writev (STDERR_FILENO, iov, v - iov + 1);
+    }
+
+  /* Get connected, output the message to the local logger.  */
+  if (!connected)
+    openlog_internal (NULL, LogStat | LOG_NDELAY, LogFacility);
+
+  /* If we have a SOCK_STREAM connection, also send ASCII NUL as a record
+     terminator.  */
+  if (LogType == SOCK_STREAM)
+    ++bufsize;
+
+  if (!connected || __send (LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
+    {
+      if (connected)
+	{
+	  /* Try to reopen the syslog connection.  Maybe it went down.  */
+	  closelog_internal ();
+	  openlog_internal (NULL, LogStat | LOG_NDELAY, LogFacility);
 	}
 
-	/* Get connected, output the message to the local logger. */
-	if (!connected)
-		openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility);
-
-	/* If we have a SOCK_STREAM connection, also send ASCII NUL as
-	   a record terminator.  */
-	if (LogType == SOCK_STREAM)
-	  ++bufsize;
-
-	if (!connected || __send(LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
-	  {
-	    if (connected)
-	      {
-		/* Try to reopen the syslog connection.  Maybe it went
-		   down.  */
-		closelog_internal ();
-		openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility);
-	      }
-
-	    if (!connected || __send(LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
-	      {
-		closelog_internal ();	/* attempt re-open next time */
-		/*
-		 * Output the message to the console; don't worry
-		 * about blocking, if console blocks everything will.
-		 * Make sure the error reported is the one from the
-		 * syslogd failure.
-		 */
-		if (LogStat & LOG_CONS &&
-		    (fd = __open(_PATH_CONSOLE, O_WRONLY|O_NOCTTY|O_CLOEXEC,
-				 0)) >= 0)
-		  {
-		    __dprintf (fd, "%s\r\n", buf + msgoff);
-		    (void)__close(fd);
-		  }
-	      }
-	  }
+      if (!connected || __send (LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
+	{
+	  closelog_internal ();	/* attempt re-open next time */
+	  /*
+	   * Output the message to the console; don't worry
+	   * about blocking, if console blocks everything will.
+	   * Make sure the error reported is the one from the
+	   * syslogd failure.
+	   */
+	  if (LogStat & LOG_CONS &&
+	      (fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
+			    | O_CLOEXEC, 0))
+	      >= 0)
+	    {
+	      __dprintf (fd, "%s\r\n", buf + msgoff);
+	      __close (fd);
+	    }
+	}
+    }
 
  out:
-	/* End of critical section.  */
-	__libc_cleanup_pop (0);
-	__libc_lock_unlock (syslog_lock);
+  /* End of critical section.  */
+  __libc_cleanup_pop (0);
+  __libc_lock_unlock (syslog_lock);
 
-	if (buf != failbuf)
-		free (buf);
+  if (buf != failbuf)
+    free (buf);
 }
 
 /* AF_UNIX address of local logger  */
@@ -312,45 +295,47 @@ static const struct sockaddr_un SyslogAddr =
 static void
 openlog_internal(const char *ident, int logstat, int logfac)
 {
-	if (ident != NULL)
-		LogTag = ident;
-	LogStat = logstat;
-	if ((logfac &~ LOG_FACMASK) == 0)
-		LogFacility = logfac;
-
-	int retry = 0;
-	while (retry < 2) {
-		if (LogFile == -1) {
-			if (LogStat & LOG_NDELAY) {
-			  LogFile = __socket(AF_UNIX, LogType | SOCK_CLOEXEC, 0);
-			  if (LogFile == -1)
-			    return;
-			}
-		}
-		if (LogFile != -1 && !connected)
+  if (ident != NULL)
+    LogTag = ident;
+  LogStat = logstat;
+  if ((logfac &~ LOG_FACMASK) == 0)
+    LogFacility = logfac;
+
+  int retry = 0;
+  while (retry < 2)
+    {
+      if (LogFile == -1)
+	{
+	  if (LogStat & LOG_NDELAY)
+	    {
+	      LogFile = __socket (AF_UNIX, LogType | SOCK_CLOEXEC, 0);
+	      if (LogFile == -1)
+		return;
+	    }
+	}
+      if (LogFile != -1 && !connected)
+	{
+	  int old_errno = errno;
+	  if (__connect (LogFile, &SyslogAddr, sizeof(SyslogAddr)) == -1)
+	    {
+	      int saved_errno = errno;
+	      int fd = LogFile;
+	      LogFile = -1;
+	      __close (fd);
+	      __set_errno (old_errno);
+	      if (saved_errno == EPROTOTYPE)
 		{
-			int old_errno = errno;
-			if (__connect(LogFile, &SyslogAddr, sizeof(SyslogAddr))
-			    == -1)
-			{
-				int saved_errno = errno;
-				int fd = LogFile;
-				LogFile = -1;
-				(void)__close(fd);
-				__set_errno (old_errno);
-				if (saved_errno == EPROTOTYPE)
-				{
-					/* retry with the other type: */
-					LogType = (LogType == SOCK_DGRAM
-						   ? SOCK_STREAM : SOCK_DGRAM);
-					++retry;
-					continue;
-				}
-			} else
-				connected = true;
+		  /* retry with the other type: */
+		  LogType = LogType == SOCK_DGRAM ? SOCK_STREAM : SOCK_DGRAM;
+		  ++retry;
+		  continue;
 		}
-		break;
+	    }
+	  else
+	    connected = true;
 	}
+      break;
+    }
 }
 
 void
@@ -395,16 +380,16 @@ closelog (void)
 int
 setlogmask (int pmask)
 {
-	int omask;
+  int omask;
 
-	/* Protect against multiple users.  */
-	__libc_lock_lock (syslog_lock);
+  /* Protect against multiple users.  */
+  __libc_lock_lock (syslog_lock);
 
-	omask = LogMask;
-	if (pmask != 0)
-		LogMask = pmask;
+  omask = LogMask;
+  if (pmask != 0)
+    LogMask = pmask;
 
-	__libc_lock_unlock (syslog_lock);
+  __libc_lock_unlock (syslog_lock);
 
-	return (omask);
+  return (omask);
 }
-- 
2.32.0

[PATCH v3 4/7] misc: syslog: Simplify implementation

[Adhemerval Zanella]

Use a temporary buffer for strftime instead of using internal libio
members, simplify fprintf call on the memstream and memory allocation,
use dprintf instead of writev for LOG_PERROR.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 misc/syslog.c | 96 ++++++++++++++++-----------------------------------
 1 file changed, 29 insertions(+), 67 deletions(-)

diff --git a/misc/syslog.c b/misc/syslog.c
index 0736459e7b..e8b1dfe9b8 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -123,13 +123,10 @@ void
 __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		   unsigned int mode_flags)
 {
-  struct tm now_tm;
-  time_t now;
-  int fd;
   FILE *f;
   char *buf = 0;
   size_t bufsize = 0;
-  size_t msgoff;
+  int msgoff;
   int saved_errno = errno;
   char failbuf[3 * sizeof (pid_t) + sizeof "out of memory []"];
 
@@ -143,9 +140,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
 
   /* Prepare for multiple users.  We have to take care: most syscalls we are
      using are cancellation points.  */
-  struct cleanup_arg clarg;
-  clarg.buf = NULL;
-  clarg.oldaction = NULL;
+  struct cleanup_arg clarg = { NULL, NULL };
   __libc_cleanup_push (cancel_handler, &clarg);
   __libc_lock_lock (syslog_lock);
 
@@ -159,51 +154,24 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
 
   /* Build the message in a memory-buffer stream.  */
   f = __open_memstream (&buf, &bufsize);
-  if (f == NULL)
-    {
-      /* We cannot get a stream.  There is not much we can do but emitting an
-	 error messages.  */
-      char numbuf[3 * sizeof (pid_t)];
-      char *nump;
-      char *endp = __stpcpy (failbuf, "out of memory [");
-      pid_t pid = __getpid ();
-
-      nump = numbuf + sizeof (numbuf);
-      /* The PID can never be zero.  */
-      do
-	*--nump = '0' + pid % 10;
-      while ((pid /= 10) != 0);
-
-      endp = __mempcpy (endp, nump, (numbuf + sizeof (numbuf)) - nump);
-      *endp++ = ']';
-      *endp = '\0';
-      buf = failbuf;
-      bufsize = endp - failbuf;
-      msgoff = 0;
-    }
-  else
+  if (f != NULL)
     {
       __fsetlocking (f, FSETLOCKING_BYCALLER);
-      fprintf (f, "<%d>", pri);
-      now = time_now ();
-      f->_IO_write_ptr += __strftime_l (f->_IO_write_ptr,
-					f->_IO_write_end - f->_IO_write_ptr,
-					"%h %e %T ",
-					__localtime_r (&now, &now_tm),
-					_nl_C_locobj_ptr);
-      msgoff = ftell (f);
-      if (LogTag == NULL)
-	LogTag = __progname;
-      if (LogTag != NULL)
-	__fputs_unlocked (LogTag, f);
-      if (LogStat & LOG_PID)
-	fprintf (f, "[%d]", (int) __getpid ());
-      if (LogTag != NULL)
-	{
-	  __putc_unlocked (':', f);
-	  __putc_unlocked (' ', f);
-	}
-
+      /* "%h %e %H:%M:%S"  */
+      char timebuf[3+1               /* "%h "  */
+                   + 2+1             /* "%e "  */
+                   + 2+1 + 2+1 + 2+1 /* "%T"  */];
+      time_t now = time_now ();
+      struct tm now_tm;
+      __localtime_r (&now, &now_tm);
+      __strftime_l (timebuf, sizeof (timebuf), "%h %e %T", &now_tm,
+		    _nl_C_locobj_ptr);
+
+      pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
+
+      fprintf (f, "<%d>%s %n%s%s%.0d%s: ", pri, timebuf, &msgoff,
+               LogTag == NULL ? __progname : LogTag,
+               pid != 0 ? "[" : "", pid, pid != 0 ? "]" : "");
       /* Restore errno for %m format.  */
       __set_errno (saved_errno);
 
@@ -217,26 +185,19 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
       /* Tell the cancellation handler to free this buffer.  */
       clarg.buf = buf;
     }
+  else
+    {
+      /* We cannot get a stream.  There is not much we can do but emitting an
+         error messages.  */
+      bufsize = __snprintf (failbuf, sizeof failbuf, "out of memory[%d]",
+                            __getpid ());
+      buf = failbuf;
+    }
 
   /* Output to stderr if requested. */
   if (LogStat & LOG_PERROR)
-    {
-      struct iovec iov[2];
-      struct iovec *v = iov;
-
-      v->iov_base = buf + msgoff;
-      v->iov_len = bufsize - msgoff;
-      /* Append a newline if necessary.  */
-      if (buf[bufsize - 1] != '\n')
-	{
-	  ++v;
-	  v->iov_base = (char *) "\n";
-	  v->iov_len = 1;
-	}
-
-      /* writev is a cancellation point.  */
-      __writev (STDERR_FILENO, iov, v - iov + 1);
-    }
+    __dprintf (STDERR_FILENO, "%s%s", buf + msgoff,
+               buf[bufsize - 1] != '\n' ? "\n" : "");
 
   /* Get connected, output the message to the local logger.  */
   if (!connected)
@@ -265,6 +226,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
 	   * Make sure the error reported is the one from the
 	   * syslogd failure.
 	   */
+	  int fd;
 	  if (LogStat & LOG_CONS &&
 	      (fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
 			    | O_CLOEXEC, 0))
-- 
2.32.0

[PATCH v3 5/7] misc: syslog: Use fixed-sized buffer

[Adhemerval Zanella]

A fixed-sized buffer is used instead of memstream for messages up to
1024 bytes to avoid the potential BUFSIZ (8K) malloc and free for
each syslog call.  The memstream is still used as fallback for
larger messages.

Checked on x86_64-linux-gnu.
---
 misc/syslog.c | 102 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 65 insertions(+), 37 deletions(-)

diff --git a/misc/syslog.c b/misc/syslog.c
index e8b1dfe9b8..7852441615 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -123,12 +123,11 @@ void
 __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		   unsigned int mode_flags)
 {
-  FILE *f;
-  char *buf = 0;
+  char *buf = NULL;
   size_t bufsize = 0;
+  bool buf_malloced = false;
   int msgoff;
   int saved_errno = errno;
-  char failbuf[3 * sizeof (pid_t) + sizeof "out of memory []"];
 
 #define	INTERNALLOG LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
   /* Check for invalid bits. */
@@ -152,46 +151,75 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
   if ((pri & LOG_FACMASK) == 0)
     pri |= LogFacility;
 
-  /* Build the message in a memory-buffer stream.  */
-  f = __open_memstream (&buf, &bufsize);
-  if (f != NULL)
+  pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
+
+  enum
+    {
+      timestamp_size = sizeof "MMM DD hh:mm:ss ",
+      bufs_size = 1024
+    };
+
+  /* "%h %e %H:%M:%S "  */
+  char timestamp[timestamp_size];
+  time_t now = time_now ();
+  struct tm now_tm;
+  __localtime_r (&now, &now_tm);
+  __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", &now_tm,
+                _nl_C_locobj_ptr);
+
+#define SYSLOG_HEADER(__pri, __timestamp, __msgoff, pid) \
+  "<%d>%s %n%s%s%.0d%s: ",                               \
+  __pri, __timestamp, __msgoff,                          \
+  LogTag == NULL ? __progname : LogTag,                  \
+  pid != 0 ? "[" : "", pid, pid != 0 ? "]" : ""
+
+  /* Try to use a static buffer as an optimization.  */
+  char bufs[bufs_size];
+  int l = __snprintf (bufs, sizeof bufs,
+                      SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+  if (l < sizeof (bufs))
     {
-      __fsetlocking (f, FSETLOCKING_BYCALLER);
-      /* "%h %e %H:%M:%S"  */
-      char timebuf[3+1               /* "%h "  */
-                   + 2+1             /* "%e "  */
-                   + 2+1 + 2+1 + 2+1 /* "%T"  */];
-      time_t now = time_now ();
-      struct tm now_tm;
-      __localtime_r (&now, &now_tm);
-      __strftime_l (timebuf, sizeof (timebuf), "%h %e %T", &now_tm,
-		    _nl_C_locobj_ptr);
-
-      pid_t pid = LogStat & LOG_PID ? __getpid () : 0;
-
-      fprintf (f, "<%d>%s %n%s%s%.0d%s: ", pri, timebuf, &msgoff,
-               LogTag == NULL ? __progname : LogTag,
-               pid != 0 ? "[" : "", pid, pid != 0 ? "]" : "");
+      va_list apc;
+      va_copy (apc, ap);
+
       /* Restore errno for %m format.  */
       __set_errno (saved_errno);
 
-      /* We have the header.  Print the user's format into the buffer.  */
-      __vfprintf_internal (f, fmt, ap, mode_flags);
+      int vl = __vsnprintf_internal (bufs + l, sizeof bufs - l, fmt, apc,
+                                     mode_flags);
+      if (l + vl < sizeof bufs)
+        {
+          buf = bufs;
+          bufsize = l + vl;
+        }
 
-      /* Close the memory stream; this will finalize the data into a malloc'd
-	 buffer in BUF.  */
-      fclose (f);
-
-      /* Tell the cancellation handler to free this buffer.  */
-      clarg.buf = buf;
+      va_end (apc);
     }
-  else
+
+  /* If the required size is larger than buffer size fallbacks to
+     open_memstream.  */
+  if (buf == NULL)
     {
-      /* We cannot get a stream.  There is not much we can do but emitting an
-         error messages.  */
-      bufsize = __snprintf (failbuf, sizeof failbuf, "out of memory[%d]",
-                            __getpid ());
-      buf = failbuf;
+      FILE *f = __open_memstream (&buf, &bufsize);
+      if (f != NULL)
+        {
+          __fsetlocking (f, FSETLOCKING_BYCALLER);
+          fprintf (f, SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+          /* Restore errno for %m format.  */
+          __set_errno (saved_errno);
+          __vfprintf_internal (f, fmt, ap, mode_flags);
+          fclose (f);
+
+          /* Tell the cancellation handler to free this buffer.  */
+          buf_malloced = true;
+          clarg.buf = buf;
+        }
+      else
+        {
+          bufsize = __snprintf (bufs, sizeof bufs,
+                                "out of memory[%d]", __getpid ());
+          buf = bufs;
+        }
     }
 
   /* Output to stderr if requested. */
@@ -243,7 +271,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
   __libc_cleanup_pop (0);
   __libc_lock_unlock (syslog_lock);
 
-  if (buf != failbuf)
+  if (buf_malloced)
     free (buf);
 }
 
-- 
2.32.0

[PATCH v3 6/7] misc: syslog: Move SYSLOG_NAME to USE_MISC (BZ #16355)

[Adhemerval Zanella]

There is no easy solution as described on first comment in bug report,
and some code (like busybox) assumes facilitynames existance when
SYSLOG_NAMES is defined (so we can't just remove it as suggested in
comment #2).

So use the easier solution and guard it with __USE_MISC.
---
 misc/sys/syslog.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc/sys/syslog.h b/misc/sys/syslog.h
index dc3b0e7ef8..bf368d1b8d 100644
--- a/misc/sys/syslog.h
+++ b/misc/sys/syslog.h
@@ -62,7 +62,7 @@
 #define	LOG_PRI(p)	((p) & LOG_PRIMASK)
 #define	LOG_MAKEPRI(fac, pri)	((fac) | (pri))
 
-#ifdef SYSLOG_NAMES
+#if defined(SYSLOG_NAMES) && defined(__USE_MISC)
 #define	INTERNAL_NOPRI	0x10	/* the "no priority" priority */
 				/* mark "facility" */
 #define	INTERNAL_MARK	LOG_MAKEPRI(LOG_NFACILITIES << 3, 0)
@@ -118,7 +118,7 @@ CODE prioritynames[] =
 				/* facility of pri */
 #define	LOG_FAC(p)	(((p) & LOG_FACMASK) >> 3)
 
-#ifdef SYSLOG_NAMES
+#if defined(SYSLOG_NAMES) && defined(__USE_MISC)
 CODE facilitynames[] =
   {
     { "auth", LOG_AUTH },
-- 
2.32.0

[PATCH v3 7/7] misc: Use gmtime instead of localtime

[Adhemerval Zanella]

We deviate from RFC3164 which states timestamp should be in localtime
because although our __localtime_r does not set tzname, the relay still
might still use localtime (which does) and if the server timezone changes
it might result in wrong timestamp from client.  It still does not help
if a process switches its TZ setting from a timezone that has leap
seconds, to one that doesn't (or vice versa), but this would incur in
other problems.

It also handles the highly unlikely case where gmtime might return NULL,
in this case only the PRI is set to hopefully instruct the relay to
get eh TIMESTAMP (as defined by the RFC).

Finally it also uses internally the 64 bit time_t interfaces (to avoid
y2038 issues on 32 bit legacy architectures).

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 misc/syslog.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/misc/syslog.c b/misc/syslog.c
index 7852441615..997e423228 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -161,11 +161,25 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
 
   /* "%h %e %H:%M:%S "  */
   char timestamp[timestamp_size];
-  time_t now = time_now ();
+  __time64_t now = time64_now ();
+
+  /* We deviate from RFC3164 which states timestamp should be in localtime
+     because although our __localtime_r does not set tzname, the relay still
+     might still use localtime (which does) and if the server timezone changes
+     it might result in wrong timestamp from client.  It still does not help
+     if a process switches its TZ setting from a timezone that has leap
+     seconds, to one that doesn't (or vice versa), but this would incur in
+     other problems.  */
   struct tm now_tm;
-  __localtime_r (&now, &now_tm);
-  __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", &now_tm,
-                _nl_C_locobj_ptr);
+  bool has_ts = __gmtime64_r (&now, &now_tm) != NULL;
+
+  /* In the highly unlike case of gmtime_r failure (the clock being
+     INT_MIN + 1900 or follow INT_MAX + 1900) we skip the hostname so the
+     message is handl as valid PRI but without TIMESTAMP or invalid TIMESTAMP
+     (which should force the relay to add the timestamp itself).  */
+  if (has_ts)
+    __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", &now_tm,
+		  _nl_C_locobj_ptr);
 
 #define SYSLOG_HEADER(__pri, __timestamp, __msgoff, pid) \
   "<%d>%s %n%s%s%.0d%s: ",                               \
@@ -173,10 +187,18 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
   LogTag == NULL ? __progname : LogTag,                  \
   pid != 0 ? "[" : "", pid, pid != 0 ? "]" : ""
 
+#define SYSLOG_HEADER_WITHOUT_TS(__pri, __msgoff)        \
+  "<%d>: %n", __pri, __msgoff
+
   /* Try to use a static buffer as an optimization.  */
   char bufs[bufs_size];
-  int l = __snprintf (bufs, sizeof bufs,
-                      SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+  int l;
+  if (has_ts)
+    l = __snprintf (bufs, sizeof bufs,
+		    SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+  else
+    l = __snprintf (bufs, sizeof bufs,
+		    SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
   if (l < sizeof (bufs))
     {
       va_list apc;
@@ -204,7 +226,10 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
       if (f != NULL)
         {
           __fsetlocking (f, FSETLOCKING_BYCALLER);
-          fprintf (f, SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+	  if (has_ts)
+	    fprintf (f, SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+	  else
+	    fprintf (f, SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
           /* Restore errno for %m format.  */
           __set_errno (saved_errno);
           __vfprintf_internal (f, fmt, ap, mode_flags);
-- 
2.32.0

Re: [PATCH v3 0/7] Refactor syslog implementation

[Paul Eggert]

Thanks for looking into this. I'm reviewing the patches all in one diff 
rather than one patch at a time, as that's more convenient for me:


> -#ifdef SYSLOG_NAMES
> +#if defined(SYSLOG_NAMES) && defined(__USE_MISC)

Need spaces before parens. Better yet, omit the parens. Please do this 
systematically in #if.


> +  enum
>      {
> +      timestamp_size = sizeof "MMM DD hh:mm:ss ",
> +      bufs_size = 1024
> +    };

As these enums are used only once it might be more readable to eliminate 
them and replace their uses with their definiens, e.g.,

    char timestamp[sizeof "MMM DD hh:mm:ss "];
    ...
    char bufs[1024];

since the later code uses "sizeof timestamp" and "sizeof bufs" anyway 
(as that's less error-prone).


> +  /* "%h %e %H:%M:%S "  */

Please prefer "%b" to "%h" here and elsewhere, as they're equivalent and 
"%b" is more mnemonic (it's short for "%B").


> +  /* We deviate from RFC3164 which states timestamp should be in localtime

Please use imperative instead of plural form: "Deviate from" instead of 
"We deviate from". None of the new comments should need to use "we" or 
"us" or "our" or "ours".


> +  bool buf_malloced = false;

This local var isn't needed. You can remove it, and replace its use with 
"buf != bufs", which is like what the old code did; this is a bit more 
efficient, I expect.


> +  bool has_ts = __gmtime64_r (&now, &now_tm) != NULL;

It'll be slightly more efficient to replace this with:

    struct tm *now_tmp = __gmtime64_r (&now, &now_tm);
    bool has_ts = now_tmp != NULL;

and replace the "&now_tm" with "now_tmp" in the next __strftime_l call.


> +  /* In the highly unlike case of gmtime_r failure (the clock being
> +     INT_MIN + 1900 or follow INT_MAX + 1900) we skip the hostname so the
> +     message is handl as valid PRI but without TIMESTAMP or invalid TIMESTAMP
> +     (which should force the relay to add the timestamp itself).  */

Some English fixups. "unlike" -> "unlikely". No need for "highly". "the 
clock being INT_MIN + 1900 or follow INT_MAX + 1900" -> "tm_year out of 
int range". "we skip" -> "skip". "handl" -> "handled".

I don't understand the bit about "without TIMESTAMP or invalid TIMESTAMP
(which should force the relay to add the timestamp itself)". Since we're 
already departing from RFC 3164, aren't we already generating an invalid 
TIMESTAMP? And if so, why can't we output our own representation of the 
out-of-range timestamp, e.g., '@67768037170140800' to represent a 
timestamp that is 67768037170140800 seconds after the Epoch?

Better yet, we could output the correct year by dividing the __time64_t 
value by 12622780800 (60 * 60 * 24 * the number of days in 400 Gregorian 
years), running __gmtime64_r on the remainder, and adding 400 times the 
quotient to the tm_year that __gmtime64_r gives us; this computation 
will always succeed and so we won't need to worry about __gmtime64_r 
failure. On platforms with leap seconds this approach would go very 
slightly wrong on timestamps millions of years in the future but those 
timestamps are wrong anyway (due to leap seconds we don't know about 
yet, plus we'll switch to some approach other than leap seconds by then 
anyway).


> +  pid != 0 ? "[" : "", pid, pid != 0 ? "]" : ""

Is GCC smart enough to optimize this to be branch-free? If not, you can 
hand-optimize it as follows:

    "[" + (pid == 0), pid, "]" + (pid == 0)

> +               buf[bufsize - 1] != '\n' ? "\n" : "");

Similarly, this can be "\n" + (buf[bufsize - 1] == '\n').


> +  if (l < sizeof (bufs))

Omit the unnecessary parentheses (for consistency with the other code).
Also, this comparison isn't safe on admittedly-theoretical platforms 
where size_t is narrower than int. So I suggest:

    if (0 <= l && l < sizeof bufs)

which is clearer and should be equally efficient.

+      if (l + vl < sizeof bufs)

l + vl could have signed integer overflow, leading to undefined 
behavior. Also, this doesn't work if vl == -1. Also, we have the same 
theoretical problem as before. So change this to "if (0 <= vl && vl < 
sizeof bufs - l)".


> +      FILE *f = __open_memstream (&buf, &bufsize);
> +      if (f != NULL)

I'm not seeing what the memstream buys you here, compared to a simple 
malloc. You can't generate anything longer than INT_MAX bytes, since 
fprintf won't let you. And you already know how many bytes to allocate, 
from the returned value of the call to snprintf on the too-small stack 
buffer. So just call malloc and then call snprintf again; there's no 
need for a memstream. (The existing code already has this problem of 
course.)

Re: [PATCH v3 7/7] misc: Use gmtime instead of localtime

[Andreas Schwab]

On Mär 18 2022, Adhemerval Zanella via Libc-alpha wrote:

> diff --git a/misc/syslog.c b/misc/syslog.c
> index 7852441615..997e423228 100644
> --- a/misc/syslog.c
> +++ b/misc/syslog.c
> @@ -161,11 +161,25 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,
>  
>    /* "%h %e %H:%M:%S "  */
>    char timestamp[timestamp_size];
> -  time_t now = time_now ();
> +  __time64_t now = time64_now ();
> +
> +  /* We deviate from RFC3164 which states timestamp should be in localtime
> +     because although our __localtime_r does not set tzname, the relay still
> +     might still use localtime (which does) and if the server timezone changes

s/still...still/still/

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v3 0/7] Refactor syslog implementation

[Adhemerval Zanella]

On 18/03/2022 18:11, Paul Eggert wrote:
> Thanks for looking into this. I'm reviewing the patches all in one diff rather than one patch at a time, as that's more convenient for me:
> 
> 
>> -#ifdef SYSLOG_NAMES
>> +#if defined(SYSLOG_NAMES) && defined(__USE_MISC)
> 
> Need spaces before parens. Better yet, omit the parens. Please do this systematically in #if.
> 

Ack.

> 
>> +  enum
>>      {
>> +      timestamp_size = sizeof "MMM DD hh:mm:ss ",
>> +      bufs_size = 1024
>> +    };
> 
> As these enums are used only once it might be more readable to eliminate them and replace their uses with their definiens, e.g.,
> 
>    char timestamp[sizeof "MMM DD hh:mm:ss "];
>    ...
>    char bufs[1024];
> 
> since the later code uses "sizeof timestamp" and "sizeof bufs" anyway (as that's less error-prone).

Yeah, it seems it would be better in this case.

> 
> 
>> +  /* "%h %e %H:%M:%S "  */
> 
> Please prefer "%b" to "%h" here and elsewhere, as they're equivalent and "%b" is more mnemonic (it's short for "%B").
> 

Ack, although in this case I am keeping the old code as-in.

> 
>> +  /* We deviate from RFC3164 which states timestamp should be in localtime
> 
> Please use imperative instead of plural form: "Deviate from" instead of "We deviate from". None of the new comments should need to use "we" or "us" or "our" or "ours".
> 

Ack.

> 
>> +  bool buf_malloced = false;
> 
> This local var isn't needed. You can remove it, and replace its use with "buf != bufs", which is like what the old code did; this is a bit more efficient, I expect.
> 

Ack.

> 
>> +  bool has_ts = __gmtime64_r (&now, &now_tm) != NULL;
> 
> It'll be slightly more efficient to replace this with:
> 
>    struct tm *now_tmp = __gmtime64_r (&now, &now_tm);
>    bool has_ts = now_tmp != NULL;
> 
> and replace the "&now_tm" with "now_tmp" in the next __strftime_l call.
> 

Ack.

> 
>> +  /* In the highly unlike case of gmtime_r failure (the clock being
>> +     INT_MIN + 1900 or follow INT_MAX + 1900) we skip the hostname so the
>> +     message is handl as valid PRI but without TIMESTAMP or invalid TIMESTAMP
>> +     (which should force the relay to add the timestamp itself).  */
> 
> Some English fixups. "unlike" -> "unlikely". No need for "highly". "the clock being INT_MIN + 1900 or follow INT_MAX + 1900" -> "tm_year out of int range". "we skip" -> "skip". "handl" -> "handled".

Ack.

> 
> I don't understand the bit about "without TIMESTAMP or invalid TIMESTAMP
> (which should force the relay to add the timestamp itself)". Since we're already departing from RFC 3164, aren't we already generating an invalid TIMESTAMP? And if so, why can't we output our own representation of the out-of-range timestamp, e.g., '@67768037170140800' to represent a timestamp that is 67768037170140800 seconds after the Epoch?

I meant the RFC3164 '4.3.2 Valid PRI but no TIMESTAMP or invalid TIMESTAMP',
which states in such case the relay should be responsible to generate the
timestamp itself.  And I think that if the clock is in a such bogus state,
I don't see a gain in generating timestamp.

> 
> Better yet, we could output the correct year by dividing the __time64_t value by 12622780800 (60 * 60 * 24 * the number of days in 400 Gregorian years), running __gmtime64_r on the remainder, and adding 400 times the quotient to the tm_year that __gmtime64_r gives us; this computation will always succeed and so we won't need to worry about __gmtime64_r failure. On platforms with leap seconds this approach would go very slightly wrong on timestamps millions of years in the future but those timestamps are wrong anyway (due to leap seconds we don't know about yet, plus we'll switch to some approach other than leap seconds by then anyway).

I really don't think we should bother for such corner cases, specially since
from systemd discussion [1] usually the local relay will use different timestamps
mechanisms than the one generated by the client (such as socket timetamp or
better resolutions obtained by server itself).

[1] https://github.com/systemd/systemd/issues/19251

> 
> 
>> +  pid != 0 ? "[" : "", pid, pid != 0 ? "]" : ""
> 
> Is GCC smart enough to optimize this to be branch-free? If not, you can hand-optimize it as follows:
> 
>    "[" + (pid == 0), pid, "]" + (pid == 0)

I would guess so, but I did not bother to check.  Your suggestion is slight
simpler though.

> 
>> +               buf[bufsize - 1] != '\n' ? "\n" : "");
> 
> Similarly, this can be "\n" + (buf[bufsize - 1] == '\n').
> 
> 

Ack.

>> +  if (l < sizeof (bufs))
> 
> Omit the unnecessary parentheses (for consistency with the other code).
> Also, this comparison isn't safe on admittedly-theoretical platforms where size_t is narrower than int. So I suggest:
> 
>    if (0 <= l && l < sizeof bufs)
> 
> which is clearer and should be equally efficient.

Ack.

> 
> +      if (l + vl < sizeof bufs)
> 
> l + vl could have signed integer overflow, leading to undefined behavior. Also, this doesn't work if vl == -1. Also, we have the same theoretical problem as before. So change this to "if (0 <= vl && vl < sizeof bufs - l)".

Ack.

> 
> 
>> +      FILE *f = __open_memstream (&buf, &bufsize);
>> +      if (f != NULL)
> 
> I'm not seeing what the memstream buys you here, compared to a simple malloc. You can't generate anything longer than INT_MAX bytes, since fprintf won't let you. And you already know how many bytes to allocate, from the returned value of the call to snprintf on the too-small stack buffer. So just call malloc and then call snprintf again; there's no need for a memstream. (The existing code already has this problem of course.)

Yeah, I tried to keep this part as is, but it seems better indeed.