Opened 14 years ago
Closed 14 years ago
#7886 closed defect (fixed)
Backend crashes when invalid network request received
Reported by: | Owned by: | cpinkham | |
---|---|---|---|
Priority: | minor | Milestone: | 0.23 |
Component: | MythTV - General | Version: | 0.22-fixes |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
I've discovered that mythbackend can be crashed by a network request which includes an invalid number of parameters. My example case was using the perl bindings and calling new_program with a blank starttime (which itself makes a QUERY_RECORDING TIMESLOT request to the backend with a blank starttime). However, I've discovered a number of other cases where it can happen.
The source of the bug is in mainserver.cpp. ProcessRequestWork? reads a "QStringList listline" off of the socket. It then takes the first element (listline[0]) and splits it into "QStringList tokens". The listline and tokens variables are then sent to various request handlers, some of which access array indices without first checking the length (which is how my backend crash occurred).
By the way...when listline[0] is accessed in ProcessRequestWork?, it never verifies size() != 0 either. I'm not sure if it's possible for MythSocket::readStringList to create a zero length stringlist while still returning true. If so, this is another problem that needs to be fixed.
I've tried following through the code, to see which functions access without properly checking the array size. For the tokens variable, the only problematic function I saw was HandleQueryRecording?. I'm attatching a patch that fixes that.
For the listline variable, my patch also fixes two spots in ProcessRequestWork? that were problems...in the (command == "MESSAGE") block and (command == "SHUTDOWN_NOW") block. However, there were a number of other cases where the indices weren't verified (and a few that use QStringList::at(index), which I couldn't determine if that would segfault too) and I wasn't sure of the appropriate way to fix them (should they generate an error too, or proceed as if a blank line was sent instead) or if they needed fixing at all in the case of the at() call. So here is the list of (potential) problems I found that my patch does NOT address:
ProcessRequestWork?, in the (command == "BACKEND_MESSAGE") block
HandleSGGetFileList
HandleSGFileQuery
Attachments (1)
Change History (4)
Changed 14 years ago by
Attachment: | mainserver_cpp.patch added |
---|
comment:1 Changed 14 years ago by
Owner: | changed from Isaac Richards to cpinkham |
---|---|
Status: | new → assigned |
comment:2 Changed 14 years ago by
Milestone: | unknown → 0.23 |
---|
comment:3 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [23529]) Add some bounds checking to prevent a few potential master backend crashes.
Fixes #7886, but only the issues pointed out in the patch.