[PATCH 00/11] Build getdomainname, gethostname, syslog with fortification

[PATCH 00/11] Build getdomainname, gethostname, syslog with fortification

[Florian Weimer]

Fortification was disabled to address build failures.  With
fortification enabled, the installed header contains an inline function
with a body, and we cannot define the function again, with its real
implementation.

The internal __snprintf function did not have an fortify wrapper, so we
lost a couple of fortification opportunities.

After these changes, use of %n in syslog becomes a potential performance
bottleneck.  The later commits extract useful functionality from
asprintf and use that to construct the syslog packet contents.

Thanks,
Florian

Florian Weimer (11):
  misc: Build getdomainname with fortification
  misc: Build gethostname with fortification
  libio: Add fortify wrapper for internal __snprintf
  syslog: Update misc/tst-syslog to check reported %n value
  syslog: Build with fortification
  stdio: Rename __printf_buffer to __vfprintf_buffer
  libio: Extract __printf_buffer_asprintf_init from asprintf
    implementation
  stdio-common: Introduce the __printf_buffer function
  stdio-common: Allow skipping initial bytes in __printf_buffer for %n
  stdio-common: Support large offsets with %lln
  syslog: Use a printf buffer directly to construct the entire packet

 debug/snprintf_chk.c                        |   1 +
 include/bits/stdio2.h                       |   9 +
 include/printf.h                            |   9 +-
 include/printf_buffer.h                     |  52 +++++-
 include/stdio.h                             |   1 +
 libio/iovdprintf.c                          |   2 +-
 libio/iovsprintf.c                          |   2 +-
 libio/obprintf.c                            |   2 +-
 libio/vasprintf.c                           |  42 ++---
 libio/vsnprintf.c                           |   2 +-
 libio/vswprintf.c                           |   2 +-
 misc/Makefile                               |   7 -
 misc/bits/syslog.h                          |   4 +
 misc/getdomain.c                            |   2 +
 misc/gethostname.c                          |   2 +
 misc/syslog.c                               | 187 +++++++-------------
 misc/tst-syslog.c                           |  12 +-
 posix/bits/unistd.h                         |   6 +-
 stdio-common/Makefile                       |   6 +
 stdio-common/printf_buffer.c                |  30 ++++
 stdio-common/tst-printf-large-n.c           |  81 +++++++++
 stdio-common/tst-printf_buffer.c            | 156 ++++++++++++++++
 stdio-common/vfprintf-internal.c            |  10 +-
 stdio-common/vfprintf-process-arg.c         |   2 +-
 sysdeps/mach/hurd/getdomain.c               |   2 +
 sysdeps/mach/hurd/gethostname.c             |   2 +
 sysdeps/posix/gethostname.c                 |   2 +
 sysdeps/unix/sysv/linux/alpha/gethostname.c |   2 +
 28 files changed, 448 insertions(+), 189 deletions(-)
 create mode 100644 stdio-common/printf_buffer.c
 create mode 100644 stdio-common/tst-printf-large-n.c
 create mode 100644 stdio-common/tst-printf_buffer.c


base-commit: 15de3d17e1d9da5d38efb0a87a82efbc5bda732d
-- 
2.43.0

[PATCH 01/11] misc: Build getdomainname with fortification

[Florian Weimer]

Introduce __glibc_nofortify_getdomainname to request disabling
the fortification wrapper.
---
 misc/Makefile                 | 1 -
 misc/getdomain.c              | 2 ++
 posix/bits/unistd.h           | 3 ++-
 sysdeps/mach/hurd/getdomain.c | 2 ++
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/misc/Makefile b/misc/Makefile
index c273ec6974..44ae89670a 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -209,7 +209,6 @@ routines := \
 
 # Exclude fortified routines from being built with _FORTIFY_SOURCE
 routines_no_fortify += \
-  getdomain \
   gethostname \
   syslog \
   # routines_no_fortify
diff --git a/misc/getdomain.c b/misc/getdomain.c
index f6325644c9..f96636821f 100644
--- a/misc/getdomain.c
+++ b/misc/getdomain.c
@@ -19,6 +19,8 @@
    The result is null-terminated if LEN is large enough for the full
    name and the terminator.  */
 
+#define __glibc_nofortify_getdomainname
+
 #include <errno.h>
 #include <unistd.h>
 #include <sys/param.h>
diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
index bd209ec28e..6ed2943bf1 100644
--- a/posix/bits/unistd.h
+++ b/posix/bits/unistd.h
@@ -149,7 +149,8 @@ __NTH (gethostname (char *__buf, size_t __buflen))
 #endif
 
 
-#if defined __USE_MISC || (defined __USE_XOPEN && !defined __USE_UNIX98)
+#if (defined __USE_MISC || (defined __USE_XOPEN && !defined __USE_UNIX98)) \
+  && !defined __glibc_nofortify_getdomainname
 __fortify_function int
 __NTH (getdomainname (char *__buf, size_t __buflen))
 {
diff --git a/sysdeps/mach/hurd/getdomain.c b/sysdeps/mach/hurd/getdomain.c
index 137ce9ad5b..e24ab2137e 100644
--- a/sysdeps/mach/hurd/getdomain.c
+++ b/sysdeps/mach/hurd/getdomain.c
@@ -15,6 +15,8 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#define __glibc_nofortify_getdomainname
+
 #include <unistd.h>
 #include "hurdhost.h"
 
-- 
2.43.0

[PATCH 02/11] misc: Build gethostname with fortification

[Florian Weimer]

Introduce __glibc_nofortify_gethostname to request disabling
the fortification wrapper.
---
 misc/Makefile                               | 1 -
 misc/gethostname.c                          | 2 ++
 posix/bits/unistd.h                         | 3 ++-
 sysdeps/mach/hurd/gethostname.c             | 2 ++
 sysdeps/posix/gethostname.c                 | 2 ++
 sysdeps/unix/sysv/linux/alpha/gethostname.c | 2 ++
 6 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/misc/Makefile b/misc/Makefile
index 44ae89670a..6d8528c925 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -209,7 +209,6 @@ routines := \
 
 # Exclude fortified routines from being built with _FORTIFY_SOURCE
 routines_no_fortify += \
-  gethostname \
   syslog \
   # routines_no_fortify
 
diff --git a/misc/gethostname.c b/misc/gethostname.c
index 2c849a95c3..82800f8abb 100644
--- a/misc/gethostname.c
+++ b/misc/gethostname.c
@@ -15,6 +15,8 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#define __glibc_nofortify_gethostname
+
 #include <errno.h>
 #include <unistd.h>
 
diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
index 6ed2943bf1..1230bba9a5 100644
--- a/posix/bits/unistd.h
+++ b/posix/bits/unistd.h
@@ -138,7 +138,8 @@ getlogin_r (char *__buf, size_t __buflen)
 #endif
 
 
-#if defined __USE_MISC || defined __USE_UNIX98
+#if defined __USE_MISC || defined __USE_UNIX98 \
+  && !defined __glibc_nofortify_gethostname
 __fortify_function int
 __NTH (gethostname (char *__buf, size_t __buflen))
 {
diff --git a/sysdeps/mach/hurd/gethostname.c b/sysdeps/mach/hurd/gethostname.c
index 91d7f45d6e..dbb7d75713 100644
--- a/sysdeps/mach/hurd/gethostname.c
+++ b/sysdeps/mach/hurd/gethostname.c
@@ -15,6 +15,8 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#define __glibc_nofortify_gethostname
+
 #include <unistd.h>
 #include "hurdhost.h"
 
diff --git a/sysdeps/posix/gethostname.c b/sysdeps/posix/gethostname.c
index 3c50706b58..3c79f77dd3 100644
--- a/sysdeps/posix/gethostname.c
+++ b/sysdeps/posix/gethostname.c
@@ -15,6 +15,8 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#define __glibc_nofortify_gethostname
+
 #include <errno.h>
 #include <string.h>
 #include <unistd.h>
diff --git a/sysdeps/unix/sysv/linux/alpha/gethostname.c b/sysdeps/unix/sysv/linux/alpha/gethostname.c
index 30da8f30e4..45eb1788d4 100644
--- a/sysdeps/unix/sysv/linux/alpha/gethostname.c
+++ b/sysdeps/unix/sysv/linux/alpha/gethostname.c
@@ -15,6 +15,8 @@
    License along with the GNU C Library.  If not, see
    <https://www.gnu.org/licenses/>.  */
 
+#define __glibc_nofortify_gethostname
+
 #include <errno.h>
 #include <string.h>
 #include <unistd.h>
-- 
2.43.0

[PATCH 03/11] libio: Add fortify wrapper for internal __snprintf

[Florian Weimer]

---
 debug/snprintf_chk.c  | 1 +
 include/bits/stdio2.h | 9 +++++++++
 include/stdio.h       | 1 +
 3 files changed, 11 insertions(+)

diff --git a/debug/snprintf_chk.c b/debug/snprintf_chk.c
index 995d805f5d..ea02efec3e 100644
--- a/debug/snprintf_chk.c
+++ b/debug/snprintf_chk.c
@@ -40,4 +40,5 @@ ___snprintf_chk (char *s, size_t maxlen, int flag, size_t slen,
 
   return ret;
 }
+ldbl_hidden_def (___snprintf_chk, __snprintf_chk)
 ldbl_strong_alias (___snprintf_chk, __snprintf_chk)
diff --git a/include/bits/stdio2.h b/include/bits/stdio2.h
index 6d7cf4306f..fb909c21ee 100644
--- a/include/bits/stdio2.h
+++ b/include/bits/stdio2.h
@@ -1 +1,10 @@
 #include <libio/bits/stdio2.h>
+
+__fortify_function int
+__NTH (__snprintf (char *__restrict __s, size_t __n,
+                   const char *__restrict __fmt, ...))
+{
+  return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
+				   __glibc_objsize (__s), __fmt,
+				   __va_arg_pack ());
+}
diff --git a/include/stdio.h b/include/stdio.h
index 24f1652f19..364f4d22a1 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -62,6 +62,7 @@ extern int __vsscanf (const char *__restrict __s,
 extern int __sprintf_chk (char *, int, size_t, const char *, ...) __THROW;
 extern int __snprintf_chk (char *, size_t, int, size_t, const char *, ...)
      __THROW;
+stdio_hidden_ldbl_proto (__, snprintf_chk)
 extern int __vsprintf_chk (char *, int, size_t, const char *,
 			   __gnuc_va_list) __THROW;
 extern int __vsnprintf_chk (char *, size_t, int, size_t, const char *,
-- 
2.43.0

[PATCH 04/11] syslog: Update misc/tst-syslog to check reported %n value

[Florian Weimer]

---
 misc/tst-syslog.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/misc/tst-syslog.c b/misc/tst-syslog.c
index ced9ae8467..741693a7ff 100644
--- a/misc/tst-syslog.c
+++ b/misc/tst-syslog.c
@@ -102,8 +102,10 @@ send_vsyslog (int options)
         {
           int facility = facilities[i];
           int priority = priorities[j];
-          call_vsyslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE,
-                        facility, priority);
+	  int pos = -1;
+          call_vsyslog (facility | priority, "%n%s %d %d",
+			&pos, SYSLOG_MSG_BASE, facility, priority);
+	  TEST_COMPARE (pos, 0);
         }
     }
 }
@@ -117,8 +119,10 @@ send_syslog (int options)
         {
           int facility = facilities[i];
           int priority = priorities[j];
-          syslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE, facility,
-                  priority);
+	  int pos = -1;
+          syslog (facility | priority, "%n%s %d %d",
+		  &pos, SYSLOG_MSG_BASE, facility, priority);
+	  TEST_COMPARE (pos, 0);
         }
     }
 }
-- 
2.43.0

[PATCH 05/11] syslog: Build with fortification

[Florian Weimer]

This causes /proc/self/maps to be processed for certain syslog calls
due to the use of %n.
---
 misc/Makefile      | 5 -----
 misc/bits/syslog.h | 4 ++++
 misc/syslog.c      | 3 +++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/misc/Makefile b/misc/Makefile
index 6d8528c925..236076a9d6 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -207,11 +207,6 @@ routines := \
   writev \
   # routines
 
-# Exclude fortified routines from being built with _FORTIFY_SOURCE
-routines_no_fortify += \
-  syslog \
-  # routines_no_fortify
-
 generated += \
   tst-allocate_once-mem.out \
   tst-allocate_once.mtrace \
diff --git a/misc/bits/syslog.h b/misc/bits/syslog.h
index aadcd42000..e6f7938a3f 100644
--- a/misc/bits/syslog.h
+++ b/misc/bits/syslog.h
@@ -31,11 +31,13 @@
    redirections, e.g. long double asm redirections.  */
 
 #ifdef __va_arg_pack
+# ifndef __glibc_nofortify_syslog
 __fortify_function void
 syslog (int __pri, const char *__fmt, ...)
 {
   __syslog_chk (__pri, __USE_FORTIFY_LEVEL - 1, __fmt, __va_arg_pack ());
 }
+# endif
 #elif !defined __cplusplus
 # define syslog(pri, ...) \
   __syslog_chk (pri, __USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
@@ -43,9 +45,11 @@ syslog (int __pri, const char *__fmt, ...)
 
 
 #ifdef __USE_MISC
+# ifndef __glibc_nofortify_vsyslog
 __fortify_function void
 vsyslog (int __pri, const char *__fmt, __gnuc_va_list __ap)
 {
   __vsyslog_chk (__pri,  __USE_FORTIFY_LEVEL - 1, __fmt, __ap);
 }
+# endif
 #endif
diff --git a/misc/syslog.c b/misc/syslog.c
index 4af87f54fd..68ee3aef5f 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -27,6 +27,9 @@
  * SUCH DAMAGE.
  */
 
+#define __glibc_nofortify_syslog
+#define __glibc_nofortify_vsyslog
+
 #if defined(LIBC_SCCS) && !defined(lint)
 static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
 #endif /* LIBC_SCCS and not lint */
-- 
2.43.0

[PATCH 06/11] stdio: Rename __printf_buffer to __vfprintf_buffer

[Florian Weimer]

And __wprintf_buffer to __vwprintf_buffer.  These functions take
a va_list argument, so they are closer to the v-variant functions
in <stdio.h>.

Also move the declaration to <printf_buffer.h>, which is arguably
a better fit than <printf.h> (which contains lots of printf
internals).
---
 include/printf.h                 |  9 +--------
 include/printf_buffer.h          |  9 +++++++++
 libio/iovdprintf.c               |  2 +-
 libio/iovsprintf.c               |  2 +-
 libio/obprintf.c                 |  2 +-
 libio/vasprintf.c                |  2 +-
 libio/vsnprintf.c                |  2 +-
 libio/vswprintf.c                |  2 +-
 stdio-common/vfprintf-internal.c | 10 ++++++----
 9 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/include/printf.h b/include/printf.h
index 2c998059d4..78aac4dcad 100644
--- a/include/printf.h
+++ b/include/printf.h
@@ -65,18 +65,11 @@ int __translated_number_width (locale_t loc,
 			       const char *first, const char *last)
   attribute_hidden;
 
-
-struct __printf_buffer;
-void __printf_buffer (struct __printf_buffer *buf, const char *format,
-		      va_list ap, unsigned int mode_flags);
-struct __wprintf_buffer;
-void __wprintf_buffer (struct __wprintf_buffer *buf, const wchar_t *format,
-		       va_list ap, unsigned int mode_flags);
-
 extern int __printf_fp (FILE *, const struct printf_info *,
 			const void *const *);
 libc_hidden_proto (__printf_fp)
 
+struct __printf_buffer;
 void __printf_fphex_l_buffer (struct __printf_buffer *, locale_t,
 			      const struct printf_info *,
 			      const void *const *) attribute_hidden;
diff --git a/include/printf_buffer.h b/include/printf_buffer.h
index eff8e3413a..fb0b42178e 100644
--- a/include/printf_buffer.h
+++ b/include/printf_buffer.h
@@ -200,6 +200,12 @@ int __printf_buffer_done (struct __printf_buffer *buf) attribute_hidden;
    known, and the flush implementation can be called directly.  */
 bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden;
 
+/* Building block for printf-style functions.  Write argument list AP
+   to BUF according to FORMAT.  MODE_FLAGS contains PRINTF_* flags
+   defined in <libioP.h>.  */
+void __vprintf_buffer (struct __printf_buffer *buf, const char *format,
+                       va_list ap, unsigned int mode_flags) attribute_hidden;
+
 /* Wide version of struct __printf_buffer follows.  */
 
 enum __wprintf_buffer_mode
@@ -274,6 +280,9 @@ int __wprintf_buffer_done (struct __wprintf_buffer *buf) attribute_hidden;
 
 bool __wprintf_buffer_flush (struct __wprintf_buffer *buf) attribute_hidden;
 
+void __vwprintf_buffer (struct __wprintf_buffer *buf, const wchar_t *format,
+                        va_list ap, unsigned int mode_flags) attribute_hidden;
+
 /* Type-generic convenience macros.  They are useful if
    printf_buffer-char.h or printf_buffer-wchar_t.h is included as
    well.  */
diff --git a/libio/iovdprintf.c b/libio/iovdprintf.c
index 6894fa8347..47d9d0ba8f 100644
--- a/libio/iovdprintf.c
+++ b/libio/iovdprintf.c
@@ -66,7 +66,7 @@ __vdprintf_internal (int d, const char *format, va_list arg,
   __printf_buffer_init (&buf.base, buf.buf, array_length (buf.buf),
 			__printf_buffer_mode_dprintf);
   buf.fd = d;
-  __printf_buffer (&buf.base, format, arg, mode_flags);
+  __vprintf_buffer (&buf.base, format, arg, mode_flags);
   if (__printf_buffer_has_failed (&buf.base))
     return -1;
   __printf_buffer_flush_dprintf (&buf);
diff --git a/libio/iovsprintf.c b/libio/iovsprintf.c
index 2d81ee3507..7fdc06dcd4 100644
--- a/libio/iovsprintf.c
+++ b/libio/iovsprintf.c
@@ -59,7 +59,7 @@ __vsprintf_internal (char *string, size_t maxlen,
     __printf_buffer_init_end (&buf, string, (char *) ~(uintptr_t) 0,
 			      __printf_buffer_mode_sprintf);
 
-  __printf_buffer (&buf, format, args, mode_flags);
+  __vprintf_buffer (&buf, format, args, mode_flags);
 
   /* Write the NUL terminator if there is room.  Do not use the putc
      operation to avoid overflowing the character write count.  */
diff --git a/libio/obprintf.c b/libio/obprintf.c
index 491d14fbae..24d1edf260 100644
--- a/libio/obprintf.c
+++ b/libio/obprintf.c
@@ -92,7 +92,7 @@ __obstack_vprintf_internal (struct obstack *obstack, const char *format,
   /* Now allocate the rest of the current chunk.  */
   obstack_blank_fast (obstack, room);
 
-  __printf_buffer (&buf.base, format, args, mode_flags);
+  __vprintf_buffer (&buf.base, format, args, mode_flags);
 
   if (buf.base.write_ptr == &buf.ch + 1)
     /* buf.ch is in use.  Put it into the obstack.  */
diff --git a/libio/vasprintf.c b/libio/vasprintf.c
index 999ae526f4..edcfab2323 100644
--- a/libio/vasprintf.c
+++ b/libio/vasprintf.c
@@ -99,7 +99,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
   __printf_buffer_init (&buf.base, buf.direct, array_length (buf.direct),
 			__printf_buffer_mode_asprintf);
 
-  __printf_buffer (&buf.base, format, args, mode_flags);
+  __vprintf_buffer (&buf.base, format, args, mode_flags);
   int done = __printf_buffer_done (&buf.base);
   if (done < 0)
     {
diff --git a/libio/vsnprintf.c b/libio/vsnprintf.c
index dd7a585cbe..7077fcd9f2 100644
--- a/libio/vsnprintf.c
+++ b/libio/vsnprintf.c
@@ -93,7 +93,7 @@ __vsnprintf_internal (char *string, size_t maxlen, const char *format,
 {
   struct __printf_buffer_snprintf buf;
   __printf_buffer_snprintf_init (&buf, string, maxlen);
-  __printf_buffer (&buf.base, format, args, mode_flags);
+  __vprintf_buffer (&buf.base, format, args, mode_flags);
   return __printf_buffer_snprintf_done (&buf);
 }
 
diff --git a/libio/vswprintf.c b/libio/vswprintf.c
index 5990bd2eff..d73246ba41 100644
--- a/libio/vswprintf.c
+++ b/libio/vswprintf.c
@@ -41,7 +41,7 @@ __vswprintf_internal (wchar_t *string, size_t maxlen, const wchar_t *format,
   struct __wprintf_buffer buf;
   __wprintf_buffer_init (&buf, string, maxlen, __wprintf_buffer_mode_swprintf);
 
-  __wprintf_buffer (&buf, format, args, mode_flags);
+  __vwprintf_buffer (&buf, format, args, mode_flags);
 
   if (buf.write_ptr == buf.write_end)
     {
diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index 771beca9bf..23754232f2 100644
--- a/stdio-common/vfprintf-internal.c
+++ b/stdio-common/vfprintf-internal.c
@@ -122,6 +122,7 @@
 
 #ifndef COMPILE_WPRINTF
 # include "printf_buffer-char.h"
+# define VFPRINTF_BUFFER __vprintf_buffer
 # define vfprintf	__vfprintf_internal
 # define OTHER_CHAR_T   wchar_t
 # define UCHAR_T	unsigned char
@@ -136,6 +137,7 @@ typedef const char *THOUSANDS_SEP_T;
 # define CONVERT_FROM_OTHER_STRING __wcsrtombs
 #else
 # include "printf_buffer-wchar_t.h"
+# define VFPRINTF_BUFFER __vwprintf_buffer
 # define vfprintf	__vfwprintf_internal
 # define OTHER_CHAR_T   char
 /* This is a hack!!!  There should be a type uwchar_t.  */
@@ -595,8 +597,8 @@ static void group_number (struct Xprintf_buffer *buf,
 
 /* The buffer-based function itself.  */
 void
-Xprintf_buffer (struct Xprintf_buffer *buf, const CHAR_T *format,
-		  va_list ap, unsigned int mode_flags)
+VFPRINTF_BUFFER (struct Xprintf_buffer *buf, const CHAR_T *format,
+		 va_list ap, unsigned int mode_flags)
 {
   /* The character used as thousands separator.  */
   THOUSANDS_SEP_T thousands_sep = 0;
@@ -1541,7 +1543,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
     {
       struct Xprintf (buffer_to_file) wrap;
       Xprintf (buffer_to_file_init) (&wrap, s);
-      Xprintf_buffer (&wrap.base, format, ap, mode_flags);
+      VFPRINTF_BUFFER (&wrap.base, format, ap, mode_flags);
       return Xprintf (buffer_to_file_done) (&wrap);
     }
 
@@ -1556,7 +1558,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
   Xprintf (buffer_to_file_init) (&wrap, s);
 
   /* Perform the printing operation on the buffer.  */
-  Xprintf_buffer (&wrap.base, format, ap, mode_flags);
+  VFPRINTF_BUFFER (&wrap.base, format, ap, mode_flags);
   done = Xprintf (buffer_to_file_done) (&wrap);
 
   /* Unlock the stream.  */
-- 
2.43.0

[PATCH 07/11] libio: Extract __printf_buffer_asprintf_init from asprintf implementation

[Florian Weimer]

And __printf_buffer_asprintf_free as well.  This will allow to reuse
the asprintf implementation for more general buffer handling, similar
to open_memstream, but without the mandatory heap allocation.
---
 include/printf_buffer.h | 30 +++++++++++++++++++++++++-----
 libio/vasprintf.c       | 40 +++++++++++++++++-----------------------
 2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/include/printf_buffer.h b/include/printf_buffer.h
index fb0b42178e..cee5afb581 100644
--- a/include/printf_buffer.h
+++ b/include/printf_buffer.h
@@ -317,6 +317,31 @@ void __printf_buffer_snprintf_init (struct __printf_buffer_snprintf *,
 int __printf_buffer_snprintf_done (struct __printf_buffer_snprintf *)
   attribute_hidden;
 
+/* Size of the initial on-stack buffer for asprintf.  It should be
+   large enough to copy almost all asprintf usages with just a single
+   (final, correctly sized) heap allocation.  */
+#define PRINTF_BUFFER_SIZE_ASPRINTF 200
+
+struct __printf_buffer_asprintf
+{
+  /* base.write_base points either to a heap-allocated buffer, or to
+     the direct array below.  */
+  struct __printf_buffer base;
+
+  /* Initial allocation.  */
+  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
+};
+
+/* Sets up BUF for writing via __printf_buffer.  */
+void __printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
+  attribute_hidden;
+
+/* Deallocates any allocated memory in *BUF.  (This is not the usual
+   done routine for asprintf because that has to preserve allocation.)
+   Always returns -1 to indicate an error.  */
+int __printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
+  attribute_hidden;
+
 /* Flush function implementations follow.  They are called from
    __printf_buffer_flush.  Generic code should not call these flush
    functions directly.  Some modes have inline implementations.  */
@@ -363,11 +388,6 @@ void __wprintf_buffer_flush_to_file (struct __wprintf_buffer_to_file *)
 /* Temporary buffer used during floating point digit translation.  */
 #define PRINTF_BUFFER_SIZE_DIGITS 64
 
-/* Size of the initial on-stack buffer for asprintf.  It should be
-   large enough to copy almost all asprintf usages with just a single
-   (final, correctly sized) heap allocation.  */
-#define PRINTF_BUFFER_SIZE_ASPRINTF 200
-
 /* This should cover most of the packet-oriented file descriptors,
    where boundaries between writes could be visible to readers.  But
    it is still small enough not to cause too many stack overflow issues.  */
diff --git a/libio/vasprintf.c b/libio/vasprintf.c
index edcfab2323..f8413eedfe 100644
--- a/libio/vasprintf.c
+++ b/libio/vasprintf.c
@@ -34,18 +34,6 @@
 #include <string.h>
 #include <printf_buffer.h>
 
-struct __printf_buffer_asprintf
-{
-  /* base.write_base points either to a heap-allocated buffer, or to
-     the direct array below.  */
-  struct __printf_buffer base;
-
-  /* Initial allocation.  200 should be large enough to copy almost
-     all asprintf usages with just a single (final, correctly sized)
-     heap allocation.  */
-  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
-};
-
 void
 __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
 {
@@ -90,23 +78,32 @@ __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
   buf->base.write_end = new_buffer + new_size;
 }
 
+void
+__printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
+{
+  __printf_buffer_init (&buf->base, buf->direct, array_length (buf->direct),
+			__printf_buffer_mode_asprintf);
+}
+
+int
+__printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
+{
+  if (buf->base.write_base != buf->direct)
+    free (buf->base.write_base);
+  return -1;
+}
 
 int
 __vasprintf_internal (char **result_ptr, const char *format, va_list args,
 		      unsigned int mode_flags)
 {
   struct __printf_buffer_asprintf buf;
-  __printf_buffer_init (&buf.base, buf.direct, array_length (buf.direct),
-			__printf_buffer_mode_asprintf);
+  __printf_buffer_asprintf_init (&buf);
 
   __vprintf_buffer (&buf.base, format, args, mode_flags);
   int done = __printf_buffer_done (&buf.base);
   if (done < 0)
-    {
-      if (buf.base.write_base != buf.direct)
-	free (buf.base.write_base);
-      return done;
-    }
+    return __printf_buffer_asprintf_free (&buf);
 
   /* Transfer to the final buffer.  */
   char *result;
@@ -122,10 +119,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
     {
       result = realloc (buf.base.write_base, size + 1);
       if (result == NULL)
-	{
-	  free (buf.base.write_base);
-	  return -1;
-	}
+	return __printf_buffer_asprintf_free (&buf);
     }
 
   /* Add NUL termination.  */
-- 
2.43.0

[PATCH 08/11] stdio-common: Introduce the __printf_buffer function

[Florian Weimer]

It can be used to write to a __print_buffer (incrementally).
---
 include/printf_buffer.h          |   5 ++
 stdio-common/Makefile            |   5 ++
 stdio-common/printf_buffer.c     |  30 +++++++
 stdio-common/tst-printf_buffer.c | 138 +++++++++++++++++++++++++++++++
 4 files changed, 178 insertions(+)
 create mode 100644 stdio-common/printf_buffer.c
 create mode 100644 stdio-common/tst-printf_buffer.c

diff --git a/include/printf_buffer.h b/include/printf_buffer.h
index cee5afb581..a5483e8a97 100644
--- a/include/printf_buffer.h
+++ b/include/printf_buffer.h
@@ -206,6 +206,11 @@ bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden;
 void __vprintf_buffer (struct __printf_buffer *buf, const char *format,
                        va_list ap, unsigned int mode_flags) attribute_hidden;
 
+/* Write the argument list to BUF according to FORMAT.
+   Unconditionally enables fortification (special procesing for %n).  */
+void __printf_buffer (struct __printf_buffer *buf, const char *format, ...)
+  __attribute__ ((__format__ (__printf__, 2, 3))) attribute_hidden;
+
 /* Wide version of struct __printf_buffer follows.  */
 
 enum __wprintf_buffer_mode
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 6447b6b444..d610ed67e6 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -64,6 +64,7 @@ routines := \
   perror \
   printf \
   printf-prs \
+  printf_buffer \
   printf_buffer_as_file \
   printf_buffer_done \
   printf_buffer_flush \
@@ -290,6 +291,10 @@ test-srcs = \
   tst-unbputc \
   # test-srcs
 
+# Test for the internal, non-exported __printf_buffer function.
+tests-internal += tst-printf_buffer
+tests-static += tst-printf_buffer
+
 ifeq ($(run-built-tests),yes)
 tests-special += \
   $(objpfx)tst-printf-bz18872-mem.out \
diff --git a/stdio-common/printf_buffer.c b/stdio-common/printf_buffer.c
new file mode 100644
index 0000000000..fabb3b5bb4
--- /dev/null
+++ b/stdio-common/printf_buffer.c
@@ -0,0 +1,30 @@
+/* Write a format string to a buffer.
+   Copyright (C) 2024 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 <printf_buffer.h>
+#include <libioP.h>
+#include <stdarg.h>
+
+void
+__printf_buffer (struct __printf_buffer *buf, const char *format, ...)
+{
+  va_list arg;
+  va_start (arg, format);
+  __vprintf_buffer (buf, format, arg, PRINTF_FORTIFY);
+  va_end (arg);
+}
diff --git a/stdio-common/tst-printf_buffer.c b/stdio-common/tst-printf_buffer.c
new file mode 100644
index 0000000000..d12da8c939
--- /dev/null
+++ b/stdio-common/tst-printf_buffer.c
@@ -0,0 +1,138 @@
+/* Basic test for the __printf_buffer function (via asprintf).
+   Copyright (C) 2024 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 <printf_buffer.h>
+#include <array_length.h>
+#include <string.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "%s", "");
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc");
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 3);
+    TEST_COMPARE_BLOB (buf.direct, 3, "abc", 3);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc[%d]DEF", 17);
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 10);
+    TEST_COMPARE_BLOB (buf.direct, 10, "abc[17]DEF", 10);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  /* Test concatenation.  */
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc");
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 3);
+    TEST_COMPARE_BLOB (buf.direct, 3, "abc", 3);
+    __printf_buffer (&buf.base, "[%d]DEF", 17);
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 10);
+    TEST_COMPARE_BLOB (buf.direct, 10, "abc[17]DEF", 10);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  {
+    struct __printf_buffer_asprintf buf;
+    char large_string[500];
+    memset (large_string, 'X', array_length (large_string));
+    large_string[array_length (large_string) -1] = '\0';
+    int n;
+
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "%s", large_string);
+    TEST_VERIFY (buf.base.write_base != buf.direct);
+    TEST_COMPARE_BLOB (buf.base.write_base,
+                       buf.base.write_ptr - buf.base.write_base,
+                       large_string, strlen (large_string));
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc");
+    __printf_buffer (&buf.base, "%n", &n);
+    /* %n counts from the start of the buffer, not from within the
+        printf call.  */
+    TEST_COMPARE (n, 3);
+    __printf_buffer (&buf.base, "%s", large_string);
+    TEST_VERIFY (buf.base.write_base != buf.direct);
+    TEST_COMPARE_BLOB (buf.base.write_base, 3, "abc", 3);
+    TEST_COMPARE_BLOB (buf.base.write_base + 3,
+                       buf.base.write_ptr - buf.base.write_base - 3,
+                       large_string, strlen (large_string));
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    __printf_buffer (&buf.base, "%n", &n);
+    TEST_COMPARE (n, 3 + strlen (large_string));
+    TEST_COMPARE_BLOB (buf.base.write_base, 3, "abc", 3);
+    TEST_COMPARE_BLOB (buf.base.write_base + 3,
+                       buf.base.write_ptr - buf.base.write_base - 3,
+                       large_string, strlen (large_string));
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
+  /* Test %n with concatenation.  */
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc");
+    int n = -1;
+    __printf_buffer (&buf.base, "D%nEF", &n);
+    TEST_COMPARE (n, 4);        /* %n counts from the start of the buffer.  */
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_COMPARE_BLOB (buf.base.write_base,
+                       buf.base.write_ptr - buf.base.write_base, "abcDEF", 6);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.43.0

[PATCH 09/11] stdio-common: Allow skipping initial bytes in __printf_buffer for %n

[Florian Weimer]

Making the written field signed allows subtraction of a prefix count
in case multiple writes happen to the same buffer.
---
 include/printf_buffer.h          |  8 +++++---
 stdio-common/tst-printf_buffer.c | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/printf_buffer.h b/include/printf_buffer.h
index a5483e8a97..2400371b94 100644
--- a/include/printf_buffer.h
+++ b/include/printf_buffer.h
@@ -92,8 +92,10 @@ struct __printf_buffer
      buffer).  Potentially updated on flush.  The actual number of
      written bytes also includes the unflushed-but-written buffer
      part, write_ptr - write_base.  A 64-bit value is used to avoid
-     the need for overflow checks.  */
-  uint64_t written;
+     the need for intermediate overflow checks.  Negative values can
+     be used to ignore leading parts of the buffer for %n computations
+     (for repeated __printf_buffer calls to the same buffer).  */
+  int64_t written;
 
   /* Identifies the flush callback.  */
   enum __printf_buffer_mode mode;
@@ -225,7 +227,7 @@ struct __wprintf_buffer
   wchar_t *write_base;
   wchar_t *write_ptr;
   wchar_t *write_end;
-  uint64_t written;
+  int64_t written;
   enum __wprintf_buffer_mode mode;
 };
 
diff --git a/stdio-common/tst-printf_buffer.c b/stdio-common/tst-printf_buffer.c
index d12da8c939..2a3ee511d0 100644
--- a/stdio-common/tst-printf_buffer.c
+++ b/stdio-common/tst-printf_buffer.c
@@ -132,6 +132,24 @@ do_test (void)
     TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
     TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
   }
+
+  /* Test %n with initial skip.  */
+  {
+    struct __printf_buffer_asprintf buf;
+    __printf_buffer_asprintf_init (&buf);
+    __printf_buffer (&buf.base, "abc");
+    buf.base.written = -2;
+    int n = -1;
+    __printf_buffer (&buf.base, "D%nEF", &n);
+    TEST_COMPARE (n, 2);        /* Two characters were skipped.  */
+    TEST_VERIFY (buf.base.write_base == buf.direct);
+    TEST_COMPARE_BLOB (buf.base.write_base,
+                       buf.base.write_ptr - buf.base.write_base, "abcDEF", 6);
+    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
+    TEST_COMPARE (__printf_buffer_done (&buf.base), 4); /* Two skipped.  */
+    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
+  }
+
   return 0;
 }
 
-- 
2.43.0

[PATCH 10/11] stdio-common: Support large offsets with %lln

[Florian Weimer]

Use of Xprintf_buffer_done is unnecessary because it performs
overflow detection against int and sets errno.
---
 stdio-common/Makefile               |  1 +
 stdio-common/tst-printf-large-n.c   | 81 +++++++++++++++++++++++++++++
 stdio-common/vfprintf-process-arg.c |  2 +-
 3 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 stdio-common/tst-printf-large-n.c

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index d610ed67e6..482d841f96 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -234,6 +234,7 @@ tests := \
   tst-printf-fp-free \
   tst-printf-fp-leak \
   tst-printf-intn \
+  tst-printf-large-n \
   tst-printf-oct \
   tst-printf-round \
   tst-printfsz \
diff --git a/stdio-common/tst-printf-large-n.c b/stdio-common/tst-printf-large-n.c
new file mode 100644
index 0000000000..4504a693d2
--- /dev/null
+++ b/stdio-common/tst-printf-large-n.c
@@ -0,0 +1,81 @@
+/* Test that %n can report values larger than INT_MAX.
+   Copyright (C) 2024 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 <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <wchar.h>
+
+#include <support/blob_repeat.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+
+static int
+do_test (void)
+{
+  enum { string_length = 1 << 28 };
+
+  FILE *fp = xfopen ("/dev/null", "w");
+
+  /* Byte-oriented streams.  */
+  {
+    struct support_blob_repeat repeat
+      = support_blob_repeat_allocate ("a", 1, string_length + 1);
+    char *string = repeat.start;
+    if (string == NULL)
+      FAIL_UNSUPPORTED ("cannot allocate large string");
+    string[string_length] = '\0';
+
+      long long int n;
+    TEST_COMPARE (fprintf (fp, "%s %s %s %s %s %s %s %s %lln",
+                           string, string, string, string, string, string,
+                           string, string, &n), -1);
+    TEST_COMPARE (errno, EOVERFLOW);
+    TEST_COMPARE (n, 8 * (string_length + 1LL));
+    TEST_VERIFY (n > INT_MAX);
+
+      support_blob_repeat_free (&repeat);
+  }
+
+  /* Wide-oriented streams.  */
+  {
+    struct support_blob_repeat repeat
+      = support_blob_repeat_allocate (L"a", sizeof (wchar_t),
+                                      string_length + 1);
+    wchar_t *string = repeat.start;
+    if (string == NULL)
+      FAIL_UNSUPPORTED ("cannot allocate large wide string");
+    string[string_length] = '\0';
+
+    long long int n;
+    TEST_COMPARE (fwprintf (fp, L"%Ls %Ls %Ls %Ls %Ls %Ls %Ls %Ls %lln",
+                            string, string, string, string, string, string,
+                            string, string, &n), -1);
+    TEST_COMPARE (errno, EOVERFLOW);
+    TEST_COMPARE (n, 8 * (string_length + 1LL));
+    TEST_VERIFY (n > INT_MAX);
+
+      support_blob_repeat_free (&repeat);
+  }
+
+  xfclose (fp);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c
index af6b570ea9..dbc78d4942 100644
--- a/stdio-common/vfprintf-process-arg.c
+++ b/stdio-common/vfprintf-process-arg.c
@@ -337,7 +337,7 @@ LABEL (form_number):
     }
   /* Answer the count of characters written.  */
   void *ptrptr = process_arg_pointer ();
-  unsigned int written = Xprintf_buffer_done (buf);
+  uint64_t written = buf->written + (buf->write_ptr - buf->write_base);
   if (is_longlong)
     *(long long int *) ptrptr = written;
   else if (is_long_num)
-- 
2.43.0

[PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Florian Weimer]

This defers buffer management largely to the asprintf implementation.
It is quite close to the original implementation around
open_memstream, except that an on-stack buffer is used for shorter
messages, and that strftime no longer writes directly into the
buffer.

The new version no longer uses the (slow) %n format specifier.
It also fixes an issue in the localtime_r failure path, where
the message is prefixed with ": " due to an incorrect placement
of the %n specifier.
---
 misc/syslog.c | 184 ++++++++++++++++----------------------------------
 1 file changed, 58 insertions(+), 126 deletions(-)

diff --git a/misc/syslog.c b/misc/syslog.c
index 68ee3aef5f..50cb252f47 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -38,13 +38,11 @@ static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
 #include <paths.h>
 #include <stdarg.h>
 #include <stdlib.h>
-#include <stdio.h>
-#include <stdio_ext.h>
 #include <sys/socket.h>
-#include <sys/uio.h>
 #include <sys/un.h>
 #include <syslog.h>
 #include <limits.h>
+#include <printf_buffer.h>
 
 static int LogType = SOCK_DGRAM;	/* type of socket connection */
 static int LogFile = -1;		/* fd for log */
@@ -60,27 +58,23 @@ __libc_lock_define_initialized (static, syslog_lock)
 static void openlog_internal (const char *, int, int);
 static void closelog_internal (void);
 
-struct cleanup_arg
+/* Unlock and deallocate the buffer.  */
+static void
+cancel_handler_buf (void *ptr)
 {
-  void *buf;
-  struct sigaction *oldaction;
-};
+  __libc_lock_unlock (syslog_lock);
+
+  struct __printf_buffer_asprintf *buf = ptr;
+  __printf_buffer_asprintf_free (buf);
+}
 
+/* Only unlock.  */
 static void
 cancel_handler (void *ptr)
 {
-  /* Restore the old signal handler.  */
-  struct cleanup_arg *clarg = (struct cleanup_arg *) ptr;
-
-  if (clarg != NULL)
-    /* Free the memstream buffer,  */
-    free (clarg->buf);
-
-  /* Free the lock.  */
   __libc_lock_unlock (syslog_lock);
 }
 
-
 /*
  * syslog, vsyslog --
  *	print message on log file; output is intended for syslogd(8).
@@ -122,16 +116,21 @@ __vsyslog_chk (int pri, int flag, const char *fmt, va_list ap)
   __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
 }
 
+/* Send the buffer contents over the syslog socket.  */
+static ssize_t
+__syslog_send (struct __printf_buffer *buf)
+{
+  /* Send the trailing NUL byte for LogType == SOCK_STREAM  only.  */
+  return __send (LogFile, buf->write_base,
+		 buf->write_ptr - buf->write_base - (LogType != SOCK_STREAM),
+		 MSG_NOSIGNAL);
+}
+
 void
 __vsyslog_internal (int pri, const char *fmt, va_list ap,
 		    unsigned int mode_flags)
 {
-  /* Try to use a static buffer as an optimization.  */
-  char bufs[1024];
-  char *buf = bufs;
-  size_t bufsize;
-
-  int msgoff;
+  ptrdiff_t msgoff;
   int saved_errno = errno;
 
 #define	INTERNALLOG LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
@@ -144,8 +143,9 @@ __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 = { NULL, NULL };
-  __libc_cleanup_push (cancel_handler, &clarg);
+  struct __printf_buffer_asprintf buf;
+  __printf_buffer_asprintf_init (&buf);
+  __libc_cleanup_push (cancel_handler_buf, &buf);
   __libc_lock_lock (syslog_lock);
 
   /* Check priority against setlogmask values. */
@@ -173,122 +173,56 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
     __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", now_tmp,
 		  _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)
+  __printf_buffer (&buf.base, "<%d>", pri);
 
-#define SYSLOG_HEADER_WITHOUT_TS(__pri, __msgoff)        \
-  "<%d>: %n", __pri, __msgoff
-
-  int l, vl;
   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 < 0)
-    goto out;
-
-  char *pos;
-  size_t len;
-
-  if (l < sizeof bufs)
     {
-      /* At this point, there is still a chance that we can print the
-         remaining part of the log into bufs and use that.  */
-      pos = bufs + l;
-      len = sizeof (bufs) - l;
+      __printf_buffer_puts (&buf.base, timestamp);
+      msgoff = buf.base.write_ptr - buf.base.write_base;
+       __printf_buffer_puts (&buf.base, LogTag == NULL ? __progname : LogTag);
+      if (pid != 0)
+	__printf_buffer (&buf.base, "[%d]", pid);
+      __printf_buffer_putc (&buf.base, ':');
+      __printf_buffer_putc (&buf.base, ' ');
     }
   else
     {
-      buf = NULL;
-      /* We already know that bufs is too small to use for this log message.
-         The next vsnprintf into bufs is used only to calculate the total
-         required buffer length.  We will discard bufs contents and allocate
-         an appropriately sized buffer later instead.  */
-      pos = bufs;
-      len = sizeof (bufs);
+      msgoff = buf.base.write_ptr - buf.base.write_base;
+      __printf_buffer_putc (&buf.base, ':');
+      __printf_buffer_putc (&buf.base, ' ');
     }
 
-  {
-    va_list apc;
-    va_copy (apc, ap);
-
-    /* Restore errno for %m format.  */
-    __set_errno (saved_errno);
-
-    vl = __vsnprintf_internal (pos, len, fmt, apc, mode_flags);
-    va_end (apc);
+  /* Do not count the bytes written to the buffer so far.  This value
+     is negative and accounts for the existing buffer contents, which
+     is not upposed to be visible to the caller.  */
+  buf.base.written = buf.base.write_base - buf.base.write_ptr;
 
-    if (vl < 0 || vl >= INT_MAX - l)
-      goto out;
+  errno = saved_errno;
+  __vprintf_buffer (&buf.base, fmt, ap, mode_flags);
 
-    if (vl >= len)
-      buf = NULL;
-
-    bufsize = l + vl;
-  }
-
-  if (buf == NULL)
-    {
-      buf = malloc ((bufsize + 1) * sizeof (char));
-      if (buf != NULL)
-	{
-	  /* Tell the cancellation handler to free this buffer.  */
-	  clarg.buf = buf;
+  /* We may need the terminator if we are sending over a SOCK_STREAM
+     connection.  Add this unconditionally to simplify error handling.  */
+  __printf_buffer_putc (&buf.base, '\0');
 
-	  int cl;
-	  if (has_ts)
-	    cl = __snprintf (buf, l + 1,
-			     SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
-	  else
-	    cl = __snprintf (buf, l + 1,
-			     SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
-	  if (cl != l)
-	    goto out;
-
-	  va_list apc;
-	  va_copy (apc, ap);
-	  cl = __vsnprintf_internal (buf + l, bufsize - l + 1, fmt, apc,
-				     mode_flags);
-	  va_end (apc);
-
-	  if (cl != vl)
-	    goto out;
-	}
-      else
-        {
-          int bl;
-	  /* Nothing much to do but emit an error message.  */
-          bl = __snprintf (bufs, sizeof bufs,
-                           "out of memory[%d]", __getpid ());
-          if (bl < 0 || bl >= sizeof bufs)
-            goto out;
-
-          bufsize = bl;
-          buf = bufs;
-          msgoff = 0;
-        }
-    }
+  if (__printf_buffer_has_failed (&buf.base))
+    goto out;
 
   /* Output to stderr if requested. */
   if (LogStat & LOG_PERROR)
-    __dprintf (STDERR_FILENO, "%s%s", buf + msgoff,
-	       "\n" + (buf[bufsize - 1] == '\n'));
+    {
+      /* The buffer always starts with '<' and a digit, so there are
+	 at least two bytes, and -2 is valid offset from the next
+	 to-be-written byte.  -2 accounts for the NUL byte.  */
+      int has_nl = buf.base.write_ptr[-2] == '\n';
+      __dprintf (STDERR_FILENO, "%s%s", buf.base.write_base + msgoff,
+		 "\n" + has_nl);
+    }
 
   /* 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 || __syslog_send (&buf.base) < 0)
     {
       if (connected)
 	{
@@ -297,7 +231,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
 	  openlog_internal (NULL, LogStat | LOG_NDELAY, LogFacility);
 	}
 
-      if (!connected || __send (LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
+      if (!connected || __syslog_send (&buf.base) < 0)
 	{
 	  closelog_internal ();	/* attempt re-open next time */
 	  /*
@@ -311,7 +245,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
 	      (fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
 			    | O_CLOEXEC, 0)) >= 0)
 	    {
-	      __dprintf (fd, "%s\r\n", buf + msgoff);
+	      __dprintf (fd, "%s\r\n", buf.base.write_base + msgoff);
 	      __close (fd);
 	    }
 	}
@@ -321,9 +255,7 @@ out:
   /* End of critical section.  */
   __libc_cleanup_pop (0);
   __libc_lock_unlock (syslog_lock);
-
-  if (buf != bufs)
-    free (buf);
+  __printf_buffer_asprintf_free (&buf);
 }
 
 /* AF_UNIX address of local logger  */
-- 
2.43.0

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Adhemerval Zanella Netto]

On 09/02/24 12:24, Florian Weimer wrote:
> Introduce __glibc_nofortify_getdomainname to request disabling
> the fortification wrapper.

I am not very found of __glibc_nofortify_getdomainname escaping to an
installed header, but I don't have a better solution.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  misc/Makefile                 | 1 -
>  misc/getdomain.c              | 2 ++
>  posix/bits/unistd.h           | 3 ++-
>  sysdeps/mach/hurd/getdomain.c | 2 ++
>  4 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/Makefile b/misc/Makefile
> index c273ec6974..44ae89670a 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -209,7 +209,6 @@ routines := \
>  
>  # Exclude fortified routines from being built with _FORTIFY_SOURCE
>  routines_no_fortify += \
> -  getdomain \
>    gethostname \
>    syslog \
>    # routines_no_fortify
> diff --git a/misc/getdomain.c b/misc/getdomain.c
> index f6325644c9..f96636821f 100644
> --- a/misc/getdomain.c
> +++ b/misc/getdomain.c
> @@ -19,6 +19,8 @@
>     The result is null-terminated if LEN is large enough for the full
>     name and the terminator.  */
>  
> +#define __glibc_nofortify_getdomainname
> +
>  #include <errno.h>
>  #include <unistd.h>
>  #include <sys/param.h>
> diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
> index bd209ec28e..6ed2943bf1 100644
> --- a/posix/bits/unistd.h
> +++ b/posix/bits/unistd.h
> @@ -149,7 +149,8 @@ __NTH (gethostname (char *__buf, size_t __buflen))
>  #endif
>  
>  
> -#if defined __USE_MISC || (defined __USE_XOPEN && !defined __USE_UNIX98)
> +#if (defined __USE_MISC || (defined __USE_XOPEN && !defined __USE_UNIX98)) \
> +  && !defined __glibc_nofortify_getdomainname
>  __fortify_function int
>  __NTH (getdomainname (char *__buf, size_t __buflen))
>  {
> diff --git a/sysdeps/mach/hurd/getdomain.c b/sysdeps/mach/hurd/getdomain.c
> index 137ce9ad5b..e24ab2137e 100644
> --- a/sysdeps/mach/hurd/getdomain.c
> +++ b/sysdeps/mach/hurd/getdomain.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#define __glibc_nofortify_getdomainname
> +
>  #include <unistd.h>
>  #include "hurdhost.h"
>  

Re: [PATCH 02/11] misc: Build gethostname with fortification

[Adhemerval Zanella Netto]

On 09/02/24 12:24, Florian Weimer wrote:
> Introduce __glibc_nofortify_gethostname to request disabling
> the fortification wrapper.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  misc/Makefile                               | 1 -
>  misc/gethostname.c                          | 2 ++
>  posix/bits/unistd.h                         | 3 ++-
>  sysdeps/mach/hurd/gethostname.c             | 2 ++
>  sysdeps/posix/gethostname.c                 | 2 ++
>  sysdeps/unix/sysv/linux/alpha/gethostname.c | 2 ++
>  6 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/Makefile b/misc/Makefile
> index 44ae89670a..6d8528c925 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -209,7 +209,6 @@ routines := \
>  
>  # Exclude fortified routines from being built with _FORTIFY_SOURCE
>  routines_no_fortify += \
> -  gethostname \
>    syslog \
>    # routines_no_fortify
>  
> diff --git a/misc/gethostname.c b/misc/gethostname.c
> index 2c849a95c3..82800f8abb 100644
> --- a/misc/gethostname.c
> +++ b/misc/gethostname.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#define __glibc_nofortify_gethostname
> +
>  #include <errno.h>
>  #include <unistd.h>
>  
> diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
> index 6ed2943bf1..1230bba9a5 100644
> --- a/posix/bits/unistd.h
> +++ b/posix/bits/unistd.h
> @@ -138,7 +138,8 @@ getlogin_r (char *__buf, size_t __buflen)
>  #endif
>  
>  
> -#if defined __USE_MISC || defined __USE_UNIX98
> +#if defined __USE_MISC || defined __USE_UNIX98 \
> +  && !defined __glibc_nofortify_gethostname
>  __fortify_function int
>  __NTH (gethostname (char *__buf, size_t __buflen))
>  {
> diff --git a/sysdeps/mach/hurd/gethostname.c b/sysdeps/mach/hurd/gethostname.c
> index 91d7f45d6e..dbb7d75713 100644
> --- a/sysdeps/mach/hurd/gethostname.c
> +++ b/sysdeps/mach/hurd/gethostname.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#define __glibc_nofortify_gethostname
> +
>  #include <unistd.h>
>  #include "hurdhost.h"
>  
> diff --git a/sysdeps/posix/gethostname.c b/sysdeps/posix/gethostname.c
> index 3c50706b58..3c79f77dd3 100644
> --- a/sysdeps/posix/gethostname.c
> +++ b/sysdeps/posix/gethostname.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#define __glibc_nofortify_gethostname
> +
>  #include <errno.h>
>  #include <string.h>
>  #include <unistd.h>
> diff --git a/sysdeps/unix/sysv/linux/alpha/gethostname.c b/sysdeps/unix/sysv/linux/alpha/gethostname.c
> index 30da8f30e4..45eb1788d4 100644
> --- a/sysdeps/unix/sysv/linux/alpha/gethostname.c
> +++ b/sysdeps/unix/sysv/linux/alpha/gethostname.c
> @@ -15,6 +15,8 @@
>     License along with the GNU C Library.  If not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#define __glibc_nofortify_gethostname
> +
>  #include <errno.h>
>  #include <string.h>
>  #include <unistd.h>

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Andreas Schwab]

On Feb 12 2024, Adhemerval Zanella Netto wrote:

> On 09/02/24 12:24, Florian Weimer wrote:
>> Introduce __glibc_nofortify_getdomainname to request disabling
>> the fortification wrapper.
>
> I am not very found of __glibc_nofortify_getdomainname escaping to an
> installed header, but I don't have a better solution.

Can't this be fixed with an alias?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 03/11] libio: Add fortify wrapper for internal __snprintf

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> ---
>  debug/snprintf_chk.c  | 1 +
>  include/bits/stdio2.h | 9 +++++++++
>  include/stdio.h       | 1 +
>  3 files changed, 11 insertions(+)
> 
> diff --git a/debug/snprintf_chk.c b/debug/snprintf_chk.c
> index 995d805f5d..ea02efec3e 100644
> --- a/debug/snprintf_chk.c
> +++ b/debug/snprintf_chk.c
> @@ -40,4 +40,5 @@ ___snprintf_chk (char *s, size_t maxlen, int flag, size_t slen,
>  
>    return ret;
>  }
> +ldbl_hidden_def (___snprintf_chk, __snprintf_chk)
>  ldbl_strong_alias (___snprintf_chk, __snprintf_chk)

I am seeing a build failure on powerpc64le with config options 
--enable-stack-protector=all --enable-tunables=yes --enable-bind-now=yes 
--enable-profile=yes --enable-fortify-source=2 --enable-hardcoded-path-in-tests:

powerpc64le-glibc-linux-gnu-gcc gconv_conf.c -c -std=gnu11 -fgnu89-inline  -g -O2 -Wall -Wwrite-strings -Wundef -Werror -fmerge-all-constants -frounding-math -fstack-protector-all -fno-common -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -Wstrict-prototypes -Wold-style-definition -fmath-errno  -mabi=ieeelongdouble -Wno-psabi -mno-gnu-attribute -mlong-double-128  -fPIE [....]
In file included from gconv_conf.c:26:
../include/stdio.h:65:1: error: ‘asm’ declaration ignored due to conflict with previous rename [-Werror=pragmas]
   65 | stdio_hidden_ldbl_proto (__, snprintf_chk)
      | ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

All other ABIs build fine.

> diff --git a/include/bits/stdio2.h b/include/bits/stdio2.h
> index 6d7cf4306f..fb909c21ee 100644
> --- a/include/bits/stdio2.h
> +++ b/include/bits/stdio2.h
> @@ -1 +1,10 @@
>  #include <libio/bits/stdio2.h>
> +
> +__fortify_function int
> +__NTH (__snprintf (char *__restrict __s, size_t __n,
> +                   const char *__restrict __fmt, ...))
> +{
> +  return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
> +				   __glibc_objsize (__s), __fmt,
> +				   __va_arg_pack ());
> +}
> diff --git a/include/stdio.h b/include/stdio.h
> index 24f1652f19..364f4d22a1 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -62,6 +62,7 @@ extern int __vsscanf (const char *__restrict __s,
>  extern int __sprintf_chk (char *, int, size_t, const char *, ...) __THROW;
>  extern int __snprintf_chk (char *, size_t, int, size_t, const char *, ...)
>       __THROW;
> +stdio_hidden_ldbl_proto (__, snprintf_chk)
>  extern int __vsprintf_chk (char *, int, size_t, const char *,
>  			   __gnuc_va_list) __THROW;
>  extern int __vsnprintf_chk (char *, size_t, int, size_t, const char *,

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Florian Weimer]

* Andreas Schwab:

> On Feb 12 2024, Adhemerval Zanella Netto wrote:
>
>> On 09/02/24 12:24, Florian Weimer wrote:
>>> Introduce __glibc_nofortify_getdomainname to request disabling
>>> the fortification wrapper.
>>
>> I am not very found of __glibc_nofortify_getdomainname escaping to an
>> installed header, but I don't have a better solution.
>
> Can't this be fixed with an alias?

It would have to be an assembler-level alias that GCC doesn't know
about.  Would you prefer that?  It results in worse debugging
information, I think.

GCC's alias attribyte counts a definition, and we already have one due
to the inline function definition in the fortify header.

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Andreas Schwab]

On Feb 12 2024, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Feb 12 2024, Adhemerval Zanella Netto wrote:
>>
>>> On 09/02/24 12:24, Florian Weimer wrote:
>>>> Introduce __glibc_nofortify_getdomainname to request disabling
>>>> the fortification wrapper.
>>>
>>> I am not very found of __glibc_nofortify_getdomainname escaping to an
>>> installed header, but I don't have a better solution.
>>
>> Can't this be fixed with an alias?
>
> It would have to be an assembler-level alias that GCC doesn't know
> about.  Would you prefer that?  It results in worse debugging
> information, I think.

I didn't realize that all the fortified functions are compiled with
disabled fortification for that reason.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Florian Weimer]

* Andreas Schwab:

> On Feb 12 2024, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Feb 12 2024, Adhemerval Zanella Netto wrote:
>>>
>>>> On 09/02/24 12:24, Florian Weimer wrote:
>>>>> Introduce __glibc_nofortify_getdomainname to request disabling
>>>>> the fortification wrapper.
>>>>
>>>> I am not very found of __glibc_nofortify_getdomainname escaping to an
>>>> installed header, but I don't have a better solution.
>>>
>>> Can't this be fixed with an alias?
>>
>> It would have to be an assembler-level alias that GCC doesn't know
>> about.  Would you prefer that?  It results in worse debugging
>> information, I think.
>
> I didn't realize that all the fortified functions are compiled with
> disabled fortification for that reason.

Does this mean you are okay with the patch as posted?

Thanks,
Florian

Re: [PATCH 01/11] misc: Build getdomainname with fortification

[Andreas Schwab]

On Feb 13 2024, Florian Weimer wrote:

> Does this mean you are okay with the patch as posted?

Yes.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 04/11] syslog: Update misc/tst-syslog to check reported %n value

[Adhemerval Zanella Netto]

The change LGTM, just that it does not follow current file indentation
(tabs instead of whitespace).

On 09/02/24 12:25, Florian Weimer wrote:
> ---
>  misc/tst-syslog.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/misc/tst-syslog.c b/misc/tst-syslog.c
> index ced9ae8467..741693a7ff 100644
> --- a/misc/tst-syslog.c
> +++ b/misc/tst-syslog.c
> @@ -102,8 +102,10 @@ send_vsyslog (int options)
>          {
>            int facility = facilities[i];
>            int priority = priorities[j];
> -          call_vsyslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE,
> -                        facility, priority);
> +	  int pos = -1;
> +          call_vsyslog (facility | priority, "%n%s %d %d",
> +			&pos, SYSLOG_MSG_BASE, facility, priority);
> +	  TEST_COMPARE (pos, 0);
>          }
>      }
>  }
> @@ -117,8 +119,10 @@ send_syslog (int options)
>          {
>            int facility = facilities[i];
>            int priority = priorities[j];
> -          syslog (facility | priority, "%s %d %d", SYSLOG_MSG_BASE, facility,
> -                  priority);
> +	  int pos = -1;
> +          syslog (facility | priority, "%n%s %d %d",
> +		  &pos, SYSLOG_MSG_BASE, facility, priority);
> +	  TEST_COMPARE (pos, 0);
>          }
>      }
>  }

Re: [PATCH 03/11] libio: Add fortify wrapper for internal __snprintf

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 09/02/24 12:25, Florian Weimer wrote:
>> ---
>>  debug/snprintf_chk.c  | 1 +
>>  include/bits/stdio2.h | 9 +++++++++
>>  include/stdio.h       | 1 +
>>  3 files changed, 11 insertions(+)
>> 
>> diff --git a/debug/snprintf_chk.c b/debug/snprintf_chk.c
>> index 995d805f5d..ea02efec3e 100644
>> --- a/debug/snprintf_chk.c
>> +++ b/debug/snprintf_chk.c
>> @@ -40,4 +40,5 @@ ___snprintf_chk (char *s, size_t maxlen, int flag, size_t slen,
>>  
>>    return ret;
>>  }
>> +ldbl_hidden_def (___snprintf_chk, __snprintf_chk)
>>  ldbl_strong_alias (___snprintf_chk, __snprintf_chk)
>
> I am seeing a build failure on powerpc64le with config options 
> --enable-stack-protector=all --enable-tunables=yes --enable-bind-now=yes 
> --enable-profile=yes --enable-fortify-source=2 --enable-hardcoded-path-in-tests:
>
> powerpc64le-glibc-linux-gnu-gcc gconv_conf.c -c -std=gnu11 -fgnu89-inline  -g -O2 -Wall -Wwrite-strings -Wundef -Werror -fmerge-all-constants -frounding-math -fstack-protector-all -fno-common -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -Wstrict-prototypes -Wold-style-definition -fmath-errno  -mabi=ieeelongdouble -Wno-psabi -mno-gnu-attribute -mlong-double-128  -fPIE [....]
> In file included from gconv_conf.c:26:
> ../include/stdio.h:65:1: error: ‘asm’ declaration ignored due to conflict with previous rename [-Werror=pragmas]
>    65 | stdio_hidden_ldbl_proto (__, snprintf_chk)
>       | ^~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> All other ABIs build fine.

The issue seems to be that libio/bits/stdio-ldbl.h wants to alias
__snprintf_chk to __snprintf_chkieee128, and stdio_hidden_ldbl_proto
wants to create an alias to ___ieee128_snprintf_chk.

I don't know how to solve this properly.  It seems that so far, few of
the fortified variants in the printf function family have hidden aliases
for PLT avoidance.

Thanks,
Florian

Re: [PATCH 05/11] syslog: Build with fortification

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> This causes /proc/self/maps to be processed for certain syslog calls
> due to the use of %n.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  misc/Makefile      | 5 -----
>  misc/bits/syslog.h | 4 ++++
>  misc/syslog.c      | 3 +++
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/misc/Makefile b/misc/Makefile
> index 6d8528c925..236076a9d6 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -207,11 +207,6 @@ routines := \
>    writev \
>    # routines
>  
> -# Exclude fortified routines from being built with _FORTIFY_SOURCE
> -routines_no_fortify += \
> -  syslog \
> -  # routines_no_fortify
> -
>  generated += \
>    tst-allocate_once-mem.out \
>    tst-allocate_once.mtrace \
> diff --git a/misc/bits/syslog.h b/misc/bits/syslog.h
> index aadcd42000..e6f7938a3f 100644
> --- a/misc/bits/syslog.h
> +++ b/misc/bits/syslog.h
> @@ -31,11 +31,13 @@
>     redirections, e.g. long double asm redirections.  */
>  
>  #ifdef __va_arg_pack
> +# ifndef __glibc_nofortify_syslog
>  __fortify_function void
>  syslog (int __pri, const char *__fmt, ...)
>  {
>    __syslog_chk (__pri, __USE_FORTIFY_LEVEL - 1, __fmt, __va_arg_pack ());
>  }
> +# endif
>  #elif !defined __cplusplus
>  # define syslog(pri, ...) \
>    __syslog_chk (pri, __USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
> @@ -43,9 +45,11 @@ syslog (int __pri, const char *__fmt, ...)
>  
>  
>  #ifdef __USE_MISC
> +# ifndef __glibc_nofortify_vsyslog
>  __fortify_function void
>  vsyslog (int __pri, const char *__fmt, __gnuc_va_list __ap)
>  {
>    __vsyslog_chk (__pri,  __USE_FORTIFY_LEVEL - 1, __fmt, __ap);
>  }
> +# endif
>  #endif
> diff --git a/misc/syslog.c b/misc/syslog.c
> index 4af87f54fd..68ee3aef5f 100644
> --- a/misc/syslog.c
> +++ b/misc/syslog.c
> @@ -27,6 +27,9 @@
>   * SUCH DAMAGE.
>   */
>  
> +#define __glibc_nofortify_syslog
> +#define __glibc_nofortify_vsyslog
> +
>  #if defined(LIBC_SCCS) && !defined(lint)
>  static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
>  #endif /* LIBC_SCCS and not lint */

Re: [PATCH 03/11] libio: Add fortify wrapper for internal __snprintf

[Adhemerval Zanella Netto]

On 13/02/24 09:13, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 09/02/24 12:25, Florian Weimer wrote:
>>> ---
>>>  debug/snprintf_chk.c  | 1 +
>>>  include/bits/stdio2.h | 9 +++++++++
>>>  include/stdio.h       | 1 +
>>>  3 files changed, 11 insertions(+)
>>>
>>> diff --git a/debug/snprintf_chk.c b/debug/snprintf_chk.c
>>> index 995d805f5d..ea02efec3e 100644
>>> --- a/debug/snprintf_chk.c
>>> +++ b/debug/snprintf_chk.c
>>> @@ -40,4 +40,5 @@ ___snprintf_chk (char *s, size_t maxlen, int flag, size_t slen,
>>>  
>>>    return ret;
>>>  }
>>> +ldbl_hidden_def (___snprintf_chk, __snprintf_chk)
>>>  ldbl_strong_alias (___snprintf_chk, __snprintf_chk)
>>
>> I am seeing a build failure on powerpc64le with config options 
>> --enable-stack-protector=all --enable-tunables=yes --enable-bind-now=yes 
>> --enable-profile=yes --enable-fortify-source=2 --enable-hardcoded-path-in-tests:
>>
>> powerpc64le-glibc-linux-gnu-gcc gconv_conf.c -c -std=gnu11 -fgnu89-inline  -g -O2 -Wall -Wwrite-strings -Wundef -Werror -fmerge-all-constants -frounding-math -fstack-protector-all -fno-common -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -Wstrict-prototypes -Wold-style-definition -fmath-errno  -mabi=ieeelongdouble -Wno-psabi -mno-gnu-attribute -mlong-double-128  -fPIE [....]
>> In file included from gconv_conf.c:26:
>> ../include/stdio.h:65:1: error: ‘asm’ declaration ignored due to conflict with previous rename [-Werror=pragmas]
>>    65 | stdio_hidden_ldbl_proto (__, snprintf_chk)
>>       | ^~~~~~~~~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>>
>> All other ABIs build fine.
> 
> The issue seems to be that libio/bits/stdio-ldbl.h wants to alias
> __snprintf_chk to __snprintf_chkieee128, and stdio_hidden_ldbl_proto
> wants to create an alias to ___ieee128_snprintf_chk.
> 
> I don't know how to solve this properly.  It seems that so far, few of
> the fortified variants in the printf function family have hidden aliases
> for PLT avoidance.

Maybe something like:

diff --git a/include/stdio.h b/include/stdio.h
index 364f4d22a1..0fda47cf82 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -24,7 +24,7 @@
     directly alias them to their internal name.  */
 # if __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1 && IS_IN (libc)
 #  define stdio_hidden_ldbl_proto(p, f) \
-  extern __typeof (p ## f) p ## f __asm (__ASMNAME ("___ieee128_" #f));
+  libc_hidden_proto_alias (p ## f, ___ieee128_ ## p ## f)
 # elif __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1
 #  define stdio_hidden_ldbl_proto(p,f) __LDBL_REDIR1_DECL (p ## f, p ## f ## ieee128)
 # else
diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-snprintf_chk.c b/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-snprintf_chk.c
index d5bd6aa25f..566f525a9c 100644
--- a/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-snprintf_chk.c
+++ b/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-snprintf_chk.c
@@ -40,3 +40,4 @@ ___ieee128___snprintf_chk (char *s, size_t maxlen, int flag, size_t slen,
   return done;
 }
 strong_alias (___ieee128___snprintf_chk, __snprintf_chkieee128)
+libc_hidden_ver (__snprintf_chkieee128, ___ieee128___snprintf_chk)

Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Adhemerval Zanella Netto]

On 09/02/24 12:26, Florian Weimer wrote:
> This defers buffer management largely to the asprintf implementation.
> It is quite close to the original implementation around
> open_memstream, except that an on-stack buffer is used for shorter
> messages, and that strftime no longer writes directly into the
> buffer.
> 
> The new version no longer uses the (slow) %n format specifier.
> It also fixes an issue in the localtime_r failure path, where
> the message is prefixed with ": " due to an incorrect placement
> of the %n specifier.

What I am not sure if this is really the direction we want for internal
FILE usage.  I had the impression that the printf buffer internal API
was mainly meant to improve the old FILE implementation and its historical
drawnbacks and limitations.  For internal usage we would continue to use
standard FILE API, should we move to always use printf buffers instead?

I am asking because it is a lot of code and refactoring for a specific
code that I would like to avoid change due the recent issues.  Most of 
complication is the static buffer optimization, so maybe we should just
remove it?

Also, since the motivation for this change is just to remove the %n
requirement, maybe we can just not enable it on syslog instead (since
we now that the internal calls should not act as a gadget)?  

diff --git a/misc/syslog.c b/misc/syslog.c
index 68ee3aef5f..53a17e612b 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -122,6 +122,29 @@ __vsyslog_chk (int pri, int flag, const char *fmt, va_list ap)
   __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
 }

+#if __USE_FORTIFY_LEVEL > 1
+static int
+syslog_snprintf_chk (char *s, size_t maxlen, size_t slen, const char *format,
+                    ...)
+{
+  if (__glibc_unlikely (slen < maxlen))
+    __chk_fail ();
+
+  /* Avoid the costly %n fortify check, so do not pass PRINTF_FORTIFY.  */
+  va_list ap;
+  va_start (ap, format);
+  int ret = __vsnprintf_internal (s, maxlen, format, ap, 0);
+  va_end (ap);
+
+  return ret;
+}
+# define syslog_snprintf(str, len, ...) \
+    syslog_snprintf_chk (str, len, __glibc_objsize (str), __VA_ARGS__)
+#else
+# define syslog_snprintf(str, len, ...) \
+    __snprintf (str, len, __VA_ARGS__)
+#endif
+
 void
 __vsyslog_internal (int pri, const char *fmt, va_list ap,
                    unsigned int mode_flags)
@@ -184,11 +207,11 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,

   int l, vl;
   if (has_ts)
-    l = __snprintf (bufs, sizeof bufs,
-                   SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+    l = syslog_snprintf (bufs, sizeof bufs,
+                        SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
   else
-    l = __snprintf (bufs, sizeof bufs,
-                   SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
+    l = syslog_snprintf (bufs, sizeof bufs,
+                        SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
   if (l < 0)
     goto out;

@@ -242,11 +265,11 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,

          int cl;
          if (has_ts)
-           cl = __snprintf (buf, l + 1,
-                            SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
+           cl = syslog_snprintf (buf, l + 1,
+                                 SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
          else
-           cl = __snprintf (buf, l + 1,
-                            SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
+           cl = syslog_snprintf (buf, l + 1,
+                                 SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
          if (cl != l)
            goto out;

@@ -263,8 +286,8 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
         {
           int bl;
          /* Nothing much to do but emit an error message.  */
-          bl = __snprintf (bufs, sizeof bufs,
-                           "out of memory[%d]", __getpid ());
+          bl = syslog_snprintf (bufs, sizeof bufs,
+                               "out of memory[%d]", __getpid ());
           if (bl < 0 || bl >= sizeof bufs)
             goto out;


> ---
>  misc/syslog.c | 184 ++++++++++++++++----------------------------------
>  1 file changed, 58 insertions(+), 126 deletions(-)
> 
> diff --git a/misc/syslog.c b/misc/syslog.c
> index 68ee3aef5f..50cb252f47 100644
> --- a/misc/syslog.c
> +++ b/misc/syslog.c
> @@ -38,13 +38,11 @@ static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
>  #include <paths.h>
>  #include <stdarg.h>
>  #include <stdlib.h>
> -#include <stdio.h>
> -#include <stdio_ext.h>
>  #include <sys/socket.h>
> -#include <sys/uio.h>
>  #include <sys/un.h>
>  #include <syslog.h>
>  #include <limits.h>
> +#include <printf_buffer.h>
>  
>  static int LogType = SOCK_DGRAM;	/* type of socket connection */
>  static int LogFile = -1;		/* fd for log */
> @@ -60,27 +58,23 @@ __libc_lock_define_initialized (static, syslog_lock)
>  static void openlog_internal (const char *, int, int);
>  static void closelog_internal (void);
>  
> -struct cleanup_arg
> +/* Unlock and deallocate the buffer.  */
> +static void
> +cancel_handler_buf (void *ptr)
>  {
> -  void *buf;
> -  struct sigaction *oldaction;
> -};
> +  __libc_lock_unlock (syslog_lock);
> +
> +  struct __printf_buffer_asprintf *buf = ptr;
> +  __printf_buffer_asprintf_free (buf);
> +}
>  
> +/* Only unlock.  */
>  static void
>  cancel_handler (void *ptr)
>  {
> -  /* Restore the old signal handler.  */
> -  struct cleanup_arg *clarg = (struct cleanup_arg *) ptr;
> -
> -  if (clarg != NULL)
> -    /* Free the memstream buffer,  */
> -    free (clarg->buf);
> -
> -  /* Free the lock.  */
>    __libc_lock_unlock (syslog_lock);
>  }
>  
> -
>  /*
>   * syslog, vsyslog --
>   *	print message on log file; output is intended for syslogd(8).
> @@ -122,16 +116,21 @@ __vsyslog_chk (int pri, int flag, const char *fmt, va_list ap)
>    __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
>  }
>  
> +/* Send the buffer contents over the syslog socket.  */
> +static ssize_t
> +__syslog_send (struct __printf_buffer *buf)
> +{
> +  /* Send the trailing NUL byte for LogType == SOCK_STREAM  only.  */
> +  return __send (LogFile, buf->write_base,
> +		 buf->write_ptr - buf->write_base - (LogType != SOCK_STREAM),
> +		 MSG_NOSIGNAL);
> +}
> +
>  void
>  __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  		    unsigned int mode_flags)
>  {
> -  /* Try to use a static buffer as an optimization.  */
> -  char bufs[1024];
> -  char *buf = bufs;
> -  size_t bufsize;
> -
> -  int msgoff;
> +  ptrdiff_t msgoff;
>    int saved_errno = errno;
>  
>  #define	INTERNALLOG LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
> @@ -144,8 +143,9 @@ __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 = { NULL, NULL };
> -  __libc_cleanup_push (cancel_handler, &clarg);
> +  struct __printf_buffer_asprintf buf;
> +  __printf_buffer_asprintf_init (&buf);
> +  __libc_cleanup_push (cancel_handler_buf, &buf);
>    __libc_lock_lock (syslog_lock);
>  
>    /* Check priority against setlogmask values. */
> @@ -173,122 +173,56 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>      __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", now_tmp,
>  		  _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)
> +  __printf_buffer (&buf.base, "<%d>", pri);
>  
> -#define SYSLOG_HEADER_WITHOUT_TS(__pri, __msgoff)        \
> -  "<%d>: %n", __pri, __msgoff
> -
> -  int l, vl;
>    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 < 0)
> -    goto out;
> -
> -  char *pos;
> -  size_t len;
> -
> -  if (l < sizeof bufs)
>      {
> -      /* At this point, there is still a chance that we can print the
> -         remaining part of the log into bufs and use that.  */
> -      pos = bufs + l;
> -      len = sizeof (bufs) - l;
> +      __printf_buffer_puts (&buf.base, timestamp);
> +      msgoff = buf.base.write_ptr - buf.base.write_base;
> +       __printf_buffer_puts (&buf.base, LogTag == NULL ? __progname : LogTag);
> +      if (pid != 0)
> +	__printf_buffer (&buf.base, "[%d]", pid);
> +      __printf_buffer_putc (&buf.base, ':');
> +      __printf_buffer_putc (&buf.base, ' ');
>      }
>    else
>      {
> -      buf = NULL;
> -      /* We already know that bufs is too small to use for this log message.
> -         The next vsnprintf into bufs is used only to calculate the total
> -         required buffer length.  We will discard bufs contents and allocate
> -         an appropriately sized buffer later instead.  */
> -      pos = bufs;
> -      len = sizeof (bufs);
> +      msgoff = buf.base.write_ptr - buf.base.write_base;
> +      __printf_buffer_putc (&buf.base, ':');
> +      __printf_buffer_putc (&buf.base, ' ');
>      }
>  
> -  {
> -    va_list apc;
> -    va_copy (apc, ap);
> -
> -    /* Restore errno for %m format.  */
> -    __set_errno (saved_errno);
> -
> -    vl = __vsnprintf_internal (pos, len, fmt, apc, mode_flags);
> -    va_end (apc);
> +  /* Do not count the bytes written to the buffer so far.  This value
> +     is negative and accounts for the existing buffer contents, which
> +     is not upposed to be visible to the caller.  */
> +  buf.base.written = buf.base.write_base - buf.base.write_ptr;
>  
> -    if (vl < 0 || vl >= INT_MAX - l)
> -      goto out;
> +  errno = saved_errno;
> +  __vprintf_buffer (&buf.base, fmt, ap, mode_flags);
>  
> -    if (vl >= len)
> -      buf = NULL;
> -
> -    bufsize = l + vl;
> -  }
> -
> -  if (buf == NULL)
> -    {
> -      buf = malloc ((bufsize + 1) * sizeof (char));
> -      if (buf != NULL)
> -	{
> -	  /* Tell the cancellation handler to free this buffer.  */
> -	  clarg.buf = buf;
> +  /* We may need the terminator if we are sending over a SOCK_STREAM
> +     connection.  Add this unconditionally to simplify error handling.  */
> +  __printf_buffer_putc (&buf.base, '\0');
>  
> -	  int cl;
> -	  if (has_ts)
> -	    cl = __snprintf (buf, l + 1,
> -			     SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
> -	  else
> -	    cl = __snprintf (buf, l + 1,
> -			     SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
> -	  if (cl != l)
> -	    goto out;
> -
> -	  va_list apc;
> -	  va_copy (apc, ap);
> -	  cl = __vsnprintf_internal (buf + l, bufsize - l + 1, fmt, apc,
> -				     mode_flags);
> -	  va_end (apc);
> -
> -	  if (cl != vl)
> -	    goto out;
> -	}
> -      else
> -        {
> -          int bl;
> -	  /* Nothing much to do but emit an error message.  */
> -          bl = __snprintf (bufs, sizeof bufs,
> -                           "out of memory[%d]", __getpid ());
> -          if (bl < 0 || bl >= sizeof bufs)
> -            goto out;
> -
> -          bufsize = bl;
> -          buf = bufs;
> -          msgoff = 0;
> -        }
> -    }
> +  if (__printf_buffer_has_failed (&buf.base))
> +    goto out;
>  
>    /* Output to stderr if requested. */
>    if (LogStat & LOG_PERROR)
> -    __dprintf (STDERR_FILENO, "%s%s", buf + msgoff,
> -	       "\n" + (buf[bufsize - 1] == '\n'));
> +    {
> +      /* The buffer always starts with '<' and a digit, so there are
> +	 at least two bytes, and -2 is valid offset from the next
> +	 to-be-written byte.  -2 accounts for the NUL byte.  */
> +      int has_nl = buf.base.write_ptr[-2] == '\n';
> +      __dprintf (STDERR_FILENO, "%s%s", buf.base.write_base + msgoff,
> +		 "\n" + has_nl);
> +    }
>  
>    /* 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 || __syslog_send (&buf.base) < 0)
>      {
>        if (connected)
>  	{
> @@ -297,7 +231,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  	  openlog_internal (NULL, LogStat | LOG_NDELAY, LogFacility);
>  	}
>  
> -      if (!connected || __send (LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
> +      if (!connected || __syslog_send (&buf.base) < 0)
>  	{
>  	  closelog_internal ();	/* attempt re-open next time */
>  	  /*
> @@ -311,7 +245,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  	      (fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
>  			    | O_CLOEXEC, 0)) >= 0)
>  	    {
> -	      __dprintf (fd, "%s\r\n", buf + msgoff);
> +	      __dprintf (fd, "%s\r\n", buf.base.write_base + msgoff);
>  	      __close (fd);
>  	    }
>  	}
> @@ -321,9 +255,7 @@ out:
>    /* End of critical section.  */
>    __libc_cleanup_pop (0);
>    __libc_lock_unlock (syslog_lock);
> -
> -  if (buf != bufs)
> -    free (buf);
> +  __printf_buffer_asprintf_free (&buf);
>  }
>  
>  /* AF_UNIX address of local logger  */

Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Florian Weimer]

* Adhemerval Zanella Netto:

> On 09/02/24 12:26, Florian Weimer wrote:
>> This defers buffer management largely to the asprintf implementation.
>> It is quite close to the original implementation around
>> open_memstream, except that an on-stack buffer is used for shorter
>> messages, and that strftime no longer writes directly into the
>> buffer.
>> 
>> The new version no longer uses the (slow) %n format specifier.
>> It also fixes an issue in the localtime_r failure path, where
>> the message is prefixed with ": " due to an incorrect placement
>> of the %n specifier.
>
> What I am not sure if this is really the direction we want for
> internal FILE usage.  I had the impression that the printf buffer
> internal API was mainly meant to improve the old FILE implementation
> and its historical drawnbacks and limitations.  For internal usage we
> would continue to use standard FILE API, should we move to always use
> printf buffers instead?

There are currently no internal uses (that I can see) of _IO_strfile for
writing purposes.  The psiginfo function uses __fmemopen, but that seems
excessive.  The syslog implementation was the only user of
open_memstream.  The reason for the open_memstream removal was
allocation removal, which makes sense for a logging function.

We must have manual constructs that use some printf function variant to
write to temporary buffers elsewhere.  One example is
stdio-common/psignal.c.  I can't find others outside sunrpc/ right now,
but they likely exist.  Switching those to fmemopen/open_memstream would
introduce the allocation issue as well, and dynamically sized printf
buffers could be a replacement.

I know that musl uses its _IO_strfile equivalent in such cases, but our
libio implementation is much more heavyweight, and it's hard to review
if new direct low-level uses of the libio internals are actually
correct.

> I am asking because it is a lot of code and refactoring for a specific
> code that I would like to avoid change due the recent issues.  Most of 
> complication is the static buffer optimization, so maybe we should just
> remove it?

Pretty much all buffer management code is deleted.  We now have even
fewer code of that than in the previous open_memstream-based function.

> Also, since the motivation for this change is just to remove the %n
> requirement, maybe we can just not enable it on syslog instead (since
> we now that the internal calls should not act as a gadget)?  

That doesn't remove the complicated buffer management.

Thanks,
Florian

Re: [PATCH 04/11] syslog: Update misc/tst-syslog to check reported %n value

[Florian Weimer]

* Adhemerval Zanella Netto:

> The change LGTM, just that it does not follow current file indentation
> (tabs instead of whitespace).

The file uses tabs elsewhere (e.g., to indent the parameter list in
check_syslog_message_large), that's why Emacs turned on tabs.  Just not
in the edited places.

Thanks,
Florian

Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Adhemerval Zanella Netto]

On 15/02/24 10:02, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 09/02/24 12:26, Florian Weimer wrote:
>>> This defers buffer management largely to the asprintf implementation.
>>> It is quite close to the original implementation around
>>> open_memstream, except that an on-stack buffer is used for shorter
>>> messages, and that strftime no longer writes directly into the
>>> buffer.
>>>
>>> The new version no longer uses the (slow) %n format specifier.
>>> It also fixes an issue in the localtime_r failure path, where
>>> the message is prefixed with ": " due to an incorrect placement
>>> of the %n specifier.
>>
>> What I am not sure if this is really the direction we want for
>> internal FILE usage.  I had the impression that the printf buffer
>> internal API was mainly meant to improve the old FILE implementation
>> and its historical drawnbacks and limitations.  For internal usage we
>> would continue to use standard FILE API, should we move to always use
>> printf buffers instead?
> 
> There are currently no internal uses (that I can see) of _IO_strfile for
> writing purposes.  The psiginfo function uses __fmemopen, but that seems
> excessive.  The syslog implementation was the only user of
> open_memstream.  The reason for the open_memstream removal was
> allocation removal, which makes sense for a logging function.
> 
> We must have manual constructs that use some printf function variant to
> write to temporary buffers elsewhere.  One example is
> stdio-common/psignal.c.  I can't find others outside sunrpc/ right now,
> but they likely exist.  Switching those to fmemopen/open_memstream would
> introduce the allocation issue as well, and dynamically sized printf
> buffers could be a replacement.

For unbounded messages I tend to agree that fmemopen/open_memstream might
be not the best option.  I would like to avoid having a another internal
API for such cases, but it seems that printf buffer is a slight better
option indeed.

> 
> I know that musl uses its _IO_strfile equivalent in such cases, but our
> libio implementation is much more heavyweight, and it's hard to review
> if new direct low-level uses of the libio internals are actually
> correct.
> 
>> I am asking because it is a lot of code and refactoring for a specific
>> code that I would like to avoid change due the recent issues.  Most of 
>> complication is the static buffer optimization, so maybe we should just
>> remove it?
> 
> Pretty much all buffer management code is deleted.  We now have even
> fewer code of that than in the previous open_memstream-based function.
> 
>> Also, since the motivation for this change is just to remove the %n
>> requirement, maybe we can just not enable it on syslog instead (since
>> we now that the internal calls should not act as a gadget)?  
> 
> That doesn't remove the complicated buffer management.

Right, the resulting code does seems simplified.

Re: [PATCH 06/11] stdio: Rename __printf_buffer to __vfprintf_buffer

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> And __wprintf_buffer to __vwprintf_buffer.  These functions take
> a va_list argument, so they are closer to the v-variant functions
> in <stdio.h>.
> 
> Also move the declaration to <printf_buffer.h>, which is arguably
> a better fit than <printf.h> (which contains lots of printf
> internals).

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  include/printf.h                 |  9 +--------
>  include/printf_buffer.h          |  9 +++++++++
>  libio/iovdprintf.c               |  2 +-
>  libio/iovsprintf.c               |  2 +-
>  libio/obprintf.c                 |  2 +-
>  libio/vasprintf.c                |  2 +-
>  libio/vsnprintf.c                |  2 +-
>  libio/vswprintf.c                |  2 +-
>  stdio-common/vfprintf-internal.c | 10 ++++++----
>  9 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/include/printf.h b/include/printf.h
> index 2c998059d4..78aac4dcad 100644
> --- a/include/printf.h
> +++ b/include/printf.h
> @@ -65,18 +65,11 @@ int __translated_number_width (locale_t loc,
>  			       const char *first, const char *last)
>    attribute_hidden;
>  
> -
> -struct __printf_buffer;
> -void __printf_buffer (struct __printf_buffer *buf, const char *format,
> -		      va_list ap, unsigned int mode_flags);
> -struct __wprintf_buffer;
> -void __wprintf_buffer (struct __wprintf_buffer *buf, const wchar_t *format,
> -		       va_list ap, unsigned int mode_flags);
> -
>  extern int __printf_fp (FILE *, const struct printf_info *,
>  			const void *const *);
>  libc_hidden_proto (__printf_fp)
>  
> +struct __printf_buffer;
>  void __printf_fphex_l_buffer (struct __printf_buffer *, locale_t,
>  			      const struct printf_info *,
>  			      const void *const *) attribute_hidden;
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index eff8e3413a..fb0b42178e 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -200,6 +200,12 @@ int __printf_buffer_done (struct __printf_buffer *buf) attribute_hidden;
>     known, and the flush implementation can be called directly.  */
>  bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden;
>  
> +/* Building block for printf-style functions.  Write argument list AP
> +   to BUF according to FORMAT.  MODE_FLAGS contains PRINTF_* flags
> +   defined in <libioP.h>.  */
> +void __vprintf_buffer (struct __printf_buffer *buf, const char *format,
> +                       va_list ap, unsigned int mode_flags) attribute_hidden;
> +
>  /* Wide version of struct __printf_buffer follows.  */
>  
>  enum __wprintf_buffer_mode
> @@ -274,6 +280,9 @@ int __wprintf_buffer_done (struct __wprintf_buffer *buf) attribute_hidden;
>  
>  bool __wprintf_buffer_flush (struct __wprintf_buffer *buf) attribute_hidden;
>  
> +void __vwprintf_buffer (struct __wprintf_buffer *buf, const wchar_t *format,
> +                        va_list ap, unsigned int mode_flags) attribute_hidden;
> +
>  /* Type-generic convenience macros.  They are useful if
>     printf_buffer-char.h or printf_buffer-wchar_t.h is included as
>     well.  */
> diff --git a/libio/iovdprintf.c b/libio/iovdprintf.c
> index 6894fa8347..47d9d0ba8f 100644
> --- a/libio/iovdprintf.c
> +++ b/libio/iovdprintf.c
> @@ -66,7 +66,7 @@ __vdprintf_internal (int d, const char *format, va_list arg,
>    __printf_buffer_init (&buf.base, buf.buf, array_length (buf.buf),
>  			__printf_buffer_mode_dprintf);
>    buf.fd = d;
> -  __printf_buffer (&buf.base, format, arg, mode_flags);
> +  __vprintf_buffer (&buf.base, format, arg, mode_flags);
>    if (__printf_buffer_has_failed (&buf.base))
>      return -1;
>    __printf_buffer_flush_dprintf (&buf);
> diff --git a/libio/iovsprintf.c b/libio/iovsprintf.c
> index 2d81ee3507..7fdc06dcd4 100644
> --- a/libio/iovsprintf.c
> +++ b/libio/iovsprintf.c
> @@ -59,7 +59,7 @@ __vsprintf_internal (char *string, size_t maxlen,
>      __printf_buffer_init_end (&buf, string, (char *) ~(uintptr_t) 0,
>  			      __printf_buffer_mode_sprintf);
>  
> -  __printf_buffer (&buf, format, args, mode_flags);
> +  __vprintf_buffer (&buf, format, args, mode_flags);
>  
>    /* Write the NUL terminator if there is room.  Do not use the putc
>       operation to avoid overflowing the character write count.  */
> diff --git a/libio/obprintf.c b/libio/obprintf.c
> index 491d14fbae..24d1edf260 100644
> --- a/libio/obprintf.c
> +++ b/libio/obprintf.c
> @@ -92,7 +92,7 @@ __obstack_vprintf_internal (struct obstack *obstack, const char *format,
>    /* Now allocate the rest of the current chunk.  */
>    obstack_blank_fast (obstack, room);
>  
> -  __printf_buffer (&buf.base, format, args, mode_flags);
> +  __vprintf_buffer (&buf.base, format, args, mode_flags);
>  
>    if (buf.base.write_ptr == &buf.ch + 1)
>      /* buf.ch is in use.  Put it into the obstack.  */
> diff --git a/libio/vasprintf.c b/libio/vasprintf.c
> index 999ae526f4..edcfab2323 100644
> --- a/libio/vasprintf.c
> +++ b/libio/vasprintf.c
> @@ -99,7 +99,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>    __printf_buffer_init (&buf.base, buf.direct, array_length (buf.direct),
>  			__printf_buffer_mode_asprintf);
>  
> -  __printf_buffer (&buf.base, format, args, mode_flags);
> +  __vprintf_buffer (&buf.base, format, args, mode_flags);
>    int done = __printf_buffer_done (&buf.base);
>    if (done < 0)
>      {
> diff --git a/libio/vsnprintf.c b/libio/vsnprintf.c
> index dd7a585cbe..7077fcd9f2 100644
> --- a/libio/vsnprintf.c
> +++ b/libio/vsnprintf.c
> @@ -93,7 +93,7 @@ __vsnprintf_internal (char *string, size_t maxlen, const char *format,
>  {
>    struct __printf_buffer_snprintf buf;
>    __printf_buffer_snprintf_init (&buf, string, maxlen);
> -  __printf_buffer (&buf.base, format, args, mode_flags);
> +  __vprintf_buffer (&buf.base, format, args, mode_flags);
>    return __printf_buffer_snprintf_done (&buf);
>  }
>  
> diff --git a/libio/vswprintf.c b/libio/vswprintf.c
> index 5990bd2eff..d73246ba41 100644
> --- a/libio/vswprintf.c
> +++ b/libio/vswprintf.c
> @@ -41,7 +41,7 @@ __vswprintf_internal (wchar_t *string, size_t maxlen, const wchar_t *format,
>    struct __wprintf_buffer buf;
>    __wprintf_buffer_init (&buf, string, maxlen, __wprintf_buffer_mode_swprintf);
>  
> -  __wprintf_buffer (&buf, format, args, mode_flags);
> +  __vwprintf_buffer (&buf, format, args, mode_flags);
>  
>    if (buf.write_ptr == buf.write_end)
>      {
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index 771beca9bf..23754232f2 100644
> --- a/stdio-common/vfprintf-internal.c
> +++ b/stdio-common/vfprintf-internal.c
> @@ -122,6 +122,7 @@
>  
>  #ifndef COMPILE_WPRINTF
>  # include "printf_buffer-char.h"
> +# define VFPRINTF_BUFFER __vprintf_buffer
>  # define vfprintf	__vfprintf_internal
>  # define OTHER_CHAR_T   wchar_t
>  # define UCHAR_T	unsigned char
> @@ -136,6 +137,7 @@ typedef const char *THOUSANDS_SEP_T;
>  # define CONVERT_FROM_OTHER_STRING __wcsrtombs
>  #else
>  # include "printf_buffer-wchar_t.h"
> +# define VFPRINTF_BUFFER __vwprintf_buffer
>  # define vfprintf	__vfwprintf_internal
>  # define OTHER_CHAR_T   char
>  /* This is a hack!!!  There should be a type uwchar_t.  */
> @@ -595,8 +597,8 @@ static void group_number (struct Xprintf_buffer *buf,
>  
>  /* The buffer-based function itself.  */
>  void
> -Xprintf_buffer (struct Xprintf_buffer *buf, const CHAR_T *format,
> -		  va_list ap, unsigned int mode_flags)
> +VFPRINTF_BUFFER (struct Xprintf_buffer *buf, const CHAR_T *format,
> +		 va_list ap, unsigned int mode_flags)
>  {
>    /* The character used as thousands separator.  */
>    THOUSANDS_SEP_T thousands_sep = 0;
> @@ -1541,7 +1543,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
>      {
>        struct Xprintf (buffer_to_file) wrap;
>        Xprintf (buffer_to_file_init) (&wrap, s);
> -      Xprintf_buffer (&wrap.base, format, ap, mode_flags);
> +      VFPRINTF_BUFFER (&wrap.base, format, ap, mode_flags);
>        return Xprintf (buffer_to_file_done) (&wrap);
>      }
>  
> @@ -1556,7 +1558,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
>    Xprintf (buffer_to_file_init) (&wrap, s);
>  
>    /* Perform the printing operation on the buffer.  */
> -  Xprintf_buffer (&wrap.base, format, ap, mode_flags);
> +  VFPRINTF_BUFFER (&wrap.base, format, ap, mode_flags);
>    done = Xprintf (buffer_to_file_done) (&wrap);
>  
>    /* Unlock the stream.  */

Re: [PATCH 07/11] libio: Extract __printf_buffer_asprintf_init from asprintf implementation

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> And __printf_buffer_asprintf_free as well.  This will allow to reuse
> the asprintf implementation for more general buffer handling, similar
> to open_memstream, but without the mandatory heap allocation.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  include/printf_buffer.h | 30 +++++++++++++++++++++++++-----
>  libio/vasprintf.c       | 40 +++++++++++++++++-----------------------
>  2 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index fb0b42178e..cee5afb581 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -317,6 +317,31 @@ void __printf_buffer_snprintf_init (struct __printf_buffer_snprintf *,
>  int __printf_buffer_snprintf_done (struct __printf_buffer_snprintf *)
>    attribute_hidden;
>  
> +/* Size of the initial on-stack buffer for asprintf.  It should be
> +   large enough to copy almost all asprintf usages with just a single
> +   (final, correctly sized) heap allocation.  */
> +#define PRINTF_BUFFER_SIZE_ASPRINTF 200

On my system the journactl message has a mean size of 124 and a median
of so I guess a size of 200 should cover most of the usage. 

> +
> +struct __printf_buffer_asprintf
> +{
> +  /* base.write_base points either to a heap-allocated buffer, or to
> +     the direct array below.  */
> +  struct __printf_buffer base;
> +
> +  /* Initial allocation.  */
> +  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
> +};
> +
> +/* Sets up BUF for writing via __printf_buffer.  */
> +void __printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
> +  attribute_hidden;
> +
> +/* Deallocates any allocated memory in *BUF.  (This is not the usual
> +   done routine for asprintf because that has to preserve allocation.)
> +   Always returns -1 to indicate an error.  */
> +int __printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
> +  attribute_hidden;
> +
>  /* Flush function implementations follow.  They are called from
>     __printf_buffer_flush.  Generic code should not call these flush
>     functions directly.  Some modes have inline implementations.  */
> @@ -363,11 +388,6 @@ void __wprintf_buffer_flush_to_file (struct __wprintf_buffer_to_file *)
>  /* Temporary buffer used during floating point digit translation.  */
>  #define PRINTF_BUFFER_SIZE_DIGITS 64
>  
> -/* Size of the initial on-stack buffer for asprintf.  It should be
> -   large enough to copy almost all asprintf usages with just a single
> -   (final, correctly sized) heap allocation.  */
> -#define PRINTF_BUFFER_SIZE_ASPRINTF 200
> -
>  /* This should cover most of the packet-oriented file descriptors,
>     where boundaries between writes could be visible to readers.  But
>     it is still small enough not to cause too many stack overflow issues.  */
> diff --git a/libio/vasprintf.c b/libio/vasprintf.c
> index edcfab2323..f8413eedfe 100644
> --- a/libio/vasprintf.c
> +++ b/libio/vasprintf.c
> @@ -34,18 +34,6 @@
>  #include <string.h>
>  #include <printf_buffer.h>
>  
> -struct __printf_buffer_asprintf
> -{
> -  /* base.write_base points either to a heap-allocated buffer, or to
> -     the direct array below.  */
> -  struct __printf_buffer base;
> -
> -  /* Initial allocation.  200 should be large enough to copy almost
> -     all asprintf usages with just a single (final, correctly sized)
> -     heap allocation.  */
> -  char direct[PRINTF_BUFFER_SIZE_ASPRINTF];
> -};
> -
>  void
>  __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>  {
> @@ -90,23 +78,32 @@ __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>    buf->base.write_end = new_buffer + new_size;
>  }
>  
> +void
> +__printf_buffer_asprintf_init (struct __printf_buffer_asprintf *buf)
> +{
> +  __printf_buffer_init (&buf->base, buf->direct, array_length (buf->direct),
> +			__printf_buffer_mode_asprintf);
> +}
> +
> +int
> +__printf_buffer_asprintf_free (struct __printf_buffer_asprintf *buf)
> +{
> +  if (buf->base.write_base != buf->direct)
> +    free (buf->base.write_base);
> +  return -1;
> +}
>  
>  int
>  __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>  		      unsigned int mode_flags)
>  {
>    struct __printf_buffer_asprintf buf;
> -  __printf_buffer_init (&buf.base, buf.direct, array_length (buf.direct),
> -			__printf_buffer_mode_asprintf);
> +  __printf_buffer_asprintf_init (&buf);
>  
>    __vprintf_buffer (&buf.base, format, args, mode_flags);
>    int done = __printf_buffer_done (&buf.base);
>    if (done < 0)
> -    {
> -      if (buf.base.write_base != buf.direct)
> -	free (buf.base.write_base);
> -      return done;
> -    }
> +    return __printf_buffer_asprintf_free (&buf);
>  
>    /* Transfer to the final buffer.  */
>    char *result;
> @@ -122,10 +119,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>      {
>        result = realloc (buf.base.write_base, size + 1);
>        if (result == NULL)
> -	{
> -	  free (buf.base.write_base);
> -	  return -1;
> -	}
> +	return __printf_buffer_asprintf_free (&buf);
>      }
>  
>    /* Add NUL termination.  */

Re: [PATCH 08/11] stdio-common: Introduce the __printf_buffer function

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> It can be used to write to a __print_buffer (incrementally).

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  include/printf_buffer.h          |   5 ++
>  stdio-common/Makefile            |   5 ++
>  stdio-common/printf_buffer.c     |  30 +++++++
>  stdio-common/tst-printf_buffer.c | 138 +++++++++++++++++++++++++++++++
>  4 files changed, 178 insertions(+)
>  create mode 100644 stdio-common/printf_buffer.c
>  create mode 100644 stdio-common/tst-printf_buffer.c
> 
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index cee5afb581..a5483e8a97 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -206,6 +206,11 @@ bool __printf_buffer_flush (struct __printf_buffer *buf) attribute_hidden;
>  void __vprintf_buffer (struct __printf_buffer *buf, const char *format,
>                         va_list ap, unsigned int mode_flags) attribute_hidden;
>  
> +/* Write the argument list to BUF according to FORMAT.
> +   Unconditionally enables fortification (special procesing for %n).  */
> +void __printf_buffer (struct __printf_buffer *buf, const char *format, ...)
> +  __attribute__ ((__format__ (__printf__, 2, 3))) attribute_hidden;
> +
>  /* Wide version of struct __printf_buffer follows.  */
>  
>  enum __wprintf_buffer_mode

Ok.

> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 6447b6b444..d610ed67e6 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -64,6 +64,7 @@ routines := \
>    perror \
>    printf \
>    printf-prs \
> +  printf_buffer \
>    printf_buffer_as_file \
>    printf_buffer_done \
>    printf_buffer_flush \
> @@ -290,6 +291,10 @@ test-srcs = \
>    tst-unbputc \
>    # test-srcs
>  
> +# Test for the internal, non-exported __printf_buffer function.
> +tests-internal += tst-printf_buffer
> +tests-static += tst-printf_buffer
> +

Maybe put in a newline to be future-proof.

>  ifeq ($(run-built-tests),yes)
>  tests-special += \
>    $(objpfx)tst-printf-bz18872-mem.out \
> diff --git a/stdio-common/printf_buffer.c b/stdio-common/printf_buffer.c
> new file mode 100644
> index 0000000000..fabb3b5bb4
> --- /dev/null
> +++ b/stdio-common/printf_buffer.c
> @@ -0,0 +1,30 @@
> +/* Write a format string to a buffer.
> +   Copyright (C) 2024 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 <printf_buffer.h>
> +#include <libioP.h>
> +#include <stdarg.h>
> +
> +void
> +__printf_buffer (struct __printf_buffer *buf, const char *format, ...)
> +{
> +  va_list arg;
> +  va_start (arg, format);
> +  __vprintf_buffer (buf, format, arg, PRINTF_FORTIFY);
> +  va_end (arg);
> +}
> diff --git a/stdio-common/tst-printf_buffer.c b/stdio-common/tst-printf_buffer.c
> new file mode 100644
> index 0000000000..d12da8c939
> --- /dev/null
> +++ b/stdio-common/tst-printf_buffer.c
> @@ -0,0 +1,138 @@
> +/* Basic test for the __printf_buffer function (via asprintf).
> +   Copyright (C) 2024 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 <printf_buffer.h>
> +#include <array_length.h>
> +#include <string.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +{
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "%s", "");
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc");
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 3);
> +    TEST_COMPARE_BLOB (buf.direct, 3, "abc", 3);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc[%d]DEF", 17);
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 10);
> +    TEST_COMPARE_BLOB (buf.direct, 10, "abc[17]DEF", 10);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  /* Test concatenation.  */
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc");
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 3);
> +    TEST_COMPARE_BLOB (buf.direct, 3, "abc", 3);
> +    __printf_buffer (&buf.base, "[%d]DEF", 17);
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_VERIFY (buf.base.write_ptr == buf.base.write_base + 10);
> +    TEST_COMPARE_BLOB (buf.direct, 10, "abc[17]DEF", 10);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    char large_string[500];
> +    memset (large_string, 'X', array_length (large_string));
> +    large_string[array_length (large_string) -1] = '\0';
> +    int n;
> +
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "%s", large_string);
> +    TEST_VERIFY (buf.base.write_base != buf.direct);
> +    TEST_COMPARE_BLOB (buf.base.write_base,
> +                       buf.base.write_ptr - buf.base.write_base,
> +                       large_string, strlen (large_string));
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc");
> +    __printf_buffer (&buf.base, "%n", &n);
> +    /* %n counts from the start of the buffer, not from within the
> +        printf call.  */
> +    TEST_COMPARE (n, 3);
> +    __printf_buffer (&buf.base, "%s", large_string);
> +    TEST_VERIFY (buf.base.write_base != buf.direct);
> +    TEST_COMPARE_BLOB (buf.base.write_base, 3, "abc", 3);
> +    TEST_COMPARE_BLOB (buf.base.write_base + 3,
> +                       buf.base.write_ptr - buf.base.write_base - 3,
> +                       large_string, strlen (large_string));
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    __printf_buffer (&buf.base, "%n", &n);
> +    TEST_COMPARE (n, 3 + strlen (large_string));
> +    TEST_COMPARE_BLOB (buf.base.write_base, 3, "abc", 3);
> +    TEST_COMPARE_BLOB (buf.base.write_base + 3,
> +                       buf.base.write_ptr - buf.base.write_base - 3,
> +                       large_string, strlen (large_string));
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
> +  /* Test %n with concatenation.  */
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc");
> +    int n = -1;
> +    __printf_buffer (&buf.base, "D%nEF", &n);
> +    TEST_COMPARE (n, 4);        /* %n counts from the start of the buffer.  */
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_COMPARE_BLOB (buf.base.write_base,
> +                       buf.base.write_ptr - buf.base.write_base, "abcDEF", 6);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Re: [PATCH 09/11] stdio-common: Allow skipping initial bytes in __printf_buffer for %n

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> Making the written field signed allows subtraction of a prefix count
> in case multiple writes happen to the same buffer.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  include/printf_buffer.h          |  8 +++++---
>  stdio-common/tst-printf_buffer.c | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/printf_buffer.h b/include/printf_buffer.h
> index a5483e8a97..2400371b94 100644
> --- a/include/printf_buffer.h
> +++ b/include/printf_buffer.h
> @@ -92,8 +92,10 @@ struct __printf_buffer
>       buffer).  Potentially updated on flush.  The actual number of
>       written bytes also includes the unflushed-but-written buffer
>       part, write_ptr - write_base.  A 64-bit value is used to avoid
> -     the need for overflow checks.  */
> -  uint64_t written;
> +     the need for intermediate overflow checks.  Negative values can
> +     be used to ignore leading parts of the buffer for %n computations
> +     (for repeated __printf_buffer calls to the same buffer).  */
> +  int64_t written;
>  
>    /* Identifies the flush callback.  */
>    enum __printf_buffer_mode mode;
> @@ -225,7 +227,7 @@ struct __wprintf_buffer
>    wchar_t *write_base;
>    wchar_t *write_ptr;
>    wchar_t *write_end;
> -  uint64_t written;
> +  int64_t written;
>    enum __wprintf_buffer_mode mode;
>  };
>  
> diff --git a/stdio-common/tst-printf_buffer.c b/stdio-common/tst-printf_buffer.c
> index d12da8c939..2a3ee511d0 100644
> --- a/stdio-common/tst-printf_buffer.c
> +++ b/stdio-common/tst-printf_buffer.c
> @@ -132,6 +132,24 @@ do_test (void)
>      TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
>      TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
>    }
> +
> +  /* Test %n with initial skip.  */
> +  {
> +    struct __printf_buffer_asprintf buf;
> +    __printf_buffer_asprintf_init (&buf);
> +    __printf_buffer (&buf.base, "abc");
> +    buf.base.written = -2;
> +    int n = -1;
> +    __printf_buffer (&buf.base, "D%nEF", &n);
> +    TEST_COMPARE (n, 2);        /* Two characters were skipped.  */
> +    TEST_VERIFY (buf.base.write_base == buf.direct);
> +    TEST_COMPARE_BLOB (buf.base.write_base,
> +                       buf.base.write_ptr - buf.base.write_base, "abcDEF", 6);
> +    TEST_VERIFY (!__printf_buffer_has_failed (&buf.base));
> +    TEST_COMPARE (__printf_buffer_done (&buf.base), 4); /* Two skipped.  */
> +    TEST_COMPARE (__printf_buffer_asprintf_free (&buf), -1);
> +  }
> +
>    return 0;
>  }
>  

Re: [PATCH 10/11] stdio-common: Support large offsets with %lln

[Adhemerval Zanella Netto]

On 09/02/24 12:25, Florian Weimer wrote:
> Use of Xprintf_buffer_done is unnecessary because it performs
> overflow detection against int and sets errno.
> ---
>  stdio-common/Makefile               |  1 +
>  stdio-common/tst-printf-large-n.c   | 81 +++++++++++++++++++++++++++++
>  stdio-common/vfprintf-process-arg.c |  2 +-
>  3 files changed, 83 insertions(+), 1 deletion(-)
>  create mode 100644 stdio-common/tst-printf-large-n.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index d610ed67e6..482d841f96 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -234,6 +234,7 @@ tests := \
>    tst-printf-fp-free \
>    tst-printf-fp-leak \
>    tst-printf-intn \
> +  tst-printf-large-n \
>    tst-printf-oct \
>    tst-printf-round \
>    tst-printfsz \
> diff --git a/stdio-common/tst-printf-large-n.c b/stdio-common/tst-printf-large-n.c
> new file mode 100644
> index 0000000000..4504a693d2
> --- /dev/null
> +++ b/stdio-common/tst-printf-large-n.c
> @@ -0,0 +1,81 @@
> +/* Test that %n can report values larger than INT_MAX.
> +   Copyright (C) 2024 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 <errno.h>
> +#include <limits.h>
> +#include <stdio.h>
> +#include <wchar.h>
> +
> +#include <support/blob_repeat.h>
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +
> +static int
> +do_test (void)
> +{
> +  enum { string_length = 1 << 28 };
> +
> +  FILE *fp = xfopen ("/dev/null", "w");
> +
> +  /* Byte-oriented streams.  */
> +  {
> +    struct support_blob_repeat repeat
> +      = support_blob_repeat_allocate ("a", 1, string_length + 1);
> +    char *string = repeat.start;
> +    if (string == NULL)
> +      FAIL_UNSUPPORTED ("cannot allocate large string");
> +    string[string_length] = '\0';
> +
> +      long long int n;

Indentation seems off here.

> +    TEST_COMPARE (fprintf (fp, "%s %s %s %s %s %s %s %s %lln",
> +                           string, string, string, string, string, string,
> +                           string, string, &n), -1);
> +    TEST_COMPARE (errno, EOVERFLOW);
> +    TEST_COMPARE (n, 8 * (string_length + 1LL));
> +    TEST_VERIFY (n > INT_MAX);
> +
> +      support_blob_repeat_free (&repeat);

Here as well.

> +  }
> +
> +  /* Wide-oriented streams.  */
> +  {
> +    struct support_blob_repeat repeat
> +      = support_blob_repeat_allocate (L"a", sizeof (wchar_t),
> +                                      string_length + 1);
> +    wchar_t *string = repeat.start;
> +    if (string == NULL)
> +      FAIL_UNSUPPORTED ("cannot allocate large wide string");
> +    string[string_length] = '\0';

It should not matter, but maybe L'\0' here?

> +
> +    long long int n;
> +    TEST_COMPARE (fwprintf (fp, L"%Ls %Ls %Ls %Ls %Ls %Ls %Ls %Ls %lln",
> +                            string, string, string, string, string, string,
> +                            string, string, &n), -1);
> +    TEST_COMPARE (errno, EOVERFLOW);
> +    TEST_COMPARE (n, 8 * (string_length + 1LL));
> +    TEST_VERIFY (n > INT_MAX);
> +
> +      support_blob_repeat_free (&repeat);
> +  }
> +
> +  xfclose (fp);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c
> index af6b570ea9..dbc78d4942 100644
> --- a/stdio-common/vfprintf-process-arg.c
> +++ b/stdio-common/vfprintf-process-arg.c
> @@ -337,7 +337,7 @@ LABEL (form_number):
>      }
>    /* Answer the count of characters written.  */
>    void *ptrptr = process_arg_pointer ();
> -  unsigned int written = Xprintf_buffer_done (buf);
> +  uint64_t written = buf->written + (buf->write_ptr - buf->write_base);
>    if (is_longlong)
>      *(long long int *) ptrptr = written;
>    else if (is_long_num)

This is strictly an alias violation, but the rest of the code follow this
pattern so it should be ok.

Re: [PATCH 11/11] syslog: Use a printf buffer directly to construct the entire packet

[Adhemerval Zanella Netto]

On 09/02/24 12:26, Florian Weimer wrote:
> This defers buffer management largely to the asprintf implementation.
> It is quite close to the original implementation around
> open_memstream, except that an on-stack buffer is used for shorter
> messages, and that strftime no longer writes directly into the
> buffer.
> 
> The new version no longer uses the (slow) %n format specifier.
> It also fixes an issue in the localtime_r failure path, where
> the message is prefixed with ": " due to an incorrect placement
> of the %n specifier.
> ---
>  misc/syslog.c | 184 ++++++++++++++++----------------------------------
>  1 file changed, 58 insertions(+), 126 deletions(-)
> 
> diff --git a/misc/syslog.c b/misc/syslog.c
> index 68ee3aef5f..50cb252f47 100644
> --- a/misc/syslog.c
> +++ b/misc/syslog.c
> @@ -38,13 +38,11 @@ static char sccsid[] = "@(#)syslog.c	8.4 (Berkeley) 3/18/94";
>  #include <paths.h>
>  #include <stdarg.h>
>  #include <stdlib.h>
> -#include <stdio.h>
> -#include <stdio_ext.h>
>  #include <sys/socket.h>
> -#include <sys/uio.h>
>  #include <sys/un.h>
>  #include <syslog.h>
>  #include <limits.h>
> +#include <printf_buffer.h>
>  
>  static int LogType = SOCK_DGRAM;	/* type of socket connection */
>  static int LogFile = -1;		/* fd for log */
> @@ -60,27 +58,23 @@ __libc_lock_define_initialized (static, syslog_lock)
>  static void openlog_internal (const char *, int, int);
>  static void closelog_internal (void);
>  
> -struct cleanup_arg
> +/* Unlock and deallocate the buffer.  */
> +static void
> +cancel_handler_buf (void *ptr)
>  {
> -  void *buf;
> -  struct sigaction *oldaction;
> -};
> +  __libc_lock_unlock (syslog_lock);
> +
> +  struct __printf_buffer_asprintf *buf = ptr;
> +  __printf_buffer_asprintf_free (buf);
> +}
>  
> +/* Only unlock.  */
>  static void
>  cancel_handler (void *ptr)
>  {
> -  /* Restore the old signal handler.  */
> -  struct cleanup_arg *clarg = (struct cleanup_arg *) ptr;
> -
> -  if (clarg != NULL)
> -    /* Free the memstream buffer,  */
> -    free (clarg->buf);
> -
> -  /* Free the lock.  */
>    __libc_lock_unlock (syslog_lock);
>  }
>  
> -
>  /*
>   * syslog, vsyslog --
>   *	print message on log file; output is intended for syslogd(8).
> @@ -122,16 +116,21 @@ __vsyslog_chk (int pri, int flag, const char *fmt, va_list ap)
>    __vsyslog_internal (pri, fmt, ap, (flag > 0) ? PRINTF_FORTIFY : 0);
>  }
>  
> +/* Send the buffer contents over the syslog socket.  */
> +static ssize_t
> +__syslog_send (struct __printf_buffer *buf)
> +{
> +  /* Send the trailing NUL byte for LogType == SOCK_STREAM  only.  */
> +  return __send (LogFile, buf->write_base,
> +		 buf->write_ptr - buf->write_base - (LogType != SOCK_STREAM),
> +		 MSG_NOSIGNAL)

Maybe encapsulate this on a function (__printf_buffer_write_len or
something)? I think we should avoid mess the internal printf buffer
state where possible.

> +}
> +
>  void
>  __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  		    unsigned int mode_flags)
>  {
> -  /* Try to use a static buffer as an optimization.  */
> -  char bufs[1024];
> -  char *buf = bufs;
> -  size_t bufsize;
> -
> -  int msgoff;
> +  ptrdiff_t msgoff;
>    int saved_errno = errno;
>  
>  #define	INTERNALLOG LOG_ERR|LOG_CONS|LOG_PERROR|LOG_PID
> @@ -144,8 +143,9 @@ __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 = { NULL, NULL };
> -  __libc_cleanup_push (cancel_handler, &clarg);
> +  struct __printf_buffer_asprintf buf;
> +  __printf_buffer_asprintf_init (&buf);
> +  __libc_cleanup_push (cancel_handler_buf, &buf);
>    __libc_lock_lock (syslog_lock);
>  
>    /* Check priority against setlogmask values. */
> @@ -173,122 +173,56 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>      __strftime_l (timestamp, sizeof timestamp, "%h %e %T ", now_tmp,
>  		  _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)
> +  __printf_buffer (&buf.base, "<%d>", pri);
>  
> -#define SYSLOG_HEADER_WITHOUT_TS(__pri, __msgoff)        \
> -  "<%d>: %n", __pri, __msgoff
> -
> -  int l, vl;
>    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 < 0)
> -    goto out;
> -
> -  char *pos;
> -  size_t len;
> -
> -  if (l < sizeof bufs)
>      {
> -      /* At this point, there is still a chance that we can print the
> -         remaining part of the log into bufs and use that.  */
> -      pos = bufs + l;
> -      len = sizeof (bufs) - l;
> +      __printf_buffer_puts (&buf.base, timestamp);
> +      msgoff = buf.base.write_ptr - buf.base.write_base;
> +       __printf_buffer_puts (&buf.base, LogTag == NULL ? __progname : LogTag);
> +      if (pid != 0)
> +	__printf_buffer (&buf.base, "[%d]", pid);
> +      __printf_buffer_putc (&buf.base, ':');
> +      __printf_buffer_putc (&buf.base, ' ');
>      }
>    else
>      {
> -      buf = NULL;
> -      /* We already know that bufs is too small to use for this log message.
> -         The next vsnprintf into bufs is used only to calculate the total
> -         required buffer length.  We will discard bufs contents and allocate
> -         an appropriately sized buffer later instead.  */
> -      pos = bufs;
> -      len = sizeof (bufs);
> +      msgoff = buf.base.write_ptr - buf.base.write_base;
> +      __printf_buffer_putc (&buf.base, ':');
> +      __printf_buffer_putc (&buf.base, ' ');
>      }

Maybe move the ': ' appending after the if/else:

  if (has_ts)
    {
      [...]
    }
  else
    [...]
  __printf_buffer_puts (&buf.base, ": ");

>  
> -  {
> -    va_list apc;
> -    va_copy (apc, ap);
> -
> -    /* Restore errno for %m format.  */
> -    __set_errno (saved_errno);
> -
> -    vl = __vsnprintf_internal (pos, len, fmt, apc, mode_flags);
> -    va_end (apc);
> +  /* Do not count the bytes written to the buffer so far.  This value
> +     is negative and accounts for the existing buffer contents, which
> +     is not upposed to be visible to the caller.  */

s/upposed/supposed

> +  buf.base.written = buf.base.write_base - buf.base.write_ptr;

Maybe encapsulate this on a function (__printf_buffer_reset_pos or
something)?

>  
> -    if (vl < 0 || vl >= INT_MAX - l)
> -      goto out;
> +  errno = saved_errno;
> +  __vprintf_buffer (&buf.base, fmt, ap, mode_flags);
>  
> -    if (vl >= len)
> -      buf = NULL;
> -
> -    bufsize = l + vl;
> -  }
> -
> -  if (buf == NULL)
> -    {
> -      buf = malloc ((bufsize + 1) * sizeof (char));
> -      if (buf != NULL)
> -	{
> -	  /* Tell the cancellation handler to free this buffer.  */
> -	  clarg.buf = buf;
> +  /* We may need the terminator if we are sending over a SOCK_STREAM
> +     connection.  Add this unconditionally to simplify error handling.  */
> +  __printf_buffer_putc (&buf.base, '\0');
>  

Ok.

> -	  int cl;
> -	  if (has_ts)
> -	    cl = __snprintf (buf, l + 1,
> -			     SYSLOG_HEADER (pri, timestamp, &msgoff, pid));
> -	  else
> -	    cl = __snprintf (buf, l + 1,
> -			     SYSLOG_HEADER_WITHOUT_TS (pri, &msgoff));
> -	  if (cl != l)
> -	    goto out;
> -
> -	  va_list apc;
> -	  va_copy (apc, ap);
> -	  cl = __vsnprintf_internal (buf + l, bufsize - l + 1, fmt, apc,
> -				     mode_flags);
> -	  va_end (apc);
> -
> -	  if (cl != vl)
> -	    goto out;
> -	}
> -      else
> -        {
> -          int bl;
> -	  /* Nothing much to do but emit an error message.  */
> -          bl = __snprintf (bufs, sizeof bufs,
> -                           "out of memory[%d]", __getpid ());
> -          if (bl < 0 || bl >= sizeof bufs)
> -            goto out;
> -
> -          bufsize = bl;
> -          buf = bufs;
> -          msgoff = 0;
> -        }
> -    }
> +  if (__printf_buffer_has_failed (&buf.base))
> +    goto out;
>  
>    /* Output to stderr if requested. */
>    if (LogStat & LOG_PERROR)
> -    __dprintf (STDERR_FILENO, "%s%s", buf + msgoff,
> -	       "\n" + (buf[bufsize - 1] == '\n'));
> +    {
> +      /* The buffer always starts with '<' and a digit, so there are
> +	 at least two bytes, and -2 is valid offset from the next
> +	 to-be-written byte.  -2 accounts for the NUL byte.  */
> +      int has_nl = buf.base.write_ptr[-2] == '\n';
> +      __dprintf (STDERR_FILENO, "%s%s", buf.base.write_base + msgoff,
> +		 "\n" + has_nl);
> +    }
>  
>    /* 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 || __syslog_send (&buf.base) < 0)
>      {
>        if (connected)
>  	{
> @@ -297,7 +231,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  	  openlog_internal (NULL, LogStat | LOG_NDELAY, LogFacility);
>  	}
>  
> -      if (!connected || __send (LogFile, buf, bufsize, MSG_NOSIGNAL) < 0)
> +      if (!connected || __syslog_send (&buf.base) < 0)
>  	{
>  	  closelog_internal ();	/* attempt re-open next time */
>  	  /*
> @@ -311,7 +245,7 @@ __vsyslog_internal (int pri, const char *fmt, va_list ap,
>  	      (fd = __open (_PATH_CONSOLE, O_WRONLY | O_NOCTTY
>  			    | O_CLOEXEC, 0)) >= 0)
>  	    {
> -	      __dprintf (fd, "%s\r\n", buf + msgoff);
> +	      __dprintf (fd, "%s\r\n", buf.base.write_base + msgoff);
>  	      __close (fd);
>  	    }
>  	}
> @@ -321,9 +255,7 @@ out:
>    /* End of critical section.  */
>    __libc_cleanup_pop (0);
>    __libc_lock_unlock (syslog_lock);
> -
> -  if (buf != bufs)
> -    free (buf);
> +  __printf_buffer_asprintf_free (&buf);
>  }
>  
>  /* AF_UNIX address of local logger  */