Re: Issue with unget_wch_sp and -D_FORTIFY_SOURCE

[Thomas Dickey]

----- Original Message -----
| From: "Thomas Dickey" <dickey@his.com>
| To: "Siddhesh Poyarekar" <siddhesh@gotplt.org>, "Martin Liška" 
<mliska@suse.cz>, "Ncurses Mailing List"
| <bug-ncurses@gnu.org>
| Cc: "Thomas Dickey" <dickey@his.com>
| Sent: Saturday, April 9, 2022 2:10:25 PM
| Subject: Re: Issue with unget_wch_sp and -D_FORTIFY_SOURCE

| 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 ();
   return result;  // FIX :-)
| }

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