[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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



Hi Tavis,

thanks for having a look into this!

Tavis Ormandy wrote:
> On 2021-02-10, Axel Beckert wrote:
> > +  else if (i < sizeof combchars / sizeof *combchars) {
> 
> This doesn't seem right, I think it should be compared against the
> calloc param at the top of utf8_handle_comb(),

Good point, thanks!

Your patch works fine on a first glance. No side effects like these
"ÿ " I saw with Michael's proposed patch. (But Michael's patch also
seems to care more about these 0x800 ff. values, so his and your patch
probably go into the right direction while my fix was rather generic
and perhaps too local.)

> but I don't really understand enough about unicode to know where
> that 0x802 comes from!

Will have a closer look into that direction tomorrow. But it indeed
sounds saner than my patch. And it is also much shorter and less
intrusive as it doesn't need to indent a whole block.

> I think for sure this code doesn't handle c > 0x801,

Which would mean it just can handle Unicode characters which are
represented by two bytes in UTF-8 representation. Because
that's what's special about characters around that value:

U+07FF NKO TAMAN SIGN        is 0xDF 0xBF      in UTF-8.
U+0800 SAMARITAN LETTER ALAF is 0xE0 0xA0 0x80 in UTF-8.

(according to gucharmap)

This also explains why we see c1 and c2 in the code (and what they
mean) but e.g. no c3. It suddenly all starts to make more sense, yes.

BTW, credit for the right hint goes to
https://stackoverflow.com/questions/47783583/generating-3-byte-0x800-to-0xffff-utf-8-encodings-in-java

> so maybe that's an acceptable fix?

I probably haven't looked around far enough for my patch, yes. Just
looked at where the backtrace of the crash pointed me (same if-clause
as you pointed out on oss-sec) and tried to guard that one very
generically.

P.S.: A disclaimer for those who aren't aware of it: I'm not a GNU
Screen developer. I'm just Debian's screen package maintainer trying
fix that package in time for Debian's next stable release. So I can't
break GNU Screen, just Debian's package of it. ;-)

		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

Attachment: signature.asc
Description: PGP signature


Reply to: