[PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

Patterns like %*[ can safely be used to match a great many characters,
and it's quite realisitic to use them for more than INT_MAX characters
from an IO stream.

With the previous approach, after INT_MAX characters (v)fscanf would
return successfully, indicating an end to the match, even though there
wasn't one.
---

I have not done a copyright assignment yet, but I think this change
should be small enough to be exempt?

 stdio-common/vfscanf-internal.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
index 38e74776a5..1d81e16f4e 100644
--- a/stdio-common/vfscanf-internal.c
+++ b/stdio-common/vfscanf-internal.c
@@ -2479,11 +2479,6 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 	  else
 	    not_in = 0;
 
-	  if (width < 0)
-	    /* There is no width given so there is also no limit on the
-	       number of characters we read.  Therefore we set width to
-	       a very high value to make the algorithm easier.  */
-	    width = INT_MAX;
 
 #ifdef COMPILE_WSCANF
 	  /* Find the beginning and the end of the scanlist.  We are not
@@ -2647,7 +2642,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 			}
 		    }
 		}
-	      while (--width > 0 && inchar () != WEOF);
+	      while ((width < 0 || --width > 0) && inchar () != WEOF);
 	    out:
 #else
 	      char buf[MB_LEN_MAX];
@@ -2732,7 +2727,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 			}
 		    }
 
-		  if (--width <= 0)
+		  if (width >= 0 && --width <= 0)
 		    break;
 		}
 	      while (inchar () != EOF);
@@ -2884,7 +2879,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 		  assert (n <= MB_LEN_MAX);
 		  str += n;
 		}
-	      while (--width > 0 && inchar () != WEOF);
+	      while ((width < 0 || --width > 0) && inchar () != WEOF);
 	    out2:
 #else
 	      do
@@ -2938,7 +2933,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 			}
 		    }
 		}
-	      while (--width > 0 && inchar () != EOF);
+	      while ((width < 0 || --width > 0) && inchar () != EOF);
 #endif
 
 	      if (__glibc_unlikely (now == read_in))
-- 
2.30.0

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Florian Weimer]

* Alyssa Ross:

> I have not done a copyright assignment yet, but I think this change
> should be small enough to be exempt?

Yes, I think it's small enough.

The test case wouldn't be, though.  I think the one on the bug needs
some large (infinite) input on the stdin, though.  A real test case
for glibc should probably involve pipe, fork, and fdopen.  fopencookie
could work, too.

>  stdio-common/vfscanf-internal.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
> index 38e74776a5..1d81e16f4e 100644
> --- a/stdio-common/vfscanf-internal.c
> +++ b/stdio-common/vfscanf-internal.c
> @@ -2479,11 +2479,6 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  	  else
>  	    not_in = 0;
>  
> -	  if (width < 0)
> -	    /* There is no width given so there is also no limit on the
> -	       number of characters we read.  Therefore we set width to
> -	       a very high value to make the algorithm easier.  */
> -	    width = INT_MAX;
>  
>  #ifdef COMPILE_WSCANF
>  	  /* Find the beginning and the end of the scanlist.  We are not
> @@ -2647,7 +2642,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  			}
>  		    }
>  		}
> -	      while (--width > 0 && inchar () != WEOF);
> +	      while ((width < 0 || --width > 0) && inchar () != WEOF);
>  	    out:
>  #else
>  	      char buf[MB_LEN_MAX];
> @@ -2732,7 +2727,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  			}
>  		    }
>  
> -		  if (--width <= 0)
> +		  if (width >= 0 && --width <= 0)
>  		    break;
>  		}
>  	      while (inchar () != EOF);
> @@ -2884,7 +2879,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  		  assert (n <= MB_LEN_MAX);
>  		  str += n;
>  		}
> -	      while (--width > 0 && inchar () != WEOF);
> +	      while ((width < 0 || --width > 0) && inchar () != WEOF);
>  	    out2:
>  #else
>  	      do
> @@ -2938,7 +2933,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  			}
>  		    }
>  		}
> -	      while (--width > 0 && inchar () != EOF);
> +	      while ((width < 0 || --width > 0) && inchar () != EOF);
>  #endif
>  
>  	      if (__glibc_unlikely (now == read_in))

So I tried to review this.  -U100 helped.  I was worried about width
starting out as positive and going negative.  But as far as I can
tell, processing stops once width == 0, so the issue cannot happen.

Do you want to work on the test case?  Will the copyright assignment
be an obstacle?

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 3367 bytes --]

>> I have not done a copyright assignment yet, but I think this change
>> should be small enough to be exempt?
>
> Yes, I think it's small enough.
>
> The test case wouldn't be, though.  I think the one on the bug needs
> some large (infinite) input on the stdin, though.  A real test case
> for glibc should probably involve pipe, fork, and fdopen.  fopencookie
> could work, too.

Oh, thanks for telling me about fopencookie!  I'd never have known about
that otherwise.  I've started having a go at a test case using it and it
seems like it'll work well.

>>  stdio-common/vfscanf-internal.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c
>> index 38e74776a5..1d81e16f4e 100644
>> --- a/stdio-common/vfscanf-internal.c
>> +++ b/stdio-common/vfscanf-internal.c
>> @@ -2479,11 +2479,6 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  	  else
>>  	    not_in = 0;
>>  
>> -	  if (width < 0)
>> -	    /* There is no width given so there is also no limit on the
>> -	       number of characters we read.  Therefore we set width to
>> -	       a very high value to make the algorithm easier.  */
>> -	    width = INT_MAX;
>>  
>>  #ifdef COMPILE_WSCANF
>>  	  /* Find the beginning and the end of the scanlist.  We are not
>> @@ -2647,7 +2642,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  			}
>>  		    }
>>  		}
>> -	      while (--width > 0 && inchar () != WEOF);
>> +	      while ((width < 0 || --width > 0) && inchar () != WEOF);
>>  	    out:
>>  #else
>>  	      char buf[MB_LEN_MAX];
>> @@ -2732,7 +2727,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  			}
>>  		    }
>>  
>> -		  if (--width <= 0)
>> +		  if (width >= 0 && --width <= 0)
>>  		    break;
>>  		}
>>  	      while (inchar () != EOF);
>> @@ -2884,7 +2879,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  		  assert (n <= MB_LEN_MAX);
>>  		  str += n;
>>  		}
>> -	      while (--width > 0 && inchar () != WEOF);
>> +	      while ((width < 0 || --width > 0) && inchar () != WEOF);
>>  	    out2:
>>  #else
>>  	      do
>> @@ -2938,7 +2933,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>>  			}
>>  		    }
>>  		}
>> -	      while (--width > 0 && inchar () != EOF);
>> +	      while ((width < 0 || --width > 0) && inchar () != EOF);
>>  #endif
>>  
>>  	      if (__glibc_unlikely (now == read_in))
>
> So I tried to review this.  -U100 helped.  I was worried about width
> starting out as positive and going negative.  But as far as I can
> tell, processing stops once width == 0, so the issue cannot happen.

That's my understanding too.

> Do you want to work on the test case?  Will the copyright assignment
> be an obstacle?

I'm happy to have a go at a test case.  I think I have most of one
already.  Copyright assignment will only be a problem in that it'll slow
things down a bit!  I just sent one in for Emacs and I've asked about
doing one for glibc as well.

One question about the test: fscanf-ing through INT_MAX characters on a
trivial memcpy-based fopencookie stream takes 20 seconds on my
(admittedly fairly old) machine.  How slow is too slow for a test?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Florian Weimer]

* Alyssa Ross:

>>> I have not done a copyright assignment yet, but I think this change
>>> should be small enough to be exempt?
>>
>> Yes, I think it's small enough.
>>
>> The test case wouldn't be, though.  I think the one on the bug needs
>> some large (infinite) input on the stdin, though.  A real test case
>> for glibc should probably involve pipe, fork, and fdopen.  fopencookie
>> could work, too.
>
> Oh, thanks for telling me about fopencookie!  I'd never have known about
> that otherwise.  I've started having a go at a test case using it and it
> seems like it'll work well.

Or you could test sscanf via <support/blob_repeat.h> on 64-bit
architectures.  It would avoid the repeated memcpy calls, at the cost
of an initial strlen on the entire buffer.

> One question about the test: fscanf-ing through INT_MAX characters on a
> trivial memcpy-based fopencookie stream takes 20 seconds on my
> (admittedly fairly old) machine.  How slow is too slow for a test?

Opinions on that vary.  Twenty seconds is stretching things as far as
I'm concerned.  I guess it depends what you mean by “fairly old”.

We have a second category of tests, xtests, that only run with “make
xcheck”.  We could put the test there if it takes too long to run
otherwise.

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

Florian Weimer <fw@deneb.enyo.de> writes:

>> Oh, thanks for telling me about fopencookie!  I'd never have known about
>> that otherwise.  I've started having a go at a test case using it and it
>> seems like it'll work well.
>
> Or you could test sscanf via <support/blob_repeat.h> on 64-bit
> architectures.  It would avoid the repeated memcpy calls, at the cost
> of an initial strlen on the entire buffer.

Would that be a problem on 32-bit?  We'd only need to map
INT_MAX bytes + 1 page, so the other half of the address space would be
enough for everything else, wouldn't it?

>> One question about the test: fscanf-ing through INT_MAX characters on a
>> trivial memcpy-based fopencookie stream takes 20 seconds on my
>> (admittedly fairly old) machine.  How slow is too slow for a test?
>
> Opinions on that vary.  Twenty seconds is stretching things as far as
> I'm concerned.  I guess it depends what you mean by “fairly old”.
>
> We have a second category of tests, xtests, that only run with “make
> xcheck”.  We could put the test there if it takes too long to run
> otherwise.

That machine is a laptop from 2012.  On my other laptop, from 2017, it
takes <5 seconds, and the fopencookie and blob_repeat versions aren't
really distinguishable from each other in terms of time taken.  I would
like to go with the blob_repeat version if it'll work everywhere,
though, because not having to write a custom stream implementation makes
the test a lot shorter and easier to understand.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Florian Weimer]

* Alyssa Ross:

> Florian Weimer <fw@deneb.enyo.de> writes:
>
>>> Oh, thanks for telling me about fopencookie!  I'd never have known about
>>> that otherwise.  I've started having a go at a test case using it and it
>>> seems like it'll work well.
>>
>> Or you could test sscanf via <support/blob_repeat.h> on 64-bit
>> architectures.  It would avoid the repeated memcpy calls, at the cost
>> of an initial strlen on the entire buffer.
>
> Would that be a problem on 32-bit?  We'd only need to map
> INT_MAX bytes + 1 page, so the other half of the address space would be
> enough for everything else, wouldn't it?

True, it would work on most 32-bit targets, particularly when running
on 64-bit kernels, where more address space is available to userspace.
s390 (without the x) has just 31-bit addresses, so it won't be able to
run the test, but that's fine, given that it's an architecture-independent
issue.

(blob_repeat sets up page aliasing behind the scenes, so the actual
memory requirements are quite limited, it's just address space that
could be problematic.)

>>> One question about the test: fscanf-ing through INT_MAX characters on a
>>> trivial memcpy-based fopencookie stream takes 20 seconds on my
>>> (admittedly fairly old) machine.  How slow is too slow for a test?
>>
>> Opinions on that vary.  Twenty seconds is stretching things as far as
>> I'm concerned.  I guess it depends what you mean by “fairly old”.
>>
>> We have a second category of tests, xtests, that only run with “make
>> xcheck”.  We could put the test there if it takes too long to run
>> otherwise.
>
> That machine is a laptop from 2012.  On my other laptop, from 2017, it
> takes <5 seconds, and the fopencookie and blob_repeat versions aren't
> really distinguishable from each other in terms of time taken.  I would
> like to go with the blob_repeat version if it'll work everywhere,
> though, because not having to write a custom stream implementation makes
> the test a lot shorter and easier to understand.

Sounds good to me.  Less than five seconds is fine, I think.

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Thu, Mar 25, 2021 at 06:25:17PM +0100, Florian Weimer wrote:
> Do you want to work on the test case?  Will the copyright assignment
> be an obstacle?

Okay, I've written a test case (thanks for all your help).  Do I need to
seperately test swscanf and friends, since their implementation is
seperate?  And should I post the revised patch with the test case as
soon as it's ready, or should I wait until I have my copyright paperwork
sorted?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Florian Weimer]

* Alyssa Ross:

> On Thu, Mar 25, 2021 at 06:25:17PM +0100, Florian Weimer wrote:
>> Do you want to work on the test case?  Will the copyright assignment
>> be an obstacle?
>
> Okay, I've written a test case (thanks for all your help).  Do I need to
> seperately test swscanf and friends, since their implementation is
> seperate?  And should I post the revised patch with the test case as
> soon as it's ready, or should I wait until I have my copyright paperwork
> sorted?

I think we can apply the fix now and the test case later.  You can
post the test case now, we can mark it as FSF Assignment Missing in
Patchwork.

[PATCH 2/2] stdio: add test for scanf matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

This doesn't test the wchar_t variants.  Won't run on s390 due to
using more than INT_MAX bytes of address space.
---
Applying this will need to wait until I have my copyright paperwork
sorted.

 stdio-common/Makefile             |  3 ++-
 stdio-common/tst-sscanf-bz27650.c | 42 +++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 stdio-common/tst-sscanf-bz27650.c

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index b2458ba4a6..281a154f5b 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -70,7 +70,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
 	 tst-vfprintf-width-prec-alloc \
 	 tst-printf-fp-free \
 	 tst-printf-fp-leak \
-	 test-strerr
+	 test-strerr \
+	 tst-sscanf-bz27650
 
 
 test-srcs = tst-unbputc tst-printf tst-printfsz-islongdouble
diff --git a/stdio-common/tst-sscanf-bz27650.c b/stdio-common/tst-sscanf-bz27650.c
new file mode 100644
index 0000000000..4636d26323
--- /dev/null
+++ b/stdio-common/tst-sscanf-bz27650.c
@@ -0,0 +1,42 @@
+#include <errno.h>
+#include <limits.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <support/blob_repeat.h>
+#include <support/check.h>
+
+/* Test that vfscanf can handle matches longer than INT_MAX bytes. */
+static int
+do_test (void)
+{
+  char next_char;
+  struct support_blob_repeat blob =
+    support_blob_repeat_allocate ("a", 1, INT_MAX + 3U);
+  char *s = blob.start;
+
+  if (s == NULL)
+    FAIL_UNSUPPORTED ("repeated blob allocation failed: %m");
+
+  s[INT_MAX + 1U] = 'b';
+  s[INT_MAX + 2U] = '\0';
+
+  errno = 0;
+  switch (sscanf (s, "%*[^b]%c", &next_char))
+    {
+    case EOF:
+      if (errno)
+	FAIL_EXIT1 ("sscanf: %m");
+      FAIL_EXIT1 ("EOF reached and no 'b' found");
+    case 0:
+      FAIL_EXIT1 ("pattern didn't match");
+    }
+
+  TEST_COMPARE (next_char, 'b');
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.0

Re: [PATCH 2/2] stdio: add test for scanf matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On Mon, Mar 29, 2021 at 06:06:40PM +0000, Alyssa Ross wrote:
> This doesn't test the wchar_t variants.  Won't run on s390 due to
> using more than INT_MAX bytes of address space.
> ---
> Applying this will need to wait until I have my copyright paperwork
> sorted.
>
>  stdio-common/Makefile             |  3 ++-
>  stdio-common/tst-sscanf-bz27650.c | 42 +++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 stdio-common/tst-sscanf-bz27650.c
>
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index b2458ba4a6..281a154f5b 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -70,7 +70,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
>  	 tst-vfprintf-width-prec-alloc \
>  	 tst-printf-fp-free \
>  	 tst-printf-fp-leak \
> -	 test-strerr
> +	 test-strerr \
> +	 tst-sscanf-bz27650
>
>
>  test-srcs = tst-unbputc tst-printf tst-printfsz-islongdouble
> diff --git a/stdio-common/tst-sscanf-bz27650.c b/stdio-common/tst-sscanf-bz27650.c
> new file mode 100644
> index 0000000000..4636d26323
> --- /dev/null
> +++ b/stdio-common/tst-sscanf-bz27650.c
> @@ -0,0 +1,42 @@
> +#include <errno.h>
> +#include <limits.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>

Just realised not all these headers are necessary (some were left in
from a previous approach with this test):

diff --git i/stdio-common/tst-sscanf-bz27650.c w/stdio-common/tst-sscanf-bz27650.c
index 4636d26323..c7dc8c814d 100644
--- i/stdio-common/tst-sscanf-bz27650.c
+++ w/stdio-common/tst-sscanf-bz27650.c
@@ -1,9 +1,6 @@
 #include <errno.h>
 #include <limits.h>
-#include <stddef.h>
 #include <stdio.h>
-#include <stdlib.h>
-#include <string.h>

 #include <support/blob_repeat.h>
 #include <support/check.h>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Florian Weimer]

* Alyssa Ross:

> Patterns like %*[ can safely be used to match a great many characters,
> and it's quite realisitic to use them for more than INT_MAX characters
> from an IO stream.
>
> With the previous approach, after INT_MAX characters (v)fscanf would
> return successfully, indicating an end to the match, even though there
> wasn't one.

I've pushed to this commit, thanks.

We can integrate the test once your copyright assignment is on file.

Thanks,
Florian

Re: [PATCH] stdio: fix vfscanf with matches longer than INT_MAX (bug 27650)

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Mon, May 03, 2021 at 10:57:12AM +0200, Florian Weimer wrote:
> * Alyssa Ross:
>
> > Patterns like %*[ can safely be used to match a great many characters,
> > and it's quite realisitic to use them for more than INT_MAX characters
> > from an IO stream.
> >
> > With the previous approach, after INT_MAX characters (v)fscanf would
> > return successfully, indicating an end to the match, even though there
> > wasn't one.
>
> I've pushed to this commit, thanks.
>
> We can integrate the test once your copyright assignment is on file.

My copyright assignment should be sorted now (as of May 6).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]