[v1] stdlib: Remove attr_write from mbstows if dst is NULL [BZ: 29265]

  mbstows is defined if dst is NULL and is defined to special cased if
dst is NULL so the fortify objsize check if incorrect in that case.

Tested on x86-64 linux.
---
Note. I wasn't able to get the test to actually throw an error
before the change.


 stdlib/Makefile      |  3 +++
 stdlib/bits/stdlib.h | 16 +++++++++++-----
 stdlib/testmb.c      |  7 +++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
  

On 21/06/2022 21:48, Noah Goldstein via Libc-alpha wrote:
> mbstows is defined if dst is NULL and is defined to special cased if
> dst is NULL so the fortify objsize check if incorrect in that case.
> 
> Tested on x86-64 linux.
> ---
> Note. I wasn't able to get the test to actually throw an error
> before the change.

The test would rely on whether the middle end is able to fold away the 
condition late enough, so it's flaky enough for us to not care about it. 
  It's fine to have it in though as a smoke test.

> 
> 
>   stdlib/Makefile      |  3 +++
>   stdlib/bits/stdlib.h | 16 +++++++++++-----
>   stdlib/testmb.c      |  7 +++++++
>   3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 60fc59c12c..6ef725ef74 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -373,6 +373,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags)
>   CFLAGS-tst-makecontext.c += -funwind-tables
>   CFLAGS-tst-makecontext2.c += $(stack-align-test-flags)
>   
> +CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror
> +
> +
>   # Run a test on the header files we use.
>   tests-special += $(objpfx)isomac.out
>   
> diff --git a/stdlib/bits/stdlib.h b/stdlib/bits/stdlib.h
> index 277d099e22..9ab66db6a4 100644
> --- a/stdlib/bits/stdlib.h
> +++ b/stdlib/bits/stdlib.h
> @@ -96,6 +96,11 @@ extern size_t __mbstowcs_chk (wchar_t *__restrict __dst,
>   			      const char *__restrict __src,
>   			      size_t __len, size_t __dstlen) __THROW
>       __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
> +extern size_t __REDIRECT_NTH (__mbstowcs_chk_nulldst,
> +			      (wchar_t *__restrict __dst,
> +			       const char *__restrict __src,
> +			       size_t __len), mbstowcs_chk)
> +    __attr_access ((__read_only__, 2));
>   extern size_t __REDIRECT_NTH (__mbstowcs_alias,
>   			      (wchar_t *__restrict __dst,
>   			       const char *__restrict __src,
> @@ -108,16 +113,17 @@ extern size_t __REDIRECT_NTH (__mbstowcs_chk_warn,
>        __warnattr ("mbstowcs called with dst buffer smaller than len "
>   		 "* sizeof (wchar_t)");
>   
> -__fortify_function size_t
> +__always_inline __fortify_function size_t
>   __NTH (mbstowcs (wchar_t *__restrict __dst, const char *__restrict __src,
>   		 size_t __len))
>   {
> -  return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t),
> -			    __glibc_objsize (__dst),
> -			    __dst, __src, __len);
> +  if (__builtin_constant_p (__dst) && __dst == NULL)

Perhaps a better condition would be __builtin_constant_p (__dst == NULL) 
&& __dst == NULL so that it does not rely on __dst to be a constant. 
For example, it could be a variable that the compiler decides can have 
only a NULL value.

> +    return __mbstowcs_chk_nulldst (__dst, __src, __len);
> +  else
> +    return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t),
> +			      __glibc_objsize (__dst), __dst, __src, __len);

OK.

>   }
>   
> -
>   extern size_t __wcstombs_chk (char *__restrict __dst,
>   			      const wchar_t *__restrict __src,
>   			      size_t __len, size_t __dstlen) __THROW
> diff --git a/stdlib/testmb.c b/stdlib/testmb.c
> index 45dae7db61..6ac4dfd21d 100644
> --- a/stdlib/testmb.c
> +++ b/stdlib/testmb.c
> @@ -16,6 +16,13 @@ main (int argc, char *argv[])
>         lose = 1;
>       }
>   
> +  i = mbstowcs (NULL, "bar", 4);
> +  if (!(i == 3 && w[1] == 'a'))
> +    {
> +      puts ("mbstowcs FAILED2!");
> +      lose = 1;
> +    }
> +
>     mbstowcs (w, "blah", 5);
>     i = wcstombs (c, w, 10);
>     if (i != 4)

OK.
  

On Wed, Jun 22, 2022 at 12:09 AM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 21/06/2022 21:48, Noah Goldstein via Libc-alpha wrote:
> > mbstows is defined if dst is NULL and is defined to special cased if
> > dst is NULL so the fortify objsize check if incorrect in that case.
> >
> > Tested on x86-64 linux.
> > ---
> > Note. I wasn't able to get the test to actually throw an error
> > before the change.
>
> The test would rely on whether the middle end is able to fold away the
> condition late enough, so it's flaky enough for us to not care about it.
>   It's fine to have it in though as a smoke test.

Alright.

>
> >
> >
> >   stdlib/Makefile      |  3 +++
> >   stdlib/bits/stdlib.h | 16 +++++++++++-----
> >   stdlib/testmb.c      |  7 +++++++
> >   3 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/stdlib/Makefile b/stdlib/Makefile
> > index 60fc59c12c..6ef725ef74 100644
> > --- a/stdlib/Makefile
> > +++ b/stdlib/Makefile
> > @@ -373,6 +373,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags)
> >   CFLAGS-tst-makecontext.c += -funwind-tables
> >   CFLAGS-tst-makecontext2.c += $(stack-align-test-flags)
> >
> > +CFLAGS-testmb.c += -D_FORTIFY_SOURCE=2 -Wall -Werror
> > +
> > +
> >   # Run a test on the header files we use.
> >   tests-special += $(objpfx)isomac.out
> >
> > diff --git a/stdlib/bits/stdlib.h b/stdlib/bits/stdlib.h
> > index 277d099e22..9ab66db6a4 100644
> > --- a/stdlib/bits/stdlib.h
> > +++ b/stdlib/bits/stdlib.h
> > @@ -96,6 +96,11 @@ extern size_t __mbstowcs_chk (wchar_t *__restrict __dst,
> >                             const char *__restrict __src,
> >                             size_t __len, size_t __dstlen) __THROW
> >       __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
> > +extern size_t __REDIRECT_NTH (__mbstowcs_chk_nulldst,
> > +                           (wchar_t *__restrict __dst,
> > +                            const char *__restrict __src,
> > +                            size_t __len), mbstowcs_chk)
> > +    __attr_access ((__read_only__, 2));
> >   extern size_t __REDIRECT_NTH (__mbstowcs_alias,
> >                             (wchar_t *__restrict __dst,
> >                              const char *__restrict __src,
> > @@ -108,16 +113,17 @@ extern size_t __REDIRECT_NTH (__mbstowcs_chk_warn,
> >        __warnattr ("mbstowcs called with dst buffer smaller than len "
> >                "* sizeof (wchar_t)");
> >
> > -__fortify_function size_t
> > +__always_inline __fortify_function size_t
> >   __NTH (mbstowcs (wchar_t *__restrict __dst, const char *__restrict __src,
> >                size_t __len))
> >   {
> > -  return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t),
> > -                         __glibc_objsize (__dst),
> > -                         __dst, __src, __len);
> > +  if (__builtin_constant_p (__dst) && __dst == NULL)
>
> Perhaps a better condition would be __builtin_constant_p (__dst == NULL)
> && __dst == NULL so that it does not rely on __dst to be a constant.
> For example, it could be a variable that the compiler decides can have
> only a NULL value.
>

Bright. Fixed in v2.

> > +    return __mbstowcs_chk_nulldst (__dst, __src, __len);
> > +  else
> > +    return __glibc_fortify_n (mbstowcs, __len, sizeof (wchar_t),
> > +                           __glibc_objsize (__dst), __dst, __src, __len);
>
> OK.
>
> >   }
> >
> > -
> >   extern size_t __wcstombs_chk (char *__restrict __dst,
> >                             const wchar_t *__restrict __src,
> >                             size_t __len, size_t __dstlen) __THROW
> > diff --git a/stdlib/testmb.c b/stdlib/testmb.c
> > index 45dae7db61..6ac4dfd21d 100644
> > --- a/stdlib/testmb.c
> > +++ b/stdlib/testmb.c
> > @@ -16,6 +16,13 @@ main (int argc, char *argv[])
> >         lose = 1;
> >       }
> >
> > +  i = mbstowcs (NULL, "bar", 4);
> > +  if (!(i == 3 && w[1] == 'a'))
> > +    {
> > +      puts ("mbstowcs FAILED2!");
> > +      lose = 1;
> > +    }
> > +
> >     mbstowcs (w, "blah", 5);
> >     i = wcstombs (c, w, 10);
> >     if (i != 4)
>
> OK.