[PATCH weston] terminal: Fix crash due to race condition in init

[Mike Blumenkrantz]

Hi,

Reverting this causes a 100% reproducible crash on startup in one of my CI
tests for weston-terminal. Here is some of the debugging info:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000000000040ab93 in handle_char (terminal=0x6348a0, utf8=...) at
clients/terminal.c:1991
1991 row[terminal->column] = utf8;
Missing separate debuginfos, use: dnf debuginfo-install
weston-1.12.0-1.fc25.x86_64
(gdb) bt
#0  0x000000000040ab93 in handle_char (terminal=0x6348a0, utf8=...) at
clients/terminal.c:1991
#1  0x000000000040b38d in terminal_data (terminal=0x6348a0,
data=0x7fffffffdb40 "sh-4.3$ ",
    length=8) at clients/terminal.c:2172
#2  0x000000000040d296 in io_handler (task=0x6348c8, events=1) at
clients/terminal.c:3021
#3  0x0000000000419d7f in display_run (display=0x62e8c0) at
clients/window.c:6506
#4  0x000000000040d5a2 in main (argc=1, argv=0x7fffffffde98) at
clients/terminal.c:3109
(gdb) p terminal->column
$1 = 0
(gdb) p row
$2 = (union utf8_char *) 0x0
(gdb) l
1986 row = terminal_get_row(terminal, terminal->row);
1987 attr_row = terminal_get_attr_row(terminal, terminal->row);
1988
1989 if (terminal->mode & MODE_IRM)
1990 terminal_shift_line(terminal, +1);
1991 row[terminal->column] = utf8;
1992 attr_row[terminal->column++] = terminal->curr_attr;
1993
1994 if (terminal->row + terminal->start + 1 > terminal->end)
1995 terminal->end = terminal->row + terminal->start + 1;
(gdb) p terminal->row
$3 = -1


On Fri, Sep 9, 2016 at 1:55 PM Quentin Glidic <
sardemff7+wayland at sardemff7.net> wrote:

> On 09/09/2016 19:02, Derek Foreman wrote:
> > On 31/08/16 05:56 PM, Quentin Glidic wrote:
> >> On 30/08/2016 04:07, Yong Bakos wrote:
> >>> On Aug 29, 2016, at 4:28 PM, Bryce Harrington <bryce at osg.samsung.com>
> >>> wrote:
> >>>>
> >>>> weston-terminal intermittently crashes on startup.  This occurs
> because
> >>>> some parameters in the weston_terminal structure such as data_pitch,
> >>>> don't get set to non-zero until the resize_handler() callback gets
> >>>> triggered.  That callback makes a call to terminal_resize_cells(), to
> >>>> calculate the proper values for these parameters.
> >>>>
> >>>> On occasion, the resize handler call is slow to resolve, and the
> program
> >>>> proceeds to start processing characters for the terminal window.  With
> >>>> the parameters defaulting to zero, certain calculations come out
> wrong,
> >>>> leading the program to attempt to scroll the buffer when it shouldn't,
> >>>> and thus follows the crash.
> >>>>
> >>>> Instead, force the call to terminal_resize_cells() during the init,
> with
> >>>> some dummy defaults, to ensure the parameters are always non-zero.
> >>>>
> >>>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=97539
> >>>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> >>>
> >>> I couldn't replicate the issue, but the fix seems innocuous enough.
> >>> Someone with more experience should chime in, but this is:
> >>>
> >>> Reviewed-by: Yong Bakos <ybakos at humanoriented.com>
> >>
> >>
> >> Don’t see any harm having it as a quick fix. I wonder if xdg_shell_v6
> >> initial configure can be used as a sync point for all this?
> >>
> >> Pushed:
> >> c6ae812..5c611d9  master -> master
> >
> > Hey, can we revert this immediately? :)
> > (Unless someone has a better patch for the problem - I haven't actually
> > seen that crash myself.)
> >
> > Turns out when you launch weston from a console with the drm backend,
> > then start a weston-terminal, it sets weston's controlling terminal to
> > be 20x5.  The ioctl() at the bottom of terminal_resize_cells() appears
> > to be hitting weston's control terminal.
> >
> > When you exit weston you get a really great surprise.
> >
> > My system has systemd-logind, but I see it when running from
> > weston-launch too...
>
> Sure, reverted:
> 6967f0e..e30b5fb  master -> master
>
> Cheers,
>
>
> >
> > Thanks,
> > Derek
> >
> >> Cheers,
> >>
> >>>
> >>>> ---
> >>>> clients/terminal.c | 1 +
> >>>> 1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/clients/terminal.c b/clients/terminal.c
> >>>> index 6257cb7..34bc2c9 100644
> >>>> --- a/clients/terminal.c
> >>>> +++ b/clients/terminal.c
> >>>> @@ -2976,6 +2976,7 @@ terminal_create(struct display *display)
> >>>>     cairo_surface_destroy(surface);
> >>>>
> >>>>     terminal_resize(terminal, 20, 5); /* Set minimum size first */
> >>>> +    terminal_resize_cells(terminal, 20, 5);
> >>>>     terminal_resize(terminal, 80, 25);
> >>>>
> >>>>     wl_list_insert(terminal_list.prev, &terminal->link);
> >>>> --
> >>>> 1.9.1
> >>>>
> >>>> _______________________________________________
> >>>> wayland-devel mailing list
> >>>> wayland-devel at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >>>
> >>> _______________________________________________
> >>> wayland-devel mailing list
> >>> wayland-devel at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >>>
> >>
> >>
> >
>
>
> --
>
> Quentin “Sardem FF7” Glidic
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170217/dccbbc2d/attachment-0001.html>