stdio-common: Add test for vfscanf with matches longer than INT_MAX [BZ #27650]

  Complement commit b03e4d7bd25b ("stdio: fix vfscanf with matches longer 
than INT_MAX (bug 27650)") and add a test case for the issue, inspired 
by the reproducer provided with the bug report.

This has been verified to succeed as from the commit referred and fail 
beforehand.

As the test requires 2GiB of data to be passed around its performance 
has been evaluated using a choice of systems and the execution time 
determined to be respectively in the range of 10s for POWER9@2.166GHz, 
28s for FU740@1.2GHz, and 48s for 74Kf@950MHz.  As this is on the verge 
of and beyond the default timeout it has been increased by the factor of 
8.  Regardless, following recent practice the test has been added to the 
standard rather than extended set.
---
Hi,

 This has been verified with the `powerpc64le-linux-gnu' (IBM POWER9) 
native target and then the same host and the `riscv64-linux-gnu' (SiFive 
FU740) and `mips-linux-gnu' (o32 ABI) (MIPS 74Kf) targets.  This is so as 
to assess performance requirements for the test case.  And with respect to 
these I have been referred off list to: 
<https://inbox.sourceware.org/libc-alpha/b52d05a2-f4c1-e385-39bd-cd4a6f4f232f@redhat.com/>
for the choice between the standard and the extended set of tests.

 Any questions, comments or concerns?  Otherwise OK to apply?

  Maciej
---
 stdio-common/Makefile            |    2 
 stdio-common/tst-scanf-bz27650.c |  216 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 218 insertions(+)

glibc-tst-scanf-bz27650.diff
  

* Maciej W. Rozycki:

> +/* Produce a stream of more than MAX_INT characters to stdout of which
> +   none is the new line character.  This is executed as a subprocess
> +   and the caller wants a void callee, upon the return from which the
> +   process will terminate successfully, so in the case of a failure we
> +   need to explicitly call exit with the failure status.  */
> +
> +static void
> +do_write (void *arg)
> +{
> +  static const char s[] = { [0 ... 4095] = 'a' };
> +  size_t i;
> +
> +  for (i = 0; i <= INT_MAX / sizeof (s); i++)
> +    if (fwrite (s, 1, sizeof (s), stdout) != sizeof (s))
> +      {
> +	int err = errno;
> +
> +	/* Close our stdout so that there's no risk for us to block
> +	   while `fscanf' is waiting on our stdout in `do_read' and
> +	   nothing checking our stderr.  If closing has failed, then
> +	   refrain from reporting anything, for the same reason.  */
> +	if (fclose (stdout) == 0)
> +	  error (0, err, "%s: fwrite: output error", __func__);
> +	exit (EXIT_FAILURE);
> +      }

Is there a reason for using stdout or stream I/O for writing the file?
You could use create_temp_file from <support/temp_file.h> and write
directly to the file descriptor.

It may be faster to use <support/blob_repeat.h> to create a string in
memory because it uses alias mappings, and parse that string using
sscanf or fmemopen+vfscanf.

Thanks,
Florian
  

On Fri, 7 Jun 2024, Florian Weimer wrote:

> * Maciej W. Rozycki:
> 
> > +/* Produce a stream of more than MAX_INT characters to stdout of which
> > +   none is the new line character.  This is executed as a subprocess
> > +   and the caller wants a void callee, upon the return from which the
> > +   process will terminate successfully, so in the case of a failure we
> > +   need to explicitly call exit with the failure status.  */
> > +
> > +static void
> > +do_write (void *arg)
> > +{
> > +  static const char s[] = { [0 ... 4095] = 'a' };
> > +  size_t i;
> > +
> > +  for (i = 0; i <= INT_MAX / sizeof (s); i++)
> > +    if (fwrite (s, 1, sizeof (s), stdout) != sizeof (s))
> > +      {
> > +	int err = errno;
> > +
> > +	/* Close our stdout so that there's no risk for us to block
> > +	   while `fscanf' is waiting on our stdout in `do_read' and
> > +	   nothing checking our stderr.  If closing has failed, then
> > +	   refrain from reporting anything, for the same reason.  */
> > +	if (fclose (stdout) == 0)
> > +	  error (0, err, "%s: fwrite: output error", __func__);
> > +	exit (EXIT_FAILURE);
> > +      }
> 
> Is there a reason for using stdout or stream I/O for writing the file?
> You could use create_temp_file from <support/temp_file.h> and write
> directly to the file descriptor.

 A pipe is used because over 2GiB of data has to be transferred.  It could 
be a bit of a stress for the target board if such a large amount was to be 
actually stored in a filesystem (even in the presence of LFS).  There may 
be limited storage available too.

 My choice to use the `fwrite' interface over raw `write' was mostly 
symmetry with the rest of code and also less hassle in handling, as with 
`fwrite' you don't have to take care of partial writes.  Besides, `stdout' 
is readily available, there's no need for an extra library call (and an 
error to handle) to extract the underlying file descriptor.  Not a big 
deal overall, just a matter of style.

> It may be faster to use <support/blob_repeat.h> to create a string in
> memory because it uses alias mappings, and parse that string using
> sscanf or fmemopen+vfscanf.

 Neat!  However with 32-bit targets the size of the allocation required 
may exceed the limit of the user VM supported by hardware or the OS (some 
targets give room for manoeuvre to the OS as to the user/kernel VM split 
while other ones have it hardwired).  As a quick check I have run 
stdlib/tst-strtod-overflow with my 74Kf target and, lo and behold:

UNSUPPORTED: stdlib/tst-strtod-overflow
original exit status 77
warning: memory allocation failed, cannot test for overflow

And I think we do want to have coverage for scanning INT_MAX+ characters 
especially with 32-bit targets, where it may be hitting more than just the 
limit of the `int' type and the specific issue reported with BZ #27650.

 Do my answers address your concerns?

  Maciej
  

On Fri, 7 Jun 2024, Maciej W. Rozycki wrote:

> Complement commit b03e4d7bd25b ("stdio: fix vfscanf with matches longer 
> than INT_MAX (bug 27650)") and add a test case for the issue, inspired 
> by the reproducer provided with the bug report.

 Ping for: 
<https://sourceware.org/pipermail/libc-alpha/2024-June/157283.html>,
<https://patchwork.sourceware.org/project/glibc/patch/7bd09b7b-bbd4-ac32-02d5-687ec5e01986@redhat.com/>.

  Maciej
  

On Fri, 7 Jun 2024, Maciej W. Rozycki wrote:

> Complement commit b03e4d7bd25b ("stdio: fix vfscanf with matches longer 
> than INT_MAX (bug 27650)") and add a test case for the issue, inspired 
> by the reproducer provided with the bug report.

 Ping for:
<https://sourceware.org/pipermail/libc-alpha/2024-June/157283.html>,
<https://patchwork.sourceware.org/project/glibc/patch/7bd09b7b-bbd4-ac32-02d5-687ec5e01986@redhat.com/>.

  Maciej
  

* Maciej W. Rozycki:

>> It may be faster to use <support/blob_repeat.h> to create a string in
>> memory because it uses alias mappings, and parse that string using
>> sscanf or fmemopen+vfscanf.
>
>  Neat!  However with 32-bit targets the size of the allocation required 
> may exceed the limit of the user VM supported by hardware or the OS (some 
> targets give room for manoeuvre to the OS as to the user/kernel VM split 
> while other ones have it hardwired).  As a quick check I have run 
> stdlib/tst-strtod-overflow with my 74Kf target and, lo and behold:
>
> UNSUPPORTED: stdlib/tst-strtod-overflow
> original exit status 77
> warning: memory allocation failed, cannot test for overflow
>
> And I think we do want to have coverage for scanning INT_MAX+ characters 
> especially with 32-bit targets, where it may be hitting more than just the 
> limit of the `int' type and the specific issue reported with BZ #27650.

Fair enough.

>  Do my answers address your concerns?

I think the test case is still unnecessarily complicated.  You can use
fopencookie to generate the data, or at least use threads to simplify
the error handling considerably.  And please do not reuse stdout for
data generation.  Given how our test error reporting works (we write to
stdout), fclose (stdout) is a bit of a debugger trap.

Thanks,
Florian
  

On Mon, 1 Jul 2024, Florian Weimer wrote:

> >  Do my answers address your concerns?
> 
> I think the test case is still unnecessarily complicated.  You can use
> fopencookie to generate the data, or at least use threads to simplify
> the error handling considerably.  And please do not reuse stdout for
> data generation.  Given how our test error reporting works (we write to
> stdout), fclose (stdout) is a bit of a debugger trap.

 But that `fclose (stdout)' is called in the child only, for the stream 
associated with the pipe connecting with the parent, which in turn is a 
consequence of how our own `support_subprocess' has been implemented -- 
not my choice.  This is how unnamed pipes work and this stream or the 
underlying pipe do not communicate with the test harness and neither have 
anything to do with the parent's stdout (and likewise stderr).

 And I don't think using threads would make the test case simpler.  In 
fact threading is complex by itself, so making the test case threaded 
would only complicate things.

 However the idea of using `fopencookie' (what a weird name!), which is a 
GNU extension I have not come across before, seems attractive to me.  It 
will remove the need to use a subprocess, likely improve performance a 
little, as there'll be no need to cross the kernel boundary anymore, and 
will greatly simplify handling overall.  Thanks for the suggestion, I'll 
be posting v2 shortly.

 Thank you for your review.

  Maciej