Re: Issue with unget_wch_sp and -D_FORTIFY_SOURCE

[Thomas Dickey]

On Sat, Apr 09, 2022 at 11:07:41AM -0400, Thomas Dickey wrote:
> On Thu, Apr 07, 2022 at 07:22:43AM +0530, Siddhesh Poyarekar wrote:
> > On 07/04/2022 06:23, Thomas Dickey wrote:
> > > not quite:
> > > 
> > >   the wcrtomb() function shall determine the number of bytes needed to
> > >   represent the character that corresponds to the wide character given by
> > >   wc (including any shift sequences), and store the resulting bytes in
> > >   the array whose first element is pointed to by s.  At most {MB_CUR_MAX}
> > >   bytes are stored.
> > > That last part is a guarantee that the representation won't be longer
> > > than a given limit.  But writing zeroes out violates the first statement,
> > > if those bytes are not _needed_.
> > 
> > Does it though?  The first statement only says that those bytes are written
> > out, not that _only_ those bytes (or needed bytes) are stored; the comment
> > about the amoung of storage is only made in the second statement.
> 
> I think the description is clear enough (if you disagree, the
>       austin-group-l@opengroup.org
> mailing list is the appropriate place to get that confirmed or whatever).
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/wcsrtombs.html
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/wcrtomb.html
> 
> > I'll admit however (in addition to the fact that I was the one who made that
> > statement to Martin) that it's likely not the intended reading, which is why
> > I qualified it with "defensive".  It's probably an unnecessary distraction
> > since the stricter fortification decision in glibc is not based on that
> > reading.
> > 
> > > However, the call that you're reporting on is a different problem than 
> > > that.
> > > 
> > > Offhand reading the code, it looks as if I didn't add 1 for the 
> > > terminating
> > > null (in the call to malloc) which is documented here:
> 
> I misread that - wcrtomb shouldn't produce a trailing null,
> and need not use that detail from wcsrtombs :-)
> 
> > >         The wcsrtombs() function returns the number of bytes that make  
> > > up  the
> > >         converted  part  of  multibyte  sequence, not including the 
> > > terminating
> > >         null byte.  If a wide character was encountered which could not 
> > > be con‐
> > >         verted, (size_t) -1 is returned, and errno set to EILSEQ.
> > 
> > The glibc fortification (enabled by _FORTIFY_SOURCE) strictly requires the
> > buffer to be MB_CUR_MAX long so despite fixing that bug (I trust your
> 
> I see this, apparently applying to wcrtomb:
> 
> https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_chapter/libc_6.html
> 
>       One word about the interface of the function: there is no parameter
>       specifying the length of the array s.  Instead the function assumes
>       that there are at least MB_CUR_MAX bytes available since this is the
>       maximum length of any byte sequence representing a single character. 
>       So the caller has to make sure that there is enough space available,
>       otherwise buffer overruns can occur.
> 
> so it's "documented" -- not in the manual page, but in the info document
> (but since I don't see the same limitation in other implementations, am
> unconvinced that it complies with POSIX).  By the way, the manual page
> mentions POSIX, while that section of the info document does not:
> 
>       CONFORMING TO
>              POSIX.1-2001, POSIX.1-2008, C99.
> 
> That paragraph in the info document doesn't allow for the case we're
> discussing:  where the application determines (using the POSIX interface) the
> number of bytes required, and uses that for the output buffer length.
> If glibc writes past that number, it's no longer an implementation "quirk", 
> but
> a bug -- in glibc -- because it is not telling the application a correct 
> number
> for the required bytes.
> 
> Likewise, if the fortify-checks don't agree with what the library
> actually does, that's at best a nuisance (such as the frequent false-positives
> warnings triggered by checks for null-pointers which I've seen in gcc over
> the past few years).
> 
> If the fortity-check had inspected the return value from the function,
> it could more aptly determine if there was a buffer overflow.  Applying
> an incorrect limit-check on entry to the function makes the result
> non-conforming.
> 
> > assertion on its existence) if the destination size is less than MB_CUR_MAX,
> > the function will still crash.  Folks from the glibc community discussed
> 
> ...but does it crash for some reproducible case with the ncurses code
> (which first determines the length)?
> 
> So far, all I've seen is an issue with a limit-check which doesn't
> agree with the expected value -- in glibc.  If the limit-check is
> wrong (and glibc works as expected _without_ that check), then it's
> a problem with the limit-check.

for instance, this is incorrect:

size_t
__wcrtomb_chk (char *s, wchar_t wchar, mbstate_t *ps, size_t buflen)
{
  /* We do not have to implement the full wctomb semantics since we
     know that S cannot be NULL when we come here.  */
  if (buflen < MB_CUR_MAX)
    __chk_fail ();

  return __wcrtomb (s, wchar, ps);
}

here's an alternative version without the above noted problems:

size_t
__wcrtomb_chk (char *s, wchar_t wchar, mbstate_t *ps, size_t buflen)
{
  size_t result = __wcrtomb (s, wchar, ps);
  if (result != (size_t)(-1) && result >= MB_CUR_MAX) 
    __chk_fail ();
}

-- 
Thomas E. Dickey <dickey@invisible-island.net>
https://invisible-island.net
ftp://ftp.invisible-island.net

signature.asc
Description: PGP signature