Re: [screen-devel] [bug #60030] Screen segfaults by displaying some UTF-8 character combination

[Axel Beckert]

Hi again,

Axel Beckert wrote:
> On Wed, Feb 10, 2021 at 08:59:15AM -0500, Michael Schröder wrote:
> > diff --git a/src/encoding.c b/src/encoding.c
> > index 11c3c41..e1ea364 100644
> > --- a/src/encoding.c
> > +++ b/src/encoding.c
> > @@ -1164,7 +1164,9 @@ void utf8_handle_comb(unsigned int c, struct mchar 
> > *mc)
> >                 if (c1 >= 0xd800 && c1 < 0xe000)
> >                         comb_tofront(root, c1 - 0xd800);
> >                 i = combchars[root]->prev;
> > -               if (c1 == i + 0xd800) {
> > +               if (i == (unsigned int)root)
> > +                       i = combchars[root ^ 1]->prev;  /* steal from other
> > root */
> > +               if (i == 0x800 || i == 0x801 || c1 == i + 0xd800) {
> >                         /* completely full, can't recycle */
> >                         mc->image = '?';
> >                         mc->font = 0;
> 
> Thanks, but this seems to break the actual output.
> 
> With that patch I now get "ÿ " after every wide character in the
> output. The beginning now looks like this for me (in the hope it will
> be passed properly through mail):
> 
> 円ᆆᆿÿ 忿ᇎᆿÿ 忘ᆿᆿÿ 忿ᆾᆿÿ 応ᆿᆿÿ 忿ᆷᆿÿ 忑ᆿᆿÿ 忿ᇠᆿÿ 冺ᆿᆿÿ 忿ᇇᆿÿ 忟ᆿᆿÿ 忿ᆺᆿÿ 忳ᆿᆿÿ 忿ᅳᆿÿ 忣ᆿᆿÿ 
> 忿ᇯᆿÿ 忇ᆿᆿÿ 忿ᇅᆿÿ

Axel Beckert wrote:
> So your bug report is already publicly visible at
> https://lists.gnu.org/archive/html/screen-devel/2021-02/msg00000.html
> even though it is hidden on Savannah. (This is something those with
> admin access to the screen project on Savannah might want to review.)

Well, but my own line above seems to have crashed my screen session
through mutt at least once, but I can't reproduce this anymore — and I
wrote that line in Emacs in the very same screen session before hand
(by pasting that line).

Anyway, I seem to have been able to make up a patch (against 4.8.0 as
in Debian Unstable) which avoids the crash as well as the issue I
described in my previous mail which I cited above.

I though have no idea if the patch castrates any other functionality
or if it has unwanted side effects. Any review would be nice:

--- a/encoding.c
+++ b/encoding.c
@@ -1408,21 +1408,23 @@
        }
       /* FIXME: delete old char from all buffers */
     }
-  else if (!combchars[i])
-    {
-      combchars[i] = (struct combchar *)malloc(sizeof(struct combchar));
-      if (!combchars[i])
-       return;
-      combchars[i]->prev = i;
-      combchars[i]->next = i;
-    }
-  combchars[i]->c1 = c1;
-  combchars[i]->c2 = c;
-  mc->image = i & 0xff;
-  mc->font  = (i >> 8) + 0xd8;
-  mc->fontx = 0;
-  debug3("combinig char %x %x -> %x\n", c1, c, i + 0xd800);
-  comb_tofront(root, i);
+  else if (i < sizeof combchars / sizeof *combchars) {
+    if (!combchars[i])
+      {
+        combchars[i] = (struct combchar *)malloc(sizeof(struct combchar));
+        if (!combchars[i])
+          return;
+        combchars[i]->prev = i;
+        combchars[i]->next = i;
+      }
+    combchars[i]->c1 = c1;
+    combchars[i]->c2 = c;
+    mc->image = i & 0xff;
+    mc->font  = (i >> 8) + 0xd8;
+    mc->fontx = 0;
+    debug3("combinig char %x %x -> %x\n", c1, c, i + 0xd800);
+    comb_tofront(root, i);
+  }
 }
 
 #else /* !UTF8 */

The basic idea is to avoid an out of bounds array access at all by
first checking if "i" is bigger than the biggest index in the
combchars array.

I have no idea if the elements of the combchars array do have all the
same size. I just assume that all have the same size as the first
element.

At least that patch doesn't show that "ÿ " string after each wide
character. I though can imagine that it suppress maybe one or two
characters at the very end of the array.

I'm currently running that patch locally and will also trying to
create a patched version for the screen versions with which I run my
mail sessions to get a feeling for it in production.

P.S. to Utkarsh: That means I will prepare patches for Stretch and
Buster at least in Debian's git at
https://salsa.debian.org/debian/screen

                Regards, Axel
-- 
 ,''`.  |  Axel Beckert <abe@debian.org>, https://people.debian.org/~abe/
: :' :  |  Debian Developer, ftp.ch.debian.org Admin
`. `'   |  4096R: 2517 B724 C5F6 CA99 5329  6E61 2FF9 CD59 6126 16B5
  `-    |  1024D: F067 EA27 26B9 C3FC 1486  202E C09E 1D89 9593 0EDE

signature.asc
Description: PGP signature