[PATCH] libio: Update number of written bytes in dprintf implementation

[PATCH] libio: Update number of written bytes in dprintf implementation

[Florian Weimer]

The __printf_buffer_flush_dprintf function needs to record that
the buffer has been written before reusing it.  Without this
accounting, dprintf always returns zero.

Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
("libio: Convert __vdprintf_internal to buffers.

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 libio/iovdprintf.c                |  1 +
 stdio-common/Makefile             |  1 +
 stdio-common/tst-dprintf-length.c | 45 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/libio/iovdprintf.c b/libio/iovdprintf.c
index fb359d263d..d9fa886fdf 100644
--- a/libio/iovdprintf.c
+++ b/libio/iovdprintf.c
@@ -54,6 +54,7 @@ __printf_buffer_flush_dprintf (struct __printf_buffer_dprintf *buf)
 	}
       p += ret;
     }
+  buf->base.written += buf->base.write_ptr - buf->base.write_base;
   buf->base.write_ptr = buf->buf;
 }
 
diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index da3034d847..34fdd6d1f8 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -180,6 +180,7 @@ tests := \
   tst-bz11319 \
   tst-bz11319-fortify2 \
   tst-cookie \
+  tst-dprintf-length \
   tst-fdopen \
   tst-ferror \
   tst-fgets \
diff --git a/stdio-common/tst-dprintf-length.c b/stdio-common/tst-dprintf-length.c
new file mode 100644
index 0000000000..abe2caf45a
--- /dev/null
+++ b/stdio-common/tst-dprintf-length.c
@@ -0,0 +1,45 @@
+/* Test that dprintf returns the expected length.
+   Copyright (C) 2023 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 <stdio.h>
+#include <string.h>
+#include <support/check.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+static int
+do_test (void)
+{
+  /* Use a datagram socket to check that everything arrives in one packet.
+     The dprintf function should perform a single write call.  */
+  int fds[2];
+  TEST_VERIFY_EXIT (socketpair (AF_LOCAL, SOCK_DGRAM, 0, fds) == 0);
+
+  TEST_COMPARE (dprintf (fds[0], "(%d)%s[%d]", 123, "---", 4567), 14);
+
+  char buf[32];
+  ssize_t ret = read (fds[1], buf, sizeof (buf));
+  TEST_VERIFY_EXIT (ret > 0);
+  TEST_COMPARE_BLOB (buf, ret, "(123)---[4567]", strlen ("(123)---[4567]"));
+
+  close (fds[1]);
+  close (fds[0]);
+  return 0;
+}
+
+#include <support/test-driver.c>

base-commit: 2f39e44a8417b4186a7f15bfeac5d0b557e63e03

Re: [PATCH] libio: Update number of written bytes in dprintf implementation

[Andreas Schwab]

On Jan 31 2023, Florian Weimer via Libc-alpha wrote:

> The __printf_buffer_flush_dprintf function needs to record that
> the buffer has been written before reusing it.  Without this
> accounting, dprintf always returns zero.
>
> Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
> ("libio: Convert __vdprintf_internal to buffers.
                                                 ")

-- 
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] libio: Update number of written bytes in dprintf implementation

[Florian Weimer]

* Andreas Schwab:

> On Jan 31 2023, Florian Weimer via Libc-alpha wrote:
>
>> The __printf_buffer_flush_dprintf function needs to record that
>> the buffer has been written before reusing it.  Without this
>> accounting, dprintf always returns zero.
>>
>> Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
>> ("libio: Convert __vdprintf_internal to buffers.
>                                                  ")

Thanks, fixed locally:

    libio: Update number of written bytes in dprintf implementation
    
    The __printf_buffer_flush_dprintf function needs to record that
    the buffer has been written before reusing it.  Without this
    accounting, dprintf always returns zero.
    
    Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
    ("libio: Convert __vdprintf_internal to buffers").

Florian

Re: [PATCH] libio: Update number of written bytes in dprintf implementation

[Carlos O'Donell]

On 1/31/23 04:38, Florian Weimer wrote:
> The __printf_buffer_flush_dprintf function needs to record that
> the buffer has been written before reusing it.  Without this
> accounting, dprintf always returns zero.
> 
> Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
> ("libio: Convert __vdprintf_internal to buffers.

Tested that regression test catches the error without the patch applied.

Tested that the fix corrects the error and correctly adjusts written with
the number of bytes that were just written in the lines before in the flush.
For dprintf we must flush everything since there is no buffer. The code
added looks correct to me and mirrors __wprintf_buffer_flush_to_file (which
adjusts written by count), __printf_buffer_flush_to_file (which always accounts
for the bytes), __printf_buffer_flush_obstack (always counted object out bytes),
and __printf_buffer_flush_snprintf (again always counted).

Tested without regression on x86_64 and i686.

OK for 2.37, pelase commit ASAP since I consider this a release blocker bug
fix since it was reported by downstream testing in Fedora as part of the
Fedora 38 Alpha testing (using latest glibc development).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>

> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.
> 
> ---
>  libio/iovdprintf.c                |  1 +
>  stdio-common/Makefile             |  1 +
>  stdio-common/tst-dprintf-length.c | 45 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/libio/iovdprintf.c b/libio/iovdprintf.c
> index fb359d263d..d9fa886fdf 100644
> --- a/libio/iovdprintf.c
> +++ b/libio/iovdprintf.c
> @@ -54,6 +54,7 @@ __printf_buffer_flush_dprintf (struct __printf_buffer_dprintf *buf)
>  	}
>        p += ret;
>      }
> +  buf->base.written += buf->base.write_ptr - buf->base.write_base;
>    buf->base.write_ptr = buf->buf;
>  }
>  
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index da3034d847..34fdd6d1f8 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -180,6 +180,7 @@ tests := \
>    tst-bz11319 \
>    tst-bz11319-fortify2 \
>    tst-cookie \
> +  tst-dprintf-length \
>    tst-fdopen \
>    tst-ferror \
>    tst-fgets \
> diff --git a/stdio-common/tst-dprintf-length.c b/stdio-common/tst-dprintf-length.c
> new file mode 100644
> index 0000000000..abe2caf45a
> --- /dev/null
> +++ b/stdio-common/tst-dprintf-length.c
> @@ -0,0 +1,45 @@
> +/* Test that dprintf returns the expected length.
> +   Copyright (C) 2023 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 <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <sys/socket.h>
> +#include <unistd.h>
> +
> +static int
> +do_test (void)
> +{
> +  /* Use a datagram socket to check that everything arrives in one packet.
> +     The dprintf function should perform a single write call.  */
> +  int fds[2];
> +  TEST_VERIFY_EXIT (socketpair (AF_LOCAL, SOCK_DGRAM, 0, fds) == 0);
> +
> +  TEST_COMPARE (dprintf (fds[0], "(%d)%s[%d]", 123, "---", 4567), 14);
> +
> +  char buf[32];
> +  ssize_t ret = read (fds[1], buf, sizeof (buf));
> +  TEST_VERIFY_EXIT (ret > 0);
> +  TEST_COMPARE_BLOB (buf, ret, "(123)---[4567]", strlen ("(123)---[4567]"));
> +
> +  close (fds[1]);
> +  close (fds[0]);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 
> base-commit: 2f39e44a8417b4186a7f15bfeac5d0b557e63e03
> 

-- 
Cheers,
Carlos.

Re: [PATCH] libio: Update number of written bytes in dprintf implementation

[Florian Weimer]

* Carlos O'Donell:

> On 1/31/23 04:38, Florian Weimer wrote:
>> The __printf_buffer_flush_dprintf function needs to record that
>> the buffer has been written before reusing it.  Without this
>> accounting, dprintf always returns zero.
>> 
>> Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600
>> ("libio: Convert __vdprintf_internal to buffers.
>
> Tested that regression test catches the error without the patch applied.
>
> Tested that the fix corrects the error and correctly adjusts written with
> the number of bytes that were just written in the lines before in the flush.
> For dprintf we must flush everything since there is no buffer. The code
> added looks correct to me and mirrors __wprintf_buffer_flush_to_file (which
> adjusts written by count), __printf_buffer_flush_to_file (which always accounts
> for the bytes), __printf_buffer_flush_obstack (always counted object out bytes),
> and __printf_buffer_flush_snprintf (again always counted).
>
> Tested without regression on x86_64 and i686.
>
> OK for 2.37, pelase commit ASAP since I consider this a release blocker bug
> fix since it was reported by downstream testing in Fedora as part of the
> Fedora 38 Alpha testing (using latest glibc development).
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Tested-by: Carlos O'Donell <carlos@redhat.com>

Thanks, pushed.

Florian