Re: Issue with unget_wch_sp and -D_FORTIFY_SOURCE

[Thomas Dickey]

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.

> this here[1] in the context of snprintf and comment 1 in particular applies
> here; fortification checks may be stricter than what the standard requires.
> 
> Siddhesh
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28989

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

signature.asc
Description: PGP signature