Bug 241538 - sysutils/screen: update from 4.6.2_3 to 4.7.0_1 crashes on startup with TERM=rxvt (and some xterm variants)
Summary: sysutils/screen: update from 4.6.2_3 to 4.7.0_1 crashes on startup with TERM=...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Christoph Moench-Tegeder
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-28 12:42 UTC by Marcin Cieślak
Modified: 2019-11-06 21:13 UTC (History)
1 user (show)

See Also:
bugzilla: maintainer-feedback? (cy)


Attachments
Quick fix : revert fad4c29fd95d30639a67142066f623396fca8536 (1.57 KB, patch)
2019-10-28 13:06 UTC, Marcin Cieślak
no flags Details | Diff
fix coredump on xterm and rxvt and bump portrevision (1.10 KB, patch)
2019-10-28 13:17 UTC, Christoph Moench-Tegeder
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcin Cieślak 2019-10-28 12:42:33 UTC
screen 4.7.0_1 dumps core when used with TERM=rxvt-unicode-256color or TERM=rxvt.

Seen on freshly upgraded 11.3-STABLE box:

FreeBSD xxx 11.3-RELEASE-p3 FreeBSD 11.3-RELEASE-p3 #0: Mon Aug 19 21:08:43 UTC 2019     root@amd64-builder.daemonology.net:/usr/obj/usr/src/sys/GENERIC amd64 amd64 amd64 FreeBSD

as well as an non-upgraded 12.0-STABLE box:

FreeBSD xxx 12.0-STABLE FreeBSD 12.0-STABLE r345375 GENERIC amd64 amd64 amd64 FreeBSD

Backtrace:


Starting program: /usr/local/bin/screen 
[Attaching after LWP 100616 of process 54574 fork to child LWP 100786 of process 54578]
[New inferior 2 (process 54578)]
[Detaching after fork from parent process 54574]
[Inferior 1 (process 54574) detached]

Thread 2.1 received signal SIGSEGV, Segmentation fault.
[Switching to LWP 100786 of process 54578]
strlen (str=0x0) at /usr/src/lib/libc/string/strlen.c:100
warning: Source file is more recent than executable.
100		va = (*lp - mask01);
(gdb) bt
#0  strlen (str=0x0) at /usr/src/lib/libc/string/strlen.c:100
#1  0x00000000004225a5 in SaveStr (str=0x0) at misc.c:59
#2  0x0000000000439535 in InitTermcap (wi=0, he=0) at termcap.c:230
#3  0x0000000000406832 in main (ac=0, av=0x7fffffffea80) at screen.c:1370
(gdb) quit

Happens with screen setuid or non-setuid root (therefore unrelated to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241530)
Comment 1 Marcin Cieślak 2019-10-28 13:06:07 UTC
Created attachment 208652 [details]
Quick fix : revert fad4c29fd95d30639a67142066f623396fca8536

A quick fix:

Reverting this change to screen 4.x branch:

commit fad4c29fd95d30639a67142066f623396fca8536
Author: Amadeusz Sławiński <amade@asmblr.net>
Date:   Sat Sep 7 17:58:50 2019 +0200

    Fix broken mouse after ncurses 6.1
    
    ncurses 6.1 changed kmous capability from "\e[M" to "\e[<". It seems to
    be done to signal that terminal supports sgr mouse mode. screen assumed
    that if kmous is set to "\e[M" it is on xterm compatible terminal
    anyway, so just dynamically detect which one is used and override
    relevant kmapdef.
    
    InitKeytab() is moved, so kmapdef[] can be overriden before
    initialization, as InitTermcap() needs to run first, as far as I can
    tell this should have no consequences.
    
    Signed-off-by: Amadeusz Sławiński <amade@asmblr.net>
Comment 2 Christoph Moench-Tegeder freebsd_committer 2019-10-28 13:17:30 UTC
Created attachment 208653 [details]
fix coredump on xterm and rxvt and bump portrevision

Here's the rub: with TERM=xterm (or rxvt, for that matter), Km ("key_mouse", "Mouse event has occured") is not set (and therefore NULL), but InitTermcap() (termcap.c:230) happily tries to strdup() that, which gets us that segfault.

As a band-aid, catch that NULL and don't strdup().

Workaround: set TERM to a terminal with Km - for example, TERM="xterm-v220" works just fine.
Comment 3 Marcin Cieślak 2019-10-28 13:20:09 UTC
Thanks, you patch is better :)
Comment 4 Marcin Cieślak 2019-10-28 13:26:58 UTC
I think it didn't crash for me with TERM=xterm though.

tput says it is there:



> tput -T xterm Km | hd
00000000  1b 5b 4d                                          |.[M|
00000003

> tput -T rxvt Km || echo not found
not found
Comment 5 Christoph Moench-Tegeder freebsd_committer 2019-10-28 13:33:53 UTC
(In reply to Marcin Cieślak from comment #4)

D'oh. Now that you mention that - I've set termName to "xterm-color" for my xterms (it has been years since I last touched my .Xresources...), and it's that xterm-color which has no Km (but it leaves TERM at "xterm", which confused me quite a bit). Yeah, real "xterm" is fine, but "xterm-color" (which disguises as xterm) is a problem.
Comment 6 Marcin Cieślak 2019-10-28 13:41:00 UTC
Upstream report: https://lists.gnu.org/archive/html/screen-users/2019-10/msg00003.html

Since screen is again in active development, maybe we should upstream some of our local patches there?
Comment 7 Cy Schubert freebsd_committer 2019-10-28 14:13:56 UTC
(In reply to Christoph Moench-Tegeder from comment #2)

I'm at $JOB at the moment. Can you commit this for me please? Consider this my approval.
Comment 8 commit-hook freebsd_committer 2019-10-28 14:43:18 UTC
A commit references this bug:

Author: cmt
Date: Mon Oct 28 14:42:21 UTC 2019
New revision: 515868
URL: https://svnweb.freebsd.org/changeset/ports/515868

Log:
  sysutils/screen: fix coredump on xterm/rxvt variants without Km

  On xterm- and rxvt-variants (and in some other cases) screen's
  InitTermcap() tries to strdup() the terminals "Km" ("key_mouse")
  termcap attribute - but that might be NULL, as some of these
  terminals (notably rxvt and xterm-color) do not have "Km". Trying
  to strdup() NULL results in segfault and coredump.
  Catch that NULL and prevent the segfault.

  PR:		241538
  Reported by:	Marcin Cie?lak, Gareth de Vaux
  Approved by:	cy@

Changes:
  head/sysutils/screen/Makefile
  head/sysutils/screen/files/patch-termcap.c
Comment 9 Christoph Moench-Tegeder freebsd_committer 2019-10-28 14:43:52 UTC
committed as ports r515868 - thanks.
Comment 10 Marcin Cieślak 2019-10-28 15:51:44 UTC
2 hours to fix - yay \o/

big thanks everyone!
Comment 11 Marcin Cieślak 2019-11-05 15:51:50 UTC
I have submitted the patch upstream:

https://lists.gnu.org/archive/html/screen-devel/2019-11/msg00013.html

https://lists.gnu.org/archive/html/screen-devel/2019-11/msg00014.html

I hope this is ok for you Christoph!
Comment 12 Christoph Moench-Tegeder freebsd_committer 2019-11-06 21:13:25 UTC
(In reply to Marcin Cieślak from comment #11)
screen is GPLed, so yeah, it's fine (and really, rather trivial, once you see it).