[Bug 60349] New: wineserver: get_next_window() walks an unlinked window's siblings forever, freezing the whole prefix

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=60349

            Bug ID: 60349
           Summary: wineserver: get_next_window() walks an unlinked
                    window's siblings forever, freezing the whole prefix
           Product: Wine
           Version: 11.16
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: major
          Priority: P2
         Component: wineserver
          Assignee: [e-mail]
          Reporter: [e-mail]
  Target Milestone: ---
      Distribution: ---

server/window.c gives each window two child lists: 'children', the Z-order, and
'unlinked'. create_window() puts every new window in 'unlinked' and
link_window()
moves one back there when no predecessor is given; the 'is_linked' bit says
which
list a window is in.

get_next_window() ignores that bit and bounds its walk on one list only:

    struct list *ptr = list_next( &win->parent->children, &win->entry );

list_next() returns NULL only when win->entry.next == &win->parent->children.
For a window in 'unlinked' that never happens: the walk reaches ITS head,
list_next() returns that head as though it were an element, and LIST_ENTRY()
produces a "window" at parent - 0x10 whose entry.next points back at the window
the walk started from. get_window_list() then cycles between the two forever.
The handle array is bounded by max_count, so nothing is overwritten; (*count)++
is not, and the loop never exits.

This code is unchanged in current master (f8b1ce3): get_next_window() there is
byte-identical to the 11.16 version measured below.

HOW IT WAS FOUND

On a headless Linux audio appliance running Wine 11.16 (Staging), after a VST3
plug-in editor was torn down. perf on the prefix's wineserver: 8086 samples,
99.9% inside the 56 bytes of that loop. gdb on the live process:

    window A  = 0x58c0b334b100   handle 0x1014e   parent 0x58c0b32e1f50
    &parent->children = 0x58c0b32e1fa8
    &parent->unlinked = 0x58c0b32e1fb8
    A->entry.next     = 0x58c0b32e1fb8   == &parent->unlinked, so A is unlinked
    LIST_ENTRY(&parent->unlinked) = 0x58c0b32e1f40 = parent - 0x10, a window
        that does not exist; its handle field reads 0xa92f4808
    that fake window's entry.next = 0x58c0b334b178 = &A->entry
    *count = 1601127262, against max_count = 127

WHY IT IS WORSE THAN ONE STUCK CALL

The wineserver is single-threaded and serves every process in the prefix, so
the whole prefix stops, not just the caller. The server kept burning 99% of a
core until it was killed. Nothing about this is specific to audio, to VST3, or
to any copy-protection scheme: any application that enumerates the siblings of
an unlinked window freezes the prefix for everything else running in it.

SUGGESTED FIX

Test is_linked and return NULL. An unlinked window has no Z-order siblings, so
that is the question the function is actually asking, and it fixes all six
get_next_window()/get_prev_window() walks in the file at once. Behaviour for a
linked window is unchanged.

Bounding the walk on &parent->unlinked as well would also stop the spin -- that
was the first version tried here -- but with two or more unlinked children it
would still report the others as Z-order siblings, which is wrong.

A patch against current master exists and will follow as a merge request on
gitlab.winehq.org referencing this bug.

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.