Opened 12 years ago

Closed 12 years ago

#4115 closed patch (fixed)

Network Control causes segfaults

Reported by: danielk Owned by: danielk
Priority: minor Milestone: 0.21
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description (last modified by danielk)

The network control in TV::processNetworkControlCommand() ignores the TV::ignoreKeys variable and so can issue unsafe NVP actions during buffer switching resulting in segfaults.

Attachments (3)

4115-v1.patch (8.4 KB) - added by danielk 12 years ago.
Makes network control a bit safer
4115-fixes-v1.patch (6.9 KB) - added by danielk 12 years ago.
Backports network control changes to 0.20-fixes
4115-v2.patch (7.3 KB) - added by danielk 12 years ago.
Updated patch for trunk

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by danielk

Attachment: 4115-v1.patch added

Makes network control a bit safer

Changed 12 years ago by danielk

Attachment: 4115-fixes-v1.patch added

Backports network control changes to 0.20-fixes

comment:1 Changed 12 years ago by danielk

Description: modified (diff)
Type: defectpatch
Version: unknownhead

comment:2 Changed 12 years ago by danielk

(In [14777]) Refs #4115. Fixes segfault in "All Tuners are Busy" dialog handling and adds an "Exit" option.

I noticed two problems with the handling of this dialog when debugging the network control.

  • If you hit escape in this dialog it would select the first recording in the list for playback, rather than returning to the main menu.
  • If you send the "return" key to the frontend application when the frontend is not focused it segfaults.

The first problem occurs because the "reject" action of the showButtonPopup() function returns '0' the index of the first recording until this changeset added an exit option. The second problem occurs because there was no range checking on the return from showButtonPopup(). We really need to move to the new less buggy MythUI dialogs in the long term, but a little bit of range checking and adding an "Exit" option to this dialog are good ideas anyway.

Changed 12 years ago by danielk

Attachment: 4115-v2.patch added

Updated patch for trunk

comment:3 Changed 12 years ago by danielk

(In [14794]) Refs #4115. Fixes for Network Control segfaults in trunk.

There are two common causes of segfaults when using the telnet network control API:

  • The NetworkControl? class itself is not thread-safe in it QString usage
  • TV Play accepts commands during LiveTV buffer switching.

This fixes the first problem by adding a couple locking instances and using QDeepCopy when passing strings between threads. And this fixes the second problem by respecting the ignoreKeys variable, it by only executing network commands after the keypressed buffer is empty (this avoids back to back processing which doesn't give the RunTV main loop a chance to get into a consistent state between commands if a key and network command are both available in the same loop iteration.)

comment:4 Changed 12 years ago by danielk

(In [14858]) Refs #4115. Another set of related fixes for segfaults when using network control.

The first fix is for MythPopupBox::defaultButtonPressedHandler(). When the focus is not on the popup box, like when you're testing mythtv dialogs with using network control from another window, then this handler will return a value > than the number of buttons. Many users of the MythPopupBox? dialogs unfortunately do no range checking so this causes a segfault. But even if it didn't it returns the wrong value. This adds another loop which checks the button's isDown() return instead. This should always work, even when the dialog loses focus. But I also check if we found any pressed button from either and print an error message and return Reject if we don't.

The first fix get's us past one set of segfaults, but show2ButtonPopup() and showButtonPopup() are also called outside the Qt event thread at times and since they improperly delete a QObject (the MythPopupBox?) this causes segfaults when the Qt processes the defaultButtonPressedHandler() after the dialog is deleted. The fix is simply to use the proper QObject call to delete the dialog. This problem exists in the other static functions in MythPopupBox?, but I was able to reliable test the segfaults this caused in the two functions I fixed using network control (it causes segfaults, albeit randomly, in the same dialogs that I used to test the defaultButtonPressedHandler().) I'll fix the others after I locate test cases for them.

comment:5 Changed 12 years ago by danielk

(In [14862]) Refs #4115. Fixes some more Network Control segfaults.

This fixes segfaults like the second one described in commit [14858]. This happens anytime a signal arrives for slot in a dialog that has already been deleted. This can happen without Network Control, but is very rare in that case. With network control it is easy to send two commands back to back or send a key while the UI is not focused.

This only addresses this type of segfault for all uses of the MythDialog? class. I haven't had time to test the other dialogs.

comment:6 Changed 12 years ago by danielk

(In [14874]) Refs #4115. Refs #4132. Changes explicit deletes of three dialogs to deleteLater() calls.

Like [14862] which addressed this problem for MythDialog?, this addresses the delete problem for three children of MythDialog?, MythProgressDialog?, MythBusyDialog?, and DialogBox?. As explained in the comments on [14858] and [14862] deleting these dialogs explicitly can allow signals to arrive for the dialog after it has been deleted causing segfaults. This is hard to do when using a keyboard and mouse to provide input, but much easier to trigger when using network control to send key commands to the UI.

This only fixes 3 dialogs and the change appears much larger than it in fact is. I'm not doing all the dialogs at once so that I can roll back a just a few at a time if there are any problems. If you have an outside plugin that fails to compile after this changeset, you probably just need to change some 'delete dialogname;' expressions to 'dialogname->deleteLater();'.

This does change the myth binary revision so plugins will have to be recompiled.

comment:7 Changed 12 years ago by danielk

(In [14919]) Refs #4115. Backports the networkcontrol.cpp portion of [14794] to 0.20-fixes.

This simply uses the existing locking mechanism in the NetworkControl? class, along with QDeepCopy<QString> to fix some segfaults caused by sharing QStrings without locking.

comment:8 Changed 12 years ago by danielk

(In [14920]) Refs #4115. This backports fixes from head for two related mythdialog problems that cause segfaults when using Network Control.

This is a backport of [14858] and a long description can be seen there. Note the second fix for show*ButtonPopup?() represents a class of problems that are fixed much more fully in head, this just addresses these popups were responsible for a large number of segfaults.

comment:9 Changed 12 years ago by danielk

Resolution: fixed
Status: newclosed

(In [14921]) Fixes #4115. Backports tv_play.cpp portion of [14794] to fix the major cause of segfaults using network control in 0.20-fixes.

The problem here seems to be that Network Control was never updated when we changed how LiveTV worked in MythTV. For keyboard input we check the ignoreKeys boolean and ignore unsafe actions while it is set. But Network Control completely ignores this and so you can do anything you want during the ringbuffer switching.

Another problem is that if both keyboard control and network control are active two incompatible actions can be triggered in a single iteration of the TV main loop, this is fixed by simply skipping network control events if we have a keyboard event, until the next iteration of the loop.

Finally, there were a couple thread safety issues which were trivially fixed.

Note: See TracTickets for help on using tickets.