CVS commit: src/lib/libedit

["Christos Zoulas" <christos%netbsd.org@localhost>]

Module Name:    src
Committed By:   christos
Date:           Mon Feb  8 17:18:43 UTC 2016

Modified Files:
        src/lib/libedit: chartype.h read.c

Log Message:
UTF-8 fixes from Ingo Schwarze:

 1. Assume that errno is non-zero when entering read_char()
    and that read(2) returns 0 (indicating end of file).
    Then, the code will clear errno before returning.
    (Obviously, the statement "errno = 0" is almost always
     a bug unless there is save_errno = errno right before it
     and the previous value is properly restored later,
     in all reachable code paths.)

 2. When encountering an invalid byte sequence, the code discards
    all following bytes until MB_LEN_MAX overflows; consider, for
    example, 0xc2 immediately followed by a few valid ASCII bytes.
    Three of those ASCII bytes will be discarded.

 3. On a POSIX system, EILSEQ will always be set after reading a
    valid (yes, valid, not invalid!) UTF-8 character.  The reason
    is that mbtowc(3) will first be called with a length limit
    (third argument) of 1, which will fail, return -1, and - on
    a POSIX system - set errno to EILSEQ.
    This third bug is mitigated a bit because i couldn't find any
    system that actually conforms to POSIX in this respect:  None
    of OpenBSD, NetBSD, FreeBSD, Solaris 11, and glibc set errno
    when an incomplete character is passed to mbtowc(3), even though
    that is required by POSIX.
    Anyway, that mbtowc(3) bug will be fixed at least in OpenBSD
    after release unlock, so it would be good to fix this bug in
    libedit before fixing the bug in mbtowc(3).

How can these three bugs be fixed?

 1. As far as i understand it, the intention of the bogus errno = 0
    is to undo the effects of failing system calls in el_wset(),
    sig_set(), and read__fixio() if the subsequent read(2) indicates
    end of file.  So, restoring errno has to be moved right after
    read__fixio().  Of course, neither 0 nor e is the right value
    to restore: 0 is wrong if errno happened to be set on entry, e
    would be wrong because if one read(2) fails but a second attempt
    succeeds after read__fixio(), errno should not be touched.  So,
    the errno to be restored in this case has to be saved before
    calling read(2) for the first time.

 2. Solving the second issue requires distinguishing invalid and
    incomplete characters, but that is impossible with the function
    mbtowc(3) because it returns -1 in both cases and sets errno
    to EILSEQ in both cases (once properly implemented).

    It is vital that each input character is processed right away.
    It is not acceptable to wait for the next input character before
    processing the previous one because this is an interactive
    library, not a batch system.  Consequently, the only situation
    where it is acceptable to wait for the next byte without first
    processing the previous one(s) is when the previous one(s) form
    an incomplete sequence that can be continued to form a valid
    character.

    Consequently, short of reimplementing a full UTF-8 state machine
    by hand, the only correct way forward is to use mbrtowc(3).
    Even then, care is needed to always have the state object
    properly initialized before using it, and to not discard a valid
    ASCII or UTF-8 lead byte if it happens to follow an invalid
    sequence.

 3. Fortunately, solution 2. also solves issue 3. as a side effect,
    by no longer using mbtowc(3) in the first place.


To generate a diff of this commit:
cvs rdiff -u -r1.15 -r1.16 src/lib/libedit/chartype.h
cvs rdiff -u -r1.71 -r1.72 src/lib/libedit/read.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.