[src/trunk]: src/lib/libedit From Ingo Schwarze:

[christos <christos%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/70e2b15831ed
branches:  trunk
changeset: 343532:70e2b15831ed
user:      christos <christos%NetBSD.org@localhost>
date:      Fri Feb 12 15:11:09 2016 +0000

description:
>From Ingo Schwarze:

If CHARSET_IS_UTF8 is not set, read_char() is broken in a large
number of ways:

 1. The isascii(3) check can yield false positives.  If a string in
    an arbitrary encoding contains a byte in the range 0..127,
    that does not at all imply that it forms a character all by
    itself, and even less that it represents the same character
    as in ASCII.  Consequently, read_char() may return characters
    the user never typed.
    Even if the encoding is not state dependent, the assumption that
    bytes in the range 0..127 represent ASCII characters is broken.
    Consider UTF-16, for example.

 2. The reverse problem can also occur.  In an arbitrary encoding,
    there is no guarantee that a character that can be represented
    by ASCII is represented by a seven-bit byte, and even less by
    the same byte as in ASCII.
    Even for single-byte encodings, these assumptions are broken.
    Consider the ISO 646 national variants, for example.
    Consequently, the current code is insufficient to keep ASCII
    characters working even for single-byte encodings.

 3. The condition "++cbp != 1" can never trigger (because initially,
    cbp is 0, and the code can only go back up via the final goto,
    which has another cbp = 0 right before it) and it has no effect
    (because cbp isn't used afterwards).

 4. bytes = ct_mbtowc(cp, cbuf, cbp) is broken.  If this returns -1,
    the code assumes that is can just call mbtowc(3) again for later
    input bytes.  In some implementations, that may even be broken
    for state-independent encodings, but trying again after mbtowc(3)
    failure certainly produces completely erratic and meaningless
    results in state-dependent encodings.

 5. The assignment "*cp = (Char)(unsigned char)cbuf[0]" is
    completely bogus.  Even if the byte cbuf[0] represents a
    character all by itself, which it usually will not, whether
    or not the cast produces the desired result depends on the
    internal representation of wchar_t in the C library, which
    the application program can know nothing about.  Even for ASCII
    in the C/POSIX locale, an ASCII character other than '\0' ==
    L'\0' == 0 need not have the same numeric value as a char and
    as a wchar_t.

To summarize, this code only works if all of the following
conditions hold:

 - The encoding is a single-byte encoding.
 - ASCII is a subset of the encoding.
 - The implementation of mbtowc(3) in the C library does not
   require re-initialization after encoding errors.
 - The implementation of wchar_t in the C library uses the
   same numerical values as ASCII.

Otherwise, it silently produces wrong results.

The simplest way to fix this is to just use the same code as for
UTF-8 (right above).  Of course, that causes functional changes
but that shouldn't matter since current behaviour is undefined.

The patch below provides the following improvements:

 - It works for all stateless single-byte encodings, no matter
   whether they are somehow related to ASCII, no matter how
   mb[r]towc(3) are internally implemented, and no matter how
   wchar_t is internally represented.
 - Instead of producing unpredictable and definitely wrong
   results for non-UTF-8 multibyte characters, it behaves in
   a well-defined way: It aborts input processing, sets errno,
   and returns failure.
   Note that short of providing full support for arbitrary locales,
   it is impossible to do better.  We cannot know whether a given
   unsupported locale is state-dependent, and for a state-dependent
   locale, it makes no sense to retry parsing after an encoding
   error, so the best we can do is abort processing for *any*
   unsupported multi-byte character.
 - Note that single-byte characters in arbitrary state-independent
   locales still work, even in locales that may potentially also
   contain multibyte characters, as long as those don't occur in
   input.  I'm not sure whether any such locales exist in practice...

Tested with UTF-8 and C/POSIX on OpenBSD.  Also tested that in the
C/POSIX locale, non-ASCII bytes get through unmangled.  You may
wish to test with ISO-LATIN on NetBSD if NetBSD supports that.

----
Also use a constant for meta to avoid warnings.

diffstat:

 lib/libedit/read.c |  28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diffs (85 lines):

diff -r 6b12d3c560ce -r 70e2b15831ed lib/libedit/read.c
--- a/lib/libedit/read.c        Fri Feb 12 09:24:15 2016 +0000
+++ b/lib/libedit/read.c        Fri Feb 12 15:11:09 2016 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: read.c,v 1.74 2016/02/11 19:21:04 christos Exp $       */
+/*     $NetBSD: read.c,v 1.75 2016/02/12 15:11:09 christos Exp $       */
 
 /*-
  * Copyright (c) 1992, 1993
@@ -37,7 +37,7 @@
 #if 0
 static char sccsid[] = "@(#)read.c     8.1 (Berkeley) 6/4/93";
 #else
-__RCSID("$NetBSD: read.c,v 1.74 2016/02/11 19:21:04 christos Exp $");
+__RCSID("$NetBSD: read.c,v 1.75 2016/02/12 15:11:09 christos Exp $");
 #endif
 #endif /* not lint && not SCCSID */
 
@@ -239,6 +239,7 @@
 private int
 read_getcmd(EditLine *el, el_action_t *cmdnum, Char *ch)
 {
+       static const Char meta = (Char)0x80;
        el_action_t cmd;
        int num;
 
@@ -250,7 +251,7 @@
                }
 
 #ifdef KANJI
-               if ((*ch & 0200)) {
+               if ((*ch & meta)) {
                        el->el_state.metanext = 0;
                        cmd = CcViMap[' '];
                        break;
@@ -259,7 +260,7 @@
 
                if (el->el_state.metanext) {
                        el->el_state.metanext = 0;
-                       *ch |= (unsigned char)0200;
+                       *ch |= meta;
                }
 #ifdef WIDECHAR
                if (*ch >= N_KEYS)
@@ -338,7 +339,7 @@
        }
 
 #ifdef WIDECHAR
-       if (el->el_flags & CHARSET_IS_UTF8) {
+       do {
                mbstate_t mbs;
                size_t rbytes;
 again_lastbyte:
@@ -361,7 +362,13 @@
                                goto again;
                        }
                case (size_t)-2:
-                       if (cbp >= MB_LEN_MAX) { /* "shouldn't happen" */
+                       /*
+                        * We don't support other multibyte charsets.
+                        * The second condition shouldn't happen
+                        * and is here merely for additional safety.
+                        */
+                       if ((el->el_flags & CHARSET_IS_UTF8) == 0 ||
+                           cbp >= MB_LEN_MAX) {
                                errno = EILSEQ;
                                *cp = '\0';
                                return -1;
@@ -373,13 +380,10 @@
                        bytes = (int)rbytes;
                        break;
                }
-       } else if (isascii((unsigned char)cbuf[0]) ||
-               /* we don't support other multibyte charsets */
-               ++cbp != 1 ||
-               /* Try non-ASCII characters in a 8-bit character set */
-               (bytes = ct_mbtowc(cp, cbuf, cbp)) != 1)
+       } while (/*CONSTCOND*/0);
+#else
+               *cp = (Char)(unsigned char)cbuf[0];
 #endif
-               *cp = (Char)(unsigned char)cbuf[0];
 
        if ((el->el_flags & IGNORE_EXTCHARS) && bytes > 1) {
                cbp = 0; /* skip this character */