correct buffer end pointer in IO_wdefault_doallocate (BZ #26874)

  An experimental build of GCC 11 with an enhanced -Warray-bounds
reports a bug in IO_wdefault_doallocate where the function forms
an invalid past-the-end pointer to an allocated wchar_t buffer
by failingf to consider the scaling by sizeof (wchar_t).

The fix path below corrects this problem.  It keeps the buffer
size the same as opposed to increasing it according to what other
code like it does.

Since the bug looks like it might be exploitable I tried to create
a test case to trigger a call to _IO_wdefault_doallocate but couldn't.
No test in the test suite seems to either, so I post this patch without
one.

Martin
  

On 1/2/21 4:04 AM, Martin Sebor via Libc-alpha wrote:
> An experimental build of GCC 11 with an enhanced -Warray-bounds
> reports a bug in IO_wdefault_doallocate where the function forms
> an invalid past-the-end pointer to an allocated wchar_t buffer
> by failingf to consider the scaling by sizeof (wchar_t).
> 
> The fix path below corrects this problem.  It keeps the buffer
> size the same as opposed to increasing it according to what other
> code like it does.
> 
> Since the bug looks like it might be exploitable I tried to create
> a test case to trigger a call to _IO_wdefault_doallocate but couldn't.
> No test in the test suite seems to either, so I post this patch without
> one.

I spent some time following those vtable setups and I don't think 
_IO_wdefault_doallocate gets called at all.  The only instances it is 
set as the doallocate callback are in strops, wmemstream and in 
vswprintf.  In all those cases, the backing buffer is either user 
supplied or is sized without using doallocate (as in the case of strops 
overflow).

The only time libio needs to allocate buffers in the wide context is in 
wfileops when it's actually using the underlying buffer.  In that case 
however, the doallocate callback is different.

Nevertheless, this is a good cleanup, so LGTM.

Thanks,
Siddhesh

> Martin
> 
> diff --git a/libio/wgenops.c b/libio/wgenops.c
> index 0a242d93ca..153b1da8dc 100644
> --- a/libio/wgenops.c
> +++ b/libio/wgenops.c
> @@ -379,12 +379,11 @@ libc_hidden_def (_IO_wdoallocbuf)
>   int
>   _IO_wdefault_doallocate (FILE *fp)
>   {
> -  wchar_t *buf;
> -
> -  buf = malloc (BUFSIZ);
> +  wchar_t *buf = (wchar_t *)malloc (BUFSIZ);
>     if (__glibc_unlikely (buf == NULL))
>       return EOF;
> -  _IO_wsetb (fp, buf, buf + BUFSIZ, 1);
> +
> +  _IO_wsetb (fp, buf, buf + BUFSIZ / sizeof *buf, 1);
>     return 1;
>   }
>   libc_hidden_def (_IO_wdefault_doallocate)
>
  

On 2/4/21 3:17 PM, Siddhesh Poyarekar wrote:
> On 1/2/21 4:04 AM, Martin Sebor via Libc-alpha wrote:
>> An experimental build of GCC 11 with an enhanced -Warray-bounds
>> reports a bug in IO_wdefault_doallocate where the function forms
>> an invalid past-the-end pointer to an allocated wchar_t buffer
>> by failingf to consider the scaling by sizeof (wchar_t).
>>
>> The fix path below corrects this problem.  It keeps the buffer
>> size the same as opposed to increasing it according to what other
>> code like it does.
>>
>> Since the bug looks like it might be exploitable I tried to create
>> a test case to trigger a call to _IO_wdefault_doallocate but couldn't.
>> No test in the test suite seems to either, so I post this patch without
>> one.
> 
> I spent some time following those vtable setups and I don't think 
> _IO_wdefault_doallocate gets called at all.  The only instances it is 
> set as the doallocate callback are in strops, wmemstream and in 
> vswprintf.  In all those cases, the backing buffer is either user 
> supplied or is sized without using doallocate (as in the case of strops 
> overflow).
> 
> The only time libio needs to allocate buffers in the wide context is in 
> wfileops when it's actually using the underlying buffer.  In that case 
> however, the doallocate callback is different.
> 
> Nevertheless, this is a good cleanup, so LGTM.

Martin, I've pushed this patch for you.

Siddhesh
  

On 3/1/21 7:06 AM, Siddhesh Poyarekar wrote:
> On 2/4/21 3:17 PM, Siddhesh Poyarekar wrote:
>> On 1/2/21 4:04 AM, Martin Sebor via Libc-alpha wrote:
>>> An experimental build of GCC 11 with an enhanced -Warray-bounds
>>> reports a bug in IO_wdefault_doallocate where the function forms
>>> an invalid past-the-end pointer to an allocated wchar_t buffer
>>> by failingf to consider the scaling by sizeof (wchar_t).
>>>
>>> The fix path below corrects this problem.  It keeps the buffer
>>> size the same as opposed to increasing it according to what other
>>> code like it does.
>>>
>>> Since the bug looks like it might be exploitable I tried to create
>>> a test case to trigger a call to _IO_wdefault_doallocate but couldn't.
>>> No test in the test suite seems to either, so I post this patch without
>>> one.
>>
>> I spent some time following those vtable setups and I don't think 
>> _IO_wdefault_doallocate gets called at all.  The only instances it is 
>> set as the doallocate callback are in strops, wmemstream and in 
>> vswprintf.  In all those cases, the backing buffer is either user 
>> supplied or is sized without using doallocate (as in the case of 
>> strops overflow).
>>
>> The only time libio needs to allocate buffers in the wide context is 
>> in wfileops when it's actually using the underlying buffer.  In that 
>> case however, the doallocate callback is different.
>>
>> Nevertheless, this is a good cleanup, so LGTM.
> 
> Martin, I've pushed this patch for you.

Thanks! (And sorry for not getting to it myself.  I'm behind on
a number of fronts.)

Martin

> 
> Siddhesh