diff mbox

vfscanf: Avoid multiple reads of multi-byte character width

Message ID 20160902131953.69ABB40191DBC@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer Sept. 2, 2016, 1:19 p.m. UTC 
This avoids a race condition if the process-global locale is changed
while vfscanf is running.  MB_LEN_MAX is always larger than MB_CUR_MAX,
so we might realloc earlier than necessary (but even MB_CUR_MAX could
be larger than the minimum required space).

The existing length was a bit questionable because str + MB_LEN_MAX
might point past the end of the buffer.

2016-09-02  Florian Weimer  <fweimer@redhat.com>

	* stdio-common/vfscanf.c (_IO_vfwscanf): Use MB_LEN_MAX instead of
	MB_CUR_MAX to avoid race condition.  Avoid pointer arithmetic
	outside of allocated array.

Comments

Andreas Schwab Sept. 2, 2016, 1:56 p.m. UTC | #1 
On Sep 02 2016, fweimer@redhat.com (Florian Weimer) wrote:

> diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
> index 8cd5955..2b7093e 100644
> --- a/stdio-common/vfscanf.c
> +++ b/stdio-common/vfscanf.c
> @@ -757,7 +757,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
>  		  size_t n;
>  
>  		  if (!(flags & SUPPRESS) && (flags & POSIX_MALLOC)
> -		      && str + MB_CUR_MAX >= *strptr + strsize)
> +		      && MB_LEN_MAX >= *strptr + strsize - str)

Please reorder the condition to put the constant part on the right hand
side (also below).

Ok with that change.

Andreas.
Florian Weimer Sept. 2, 2016, 2:15 p.m. UTC | #2 
On 09/02/2016 03:56 PM, Andreas Schwab wrote:
> On Sep 02 2016, fweimer@redhat.com (Florian Weimer) wrote:
>
>> diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
>> index 8cd5955..2b7093e 100644
>> --- a/stdio-common/vfscanf.c
>> +++ b/stdio-common/vfscanf.c
>> @@ -757,7 +757,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
>>  		  size_t n;
>>
>>  		  if (!(flags & SUPPRESS) && (flags & POSIX_MALLOC)
>> -		      && str + MB_CUR_MAX >= *strptr + strsize)
>> +		      && MB_LEN_MAX >= *strptr + strsize - str)
>
> Please reorder the condition to put the constant part on the right hand
> side (also below).

Thanks, committed with that change.

Florian
diff mbox

Patch

diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
index 8cd5955..2b7093e 100644
--- a/stdio-common/vfscanf.c
+++ b/stdio-common/vfscanf.c
@@ -757,7 +757,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		  size_t n;
 
 		  if (!(flags & SUPPRESS) && (flags & POSIX_MALLOC)
-		      && str + MB_CUR_MAX >= *strptr + strsize)
+		      && MB_LEN_MAX >= *strptr + strsize - str)
 		    {
 		      /* We have to enlarge the buffer if the `m' flag
 			 was given.  */
@@ -769,7 +769,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			{
 			  /* Can't allocate that much.  Last-ditch effort.  */
 			  newstr = (char *) realloc (*strptr,
-						     strleng + MB_CUR_MAX);
+						     strleng + MB_LEN_MAX);
 			  if (newstr == NULL)
 			    {
 			      /* c can't have `a' flag, only `m'.  */
@@ -780,7 +780,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			    {
 			      *strptr = newstr;
 			      str = newstr + strleng;
-			      strsize = strleng + MB_CUR_MAX;
+			      strsize = strleng + MB_LEN_MAX;
 			    }
 			}
 		      else
@@ -1048,7 +1048,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		    size_t n;
 
 		    if (!(flags & SUPPRESS) && (flags & MALLOC)
-			&& str + MB_CUR_MAX >= *strptr + strsize)
+			&& MB_LEN_MAX >= *strptr + strsize - str)
 		      {
 			/* We have to enlarge the buffer if the `a' or `m'
 			   flag was given.  */
@@ -1061,7 +1061,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			    /* Can't allocate that much.  Last-ditch
 			       effort.  */
 			    newstr = (char *) realloc (*strptr,
-						       strleng + MB_CUR_MAX);
+						       strleng + MB_LEN_MAX);
 			    if (newstr == NULL)
 			      {
 				if (flags & POSIX_MALLOC)
@@ -1081,7 +1081,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			      {
 				*strptr = newstr;
 				str = newstr + strleng;
-				strsize = strleng + MB_CUR_MAX;
+				strsize = strleng + MB_LEN_MAX;
 			      }
 			  }
 			else
@@ -1097,7 +1097,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		    if (__glibc_unlikely (n == (size_t) -1))
 		      encode_error ();
 
-		    assert (n <= MB_CUR_MAX);
+		    assert (n <= MB_LEN_MAX);
 		    str += n;
 		  }
 #else
@@ -2675,7 +2675,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			  /* Possibly correct character, just not enough
 			     input.  */
 			  ++cnt;
-			  assert (cnt < MB_CUR_MAX);
+			  assert (cnt < MB_LEN_MAX);
 			  continue;
 			}
 		      cnt = 0;
@@ -2827,7 +2827,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		  if (!(flags & SUPPRESS))
 		    {
 		      if ((flags & MALLOC)
-			  && str + MB_CUR_MAX >= *strptr + strsize)
+			  && MB_LEN_MAX >= *strptr + strsize - str)
 			{
 			  /* Enlarge the buffer.  */
 			  size_t strleng = str - *strptr;
@@ -2839,7 +2839,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 			      /* Can't allocate that much.  Last-ditch
 				 effort.  */
 			      newstr = (char *) realloc (*strptr,
-							 strleng + MB_CUR_MAX);
+							 strleng + MB_LEN_MAX);
 			      if (newstr == NULL)
 				{
 				  if (flags & POSIX_MALLOC)
@@ -2859,7 +2859,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 				{
 				  *strptr = newstr;
 				  str = newstr + strleng;
-				  strsize = strleng + MB_CUR_MAX;
+				  strsize = strleng + MB_LEN_MAX;
 				}
 			    }
 			  else
@@ -2875,7 +2875,7 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		  if (__glibc_unlikely (n == (size_t) -1))
 		    encode_error ();
 
-		  assert (n <= MB_CUR_MAX);
+		  assert (n <= MB_LEN_MAX);
 		  str += n;
 		}
 	      while (--width > 0 && inchar () != WEOF);