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

[Carlos O'Donell <carlos@redhat.com>]

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.